Created attachment 150047 [details] [review]
Patch for Comment tag support

Patch which adds support for the Comment tag:
- Preferences check button toggle for enabling/disabling it
- Comment field displayed in library, playlists and non side-pane play queue
- Multi-line editing of comment field
- Sorting by comment field
- Support for comment field in Rhythmbox database

I enabled "single-paragraph-mode" in rb-entry-view.c for comment field so that multi-line comments display a 000A character instead of an actual newline.  I figured it didn't hurt to apply this to all entry view fields, though it might make sense to just use it with the comment tag.

Review of attachment 150047 [details] [review]:

Not sure why it took me so long to get to this.. sorry.  Generally looks good.  I think the one thing that's missing is support for extracting comment tags from files.  This would need to be done in metadata/rb-metadata-gst-common.c, along with a new RB_METADATA_FIELD_ value in metadata/rb-metadata.h and metadata/rb-metadata.c.

It extracts comments for me and I'm pretty sure the patch is complete.  It looks like, RB_METADATA_FIELD_COMMENT is already in metadata/rb-metadata.h and processed in metadata/rb-metadata-gst-common.c with rhythmbox-0.12.6, it just needed to be added to the rest of the application.

I just remembered that there was one change I wanted to make to the comment GtkTextView.  It wasn't embedded in a GtkScrolledWindow before, which caused the dialog to expand when typing in the comment field.  I just made this change and have attached a new updated patch, obsoleting the previous one.

Created attachment 161570 [details] [review]
Patch for Comment tag support with scrollable GtkTextView

patch doesn't work after git clone:
$ git clone git://git.gnome.org/rhythmbox
Initialized empty Git repository in rhythmbox/.git/
remote: Counting objects: 50523, done.
remote: Compressing objects: 100% (13446/13446), done.
remote: Total 50523 (delta 41241), reused 44798 (delta 36884)
Receiving objects: 100% (50523/50523), 26.57 MiB | 698 KiB/s, done.
Resolving deltas: 100% (41241/41241), done.
$ cd rhythmbox/
rhythmbox $ patch -p1 < ../rhythmbox-comment-v2.patch
patching file backends/gstreamer/rb-encoder-gst.c
Hunk #1 succeeded at 460 (offset 19 lines).
patching file data/ui/general-prefs.ui
patching file data/ui/song-info.ui
patching file rhythmdb/rhythmdb-private.h
patching file rhythmdb/rhythmdb-tree.c
Hunk #1 succeeded at 976 with fuzz 2 (offset 18 lines).
patching file rhythmdb/rhythmdb.c
Hunk #1 succeeded at 576 (offset 34 lines).
Hunk #2 succeeded at 1661 (offset 105 lines).
Hunk #3 succeeded at 1858 (offset 105 lines).
Hunk #4 FAILED at 1990.
Hunk #5 succeeded at 3558 (offset 29 lines).
Hunk #6 succeeded at 4423 (offset 122 lines).
Hunk #7 succeeded at 5256 (offset 180 lines).
Hunk #8 succeeded at 5380 (offset 197 lines).
1 out of 8 hunks FAILED -- saving rejects to file rhythmdb/rhythmdb.c.rej
patching file rhythmdb/rhythmdb.h
patching file shell/rb-shell-preferences.c
Hunk #2 succeeded at 217 (offset 3 lines).
Hunk #3 succeeded at 453 (offset 3 lines).
Hunk #4 succeeded at 542 (offset 3 lines).
patching file sources/rb-browser-source.c
patching file sources/rb-playlist-source.c
Hunk #1 succeeded at 370 (offset 23 lines).
patching file widgets/rb-entry-view.c
Hunk #1 succeeded at 1348 (offset 16 lines).
Hunk #2 succeeded at 1409 (offset 16 lines).
Hunk #3 succeeded at 1524 (offset 16 lines).
Hunk #4 succeeded at 1677 (offset 16 lines).
Hunk #5 succeeded at 2682 (offset 28 lines).
patching file widgets/rb-entry-view.h
patching file widgets/rb-song-info.c
Hunk #2 succeeded at 491 (offset 5 lines).
Hunk #3 succeeded at 508 with fuzz 1 (offset 8 lines).
Hunk #4 succeeded at 530 (offset 10 lines).
Hunk #5 succeeded at 567 (offset 18 lines).
Hunk #6 succeeded at 1045 (offset 43 lines).
Hunk #7 succeeded at 1628 with fuzz 2 (offset 88 lines).
Hunk #8 succeeded at 1638 (offset 88 lines).
Hunk #9 succeeded at 1650 with fuzz 2 (offset 90 lines).
Hunk #10 succeeded at 1784 (offset 114 lines).
Hunk #11 succeeded at 1804 (offset 114 lines).

Any ideas what I did wrong?

OK I re-read the history - patch is for 0.12.6 and works with that.

*** Bug 591500 has been marked as a duplicate of this bug. ***

I updated this to apply to git master, added support for the comment tag in auto playlist criteria, and pushed it as commit ad6f038.

I think this patch has a few problems:
First, and most importantly, it seems to break id3 tag writing and leave edited files with NO id3 tag at all. Can someone build this and check - I used id3v2 before and after changing the comment field on a test file.
Second, (and relatively minor) it doesn't read existing comment tags in files already in the database - I can only find the brute-force method of removing the rhythmbox database and re-scan everything.
Third, my extra plugins no longer work (I have one called PlayPause which stops playback at the end of each track)

(In reply to comment #10)
> I think this patch has a few problems:
> First, and most importantly, it seems to break id3 tag writing and leave edited
> files with NO id3 tag at all. Can someone build this and check - I used id3v2
> before and after changing the comment field on a test file.

The id3v2 command does not support id3 v2.4, which is the version rhythmbox writes.

> Second, (and relatively minor) it doesn't read existing comment tags in files
> already in the database - I can only find the brute-force method of removing
> the rhythmbox database and re-scan everything.

After some other upcoming changes, I'll add some code to cause a full tag re-scan.  Until then, 'find /path/to/library -type f -exec touch {} \;'.

> Third, my extra plugins no longer work (I have one called PlayPause which stops
> playback at the end of each track)

I haven't seen any problems like this.

If you were previously using a packaged version of rhythmbox, it's possible that you built the version you're using now without python plugin support.