Created attachment 67920 [details] [review]
initial patch

This patch makes Rhythmbox not use "Unknown" for missing tags. There are several things that need to be discussed/solved:

* The patch treats no tag the same as an empty tag. The main reason is that it would then require a lot of code to check for NULL and special case it.
* DAAP sharing should just not send a piece of metadata rather than an empty string.
* Song info windows currently treat a field being empty as "don't change". This is particularly important when multiple entries are being modified, and their values are different
* Do we need an "is blank" option for auto playlists, or just allow "equals ''" to work?
* Probably lots of other things...

> * The patch treats no tag the same as an empty tag. The main reason is that it
> would then require a lot of code to check for NULL and special case it.

Will this allow to distinguish empty tag from missing tag on playlist view
and how does this affect on playlist sorting?

(In reply to comment #2)
> Will this allow to distinguish empty tag from missing tag on playlist view
> and how does this affect on playlist sorting?


It doesn't distinguish them, it treats then identically. Having them be different will make UI more complicated in many places, as we will need some indication of which it is, and complicated editing UI too.

Can anyone give me an example of when you'd actually want to have "" stored in a file's metadata, and how it should be treated differently from no tag? As I can't really think of any, I don't know how it should affect things.

(In reply to comment #3)

> It doesn't distinguish them, it treats then identically. Having them be
> different will make UI more complicated in many places, as we will need some
> indication of which it is, and complicated editing UI too.
> 
> Can anyone give me an example of when you'd actually want to have "" stored in
> a file's metadata, and how it should be treated differently from no tag? As I
> can't really think of any, I don't know how it should affect things.

It's not so much that you'd *want* this, and it would be fine if everything was within the rhythmbox universe.   But when you are importing mp3s that have been ripped elsewhere (e.g. under iTunes or god forbid some ugly software like MusicMatch) that the ID3 tags added blank tags rather than simply not tagging them at all.

In the case you are cleaning up a lot of files like that, you'd want to be able to find all the tags that are simply set to blank vs. all the tags where they weren't set at all.  It's also possible that an album title is actually "" (think John Cage type music).  

I'd prefer that the distinction be noted somehow, but having this patch applied in the meantime (haven't tested it yet), will probably be a step forward.

Just tested, I notice that the music will need to be re-imported for this to take effect.  Perhaps the version of the rhythmdb.xml could be bumped and file metadata be reimported upon the application of this patch.

Also justed noticed that the Year is still marked as "Unknown".  

In terms of UI for this, I suggest a different colour for that cell in the interface (both in song info and in entry view) for actual missing tag (vs. "").

Patch doesn't apply cleanly against current CVS.

Mentioned in https://launchpad.net/distros/ubuntu/+source/rhythmbox/+bug/58941 as well.

Any chance this patch could be updated against CVS?

Created attachment 73826 [details] [review]
updated patch

Updated to cvs. Duration, bitrate and mime-type still use "unknown", because they aren't directly extracted from the file.

Hmm, still doesn't apply cleanly.  I am also using the lastfm patch on bug #313049.  Perhaps they conflict?


$ patch -p0 < patches-manual/active/rb-metadata-unknown.diff
patching file daapsharing/rb-daap-connection.c
patching file lib/rb-cut-and-paste-code.c
patching file lib/rb-util.c
patching file metadata/rb-metadata-gst.c
Hunk #1 succeeded at 1278 (offset 1 line).
patching file plugins/artdisplay/artdisplay/AmazonCoverArtSearch.py
patching file plugins/artdisplay/artdisplay/CoverArtDatabase.py
patching file plugins/audioscrobbler/rb-audioscrobbler.c
Hunk #2 succeeded at 1022 (offset -1 lines).
patching file podcast/rb-feed-podcast-properties-dialog.c
patching file podcast/rb-podcast-manager.c
patching file podcast/rb-podcast-properties-dialog.c
patching file rhythmdb/rb-refstring.c
patching file rhythmdb/rhythmdb-tree.c
patching file rhythmdb/rhythmdb.c
Hunk #1 succeeded at 1293 (offset 37 lines).
Hunk #3 succeeded at 2577 (offset 37 lines).
patching file shell/rb-shell-player.c
Hunk #1 FAILED at 2816.
1 out of 1 hunk FAILED -- saving rejects to file shell/rb-shell-player.c.rej
patching file sources/rb-audiocd-source.c
Hunk #1 succeeded at 142 (offset 3 lines).
Hunk #2 succeeded at 196 (offset 1 line).
patching file sources/rb-ipod-source.c
Hunk #1 succeeded at 230 (offset 2 lines).
patching file sources/rb-iradio-source.c
Hunk #1 succeeded at 547 (offset 68 lines).
patching file widgets/rb-entry-view.c
patching file widgets/rb-song-info.c

(In reply to comment #11)
> Hmm, still doesn't apply cleanly.  I am also using the lastfm patch on bug
> #313049.  Perhaps they conflict?

Actually, no.  I got a clean copy of rb-shell-player.c from CVS and the patch still fails.  I think this change in CVS broke the patch:

2006-10-02  Jonathan Matthew  <jonathan@kaolin.wh9.net>

....
        * shell/Makefile.am:
        * shell/rb-shell-player.c: (rb_shell_player_class_init),
        (rb_shell_player_constructor), (rb_shell_player_init),
        (rb_shell_player_get_property), (open_location_thread),
        (rb_shell_player_open_location),
        (rb_shell_player_sync_with_source),
        (rb_shell_player_set_playing_source_internal), (tick_cb):
        * shell/rb-shell.c: (construct_widgets), (construct_sources),
        (rb_shell_player_elapsed_changed_cb):
        Remove iradio-specific code, use extra metadata fields for streaming
        title and artist name.

Actually, the patch seems to work for me if I ignore the rejection on the rb-shell-player.c file as suggested by Doc on IRC.

As I said before, itt might be the case that there is a track title/album/genre that is truly the empty string "" which wouldn't be missing data.  It would therefore not be possible to create (say) an autoplaylist for all missing tags without it including tags that were the empty string on purpose.

However the behaviour of this patch is infinitely better than the old behaviour so I'd say commit it and we can worry about that corner case later.

Do not use patch 73826!  It will blank your db.  Needs work.

On IRC, Jonathan Matthew pointed out that:

if (str == NULL || strcmp (str, ""))

is missing the "== 0" and should be:

if (str == NULL || strcmp (str, "") == 0)

i was about to fill a bug report but that one seems to be the good place to do it.

I'm using RB 0.11.5 / Ubuntu Hardy

When i want to edit a tag to fill the genre of the song, i've first to erase the "unknown" string before to complete the genre
It means :
1. selecting the field
2. selecting the string
3. pressing the delete key
4. then filling the field with the genre
It's very boring.

I'd like to see the "unknown" string disappear by itself when selecting the field.
It would mean from a user point of view :
1. selecting the field
2. filling the field with the genre

This would save lots of time

For a mockup, you can look at search field in Firefox 3 for instance : there is a "google" string (or any other search website) in light grey in the search field that dispappear when the user select the field.

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

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/rhythmbox/issues/128.