Created attachment 110961 [details] [review]
patch proposed

This enables the metadata dialog not to bring this "Unknown" strings to the edition.
BTW, it fixes some things like the sorting of "Unknown" columns as the "U" position, and is also the base I need for fixing bug 533218.
Can it be commited?
It includes changelogs.

IRC log of why this is not accepted:

(05:55:40 PM) gabaug [~gabe@24-136-31-88.mart-bsr1.chi-mart.il.cable.rcn.com] entered the room.
(05:55:41 PM) mode (+o gabaug ) by Rupert
gabaug gaminggeek garrett 
(05:59:10 PM) knocte: gabaug: good morning! do you have time to review a small patch? it's in http://bugzilla.gnome.org/show_bug.cgi?id=528493
(05:59:55 PM) foolish left the room (quit: Remote closed the connection).
(06:02:39 PM) batman [~janhaa@cm-84.208.84.128.getinternet.no] entered the room.
(06:03:03 PM) gabaug: knocte|off: sure
(06:03:11 PM) patoh left the room (quit: Remote closed the connection).
(06:03:43 PM) patoh [~patoh@CPE-30-176.dsl.onthenet.net] entered the room.
(06:06:30 PM) knocte: hyperair_: oh sorry, I just read the above and saw you were interested too, thanks
(06:07:18 PM) hyperair_: heh it's not like i have commit rights or anything
(06:07:24 PM) hyperair_: i was just curious to know what it was you were working on
(06:07:30 PM) hyperair_: =p
(06:07:31 PM) hyperair_ is now known as hyperair
(06:09:09 PM) knocte: :)  I hope it gets accepted, it would be my first contribution, and it would allow me to use banshee more easily because I don't want it to mess up my ID3tags, and it happens constantly (without this patch) if you happen to got the majority of your songs without metadata on the Album field
(06:10:15 PM) batman: hi
(06:10:46 PM) batman: is "File system organization" supposed to work on my videos as well?
(06:10:53 PM) batman: (1.0beta1)
(06:11:46 PM) gabaug: knocte|off: so what's your real problem, that mp3s you have that don't have artist or album set
(06:11:56 PM) gabaug: Banshee writes "Unknown Artist/Album" to the id3v2?
(06:12:32 PM) knocte: yeah, well, it proposes it when launching the metadata edition dialog
(06:12:39 PM) knocte: and I don't want to remove it constantly
(06:12:44 PM) gabaug: ahh
(06:12:47 PM) knocte: but BTW I have fixed more things
(06:12:49 PM) gabaug: so...why not change the editor
(06:12:53 PM) knocte: it's all in the changelog
(06:13:16 PM) gabaug: well, I want to talk about them
(06:13:19 PM) knocte: because i've realised it's easier just not to fake the DB
(06:13:25 PM) gabaug: b/c I'm not sure I'm seeing eye to eye on them
(06:13:34 PM) gabaug: but then you don't get sorting
(06:13:48 PM) knocte: yes, you get, because I've bound a new property
(06:13:56 PM) gabaug: no, you don't
(06:13:59 PM) knocte: not AlbumTitle but DisplayAlbumTitle
(06:14:00 PM) gabaug: b/c sorting is done in the db
(06:14:24 PM) gabaug: and in the db, you're just storing what, '' ?
(06:14:32 PM) knocte: anyway, you get sorting, because with this patch you get ''
(06:14:33 PM) gabaug: for CoreArtist.Name, eg?
(06:14:33 PM) knocte: right
(06:14:47 PM) knocte: it's better to sort an empty like empty, not like U
(06:14:51 PM) gabaug: well I don't consider having U before A sorting
(06:15:02 PM) knocte: no, but
(06:15:10 PM) knocte: I've put some prefixes 
(06:15:19 PM) gabaug: is that a really common thing in other apps, to put unknown at the top?
(06:15:33 PM) knocte: "[ Unknown Album ]" instead of "Unknown Album" because it's metainformation
(06:15:57 PM) knocte: I don't know but it doesn't make sense IMO
(06:16:08 PM) knocte: (to sort it like Unknown Album I mean)
(06:16:11 PM) hyperair: i agree with grouping unknown separately and not with U
(06:16:13 PM) jony|chow is now known as jony
(06:16:16 PM) hyperair: but instead of on top, perhaps at the bottom
(06:16:30 PM) gabaug: hyperair: heh
(06:16:33 PM) hyperair: i don't want an unsorted bunch of garbage at the top of my playlist
(06:16:38 PM) gabaug: right
(06:16:39 PM) hyperair: but then again that's just my opinion
(06:16:45 PM) knocte: hyperair: but how do you do that? the widget sort alphabetically
(06:16:46 PM) gabaug: you want it sorted, alphabetically :)
(06:16:54 PM) hyperair: hmm
(06:16:59 PM) hyperair: does [ go before A?
(06:17:04 PM) knocte: yep
(06:17:07 PM) knocte: and empty also
(06:17:08 PM) hyperair: damn
(06:17:11 PM) gabaug: I am entirely unconvinced for the need to change how we sort
(06:17:13 PM) hyperair: okay what goes after z?
(06:17:19 PM) foolish [~foolish@cBF6B47C1.dhcp.bluecom.no] entered the room.
(06:17:45 PM) hyperair: since nobody's answering me i'm going to find myself an ascii table to look at
(06:18:02 PM) ***gabaug smacks forehead
(06:18:49 PM) hyperair: what?
(06:18:49 PM) knocte: gabaug: but anyway I think replacing null strings with "unknown" strings in the DB is just wrong, because it makes fixing bugs like http://bugzilla.gnome.org/show_bug.cgi?id=533218 much harder
(06:19:54 PM) hyperair: wait a sec... [ goes before A? that's weird O_O
(06:20:01 PM) hyperair: i thougt it sorted by ascii table
(06:20:15 PM) knocte: and it makes easier to discard non-albums in the albums view (the change I made to DatabaseAlbumListModel.cs
(06:20:31 PM) hyperair: oh wait then it's not alphabetical order anymore
(06:20:31 PM) hyperair: aaaaaaaaaaah ignore me T_T
(06:20:45 PM) ***hyperair goes and sits in the corner to sort his head out
(06:20:51 PM) knocte: hyperair: indeed it's sorting by '', the [] is added later
(06:21:03 PM) gabaug: knocte|off: OK, we're not changing the sort order, sorry
(06:21:24 PM) gabaug: which means we are saving it in the database as Unknown Artist etc
(06:21:29 PM) gabaug: let's get back to your real problems
(06:21:40 PM) gabaug: which are 1) having to clear 'Unknown Artist' etc from the Track Editor
(06:21:56 PM) gabaug: just check if the track.ArtistName == 'Unknown Artist' and if so, don't preload the field
(06:22:11 PM) knocte: yeah, and fixing 533218 easily (without hardcoding "Unknown Album" everywhere)
(06:22:30 PM) knocte: I think it's not worth making that check everywhere in the code
(06:23:29 PM) knocte: it's indeed an architecture issue, we could do this change and hack a bit the ordering algorithm to put unknown albums the last
(06:23:38 PM) gabaug: your patch also breaks user's ability to search for artist:"Unknown Artist"
(06:23:43 PM) jony left the room (quit: Leaving).
(06:23:44 PM) gabaug: (and to have smart playlists like that)
(06:23:56 PM) knocte: is that really needed?
(06:24:07 PM) gabaug: yes
(06:24:14 PM) gabaug: b/c if you show one thing on the Track list
(06:24:19 PM) gabaug: but users can't search for it
(06:24:30 PM) gabaug: that's not good
(06:24:41 PM) knocte: a user that has his music with "Unknown artist" laying around is not going to bother about smart playlists
(06:25:04 PM) gabaug: maybe they will
(06:25:09 PM) gabaug: b/c those are the songs
(06:25:13 PM) knocte: unknown * should not be considered something you can search for, because it's not real information, it's information that banshee inserted there
(06:25:14 PM) gabaug: that they still haven't cleaned up the metadata on
(06:25:33 PM) hyperair: supposing a user wants to hunt down all untagged files, and fix them, then he would need to search for "unknown artist"
(06:25:57 PM) hyperair: that's just one of the use cases
(06:26:11 PM) knocte: it should be an option on a combobox or something because unknown is not the real text
(06:26:15 PM) gabaug: knocte|off: your patch may fix some hacks (having to check for/introduce 'Unknown Artist' in some places) but it introduces/requires plenty of its own
(06:26:35 PM) gabaug: knocte|off: nope, I'm absolutely against having an option for such a minor thing
(06:26:40 PM) knocte: for example I have an album from soundgarden that is called SuperUnknown, which may also collide with this banshee's behaviour
(06:26:52 PM) gabaug: um, no
(06:27:09 PM) gabaug: why would that 'collide'?
(06:27:17 PM) knocte: in the sense of the case that hyperair explained
(06:27:30 PM) gabaug: well sure, if you just search for 'unknown'
(06:27:34 PM) gabaug: not 'unknown artist'
(06:27:43 PM) knocte: someone thinks that it's sufficient to put "unknown" to search for their empty albums, and then fuck up the soundgarden metadata
(06:27:59 PM) gabaug: nope, I think you're making stuff up
(06:28:03 PM) gabaug: if a user did such a search
(06:28:12 PM) knocte: and what will happen the day in which some artist calls himself this way :)
(06:28:16 PM) gabaug: they'd see that it returned some tracks that already have data
(06:28:36 PM) gabaug: knocte|off: I find your scenarios very uninteresting
(06:28:48 PM) knocte: yeah but imagine they have a lot of data and they don't scroll to the last...
(06:28:52 PM) hyperair: search string: artist:"unknown artist"|album:"unknown album"
(06:28:57 PM) knocte: well, just showing how a hack can break things
(06:29:00 PM) gabaug: I'm concerned with having a good user experience for 99% of our users 99% of the time (or more, if possible)
(06:29:29 PM) knocte: then I'll have to live with a patched banshee-1, just for being in the 1%
(06:29:35 PM) gabaug: yep
(06:29:39 PM) lamalex_2 [~alex@c-71-225-68-151.hsd1.pa.comcast.net] entered the room.
(06:30:27 PM) knocte: will be the patch committed if I find a way to workaround the search algorithm and the unknown search?
(06:30:51 PM) hyperair: gabaug: i still think that the editor shouldn't automatically fill "unknown artist" into the tag
(06:30:56 PM) hyperair: and unknown album of course
(06:31:48 PM) gabaug: knocte|off: What are your *real* problems with how things currently are?  1) track editor pre-fills 'Unknown X'  2) 'Unknown X' is actually written to files' metadata ?
(06:31:55 PM) gabaug: are those accurate/any others?
(06:32:45 PM) knocte: 2) and having Unknown be ordered by U, it should be ordered the last item, having [ Unknown X]
(06:32:54 PM) gabaug: you've wrapped up too many changes into one thing for my taste
(06:33:06 PM) lamalex left the room (quit: Ping timeout: 600 seconds).
(06:33:31 PM) gabaug: ok, so that one is off the table
(06:33:35 PM) knocte: it's just that I feel I don't want to change the Metadata dialog if I don't need to, I wanted to fix the root of the problem
(06:33:39 PM) gabaug: are the other two still concerns of yours?

New wonderful idea from Gabriel:

(06:36:56 PM) gabaug: of being able to have the artist name be "The Beatles" but sort them by "Beatles, The"
(06:37:20 PM) gabaug: if we had another column in CoreArtists and CoreAlbums for SortName/Title
(06:37:21 PM) gabaug: or something
(06:37:33 PM) gabaug: we could put the 'Unknown Artist/Album' in there
(06:37:43 PM) knocte: mmmm, interesting
(06:37:51 PM) gabaug: and keep the actual CoreArtist.Name / CoreAlbum.Title null
(06:37:59 PM) gabaug: and search/smart playlists could search both
(06:38:01 PM) knocte: that's the real fix I think
(06:38:25 PM) gabaug: yeah, I'm much happier w/ that idea than putting "[Unknown..]" in the db
(06:38:45 PM) knocte: ok, I'll see if I have time to cook a new patch, thanks

Created attachment 111190 [details] [review]
new patch inspired in gabaug's comments, which is also a base patch for bugs 499650 and 533218

Created attachment 111774 [details] [review]
New version, dropping reordering

Created attachment 111785 [details] [review]
Corrected changelog of last patch

Aaron, can this patch be committed? I believe there are no more concerns you had with it wrt to our IRC conversations. Not having this committed is blocking me to hack on other bugs that depend on this patch.

Also, I think you have scheduled 1.2 for July right? And the milestone of this is 1.1. Please give me feedback or else the approval to commit.

Thanks.

The patch doesn't apply cleanly to the trunk anymore. I guess some recent changes in DefaultColumnController.cs broke it.

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

Created attachment 116832 [details] [review]
Updated patch with current HEAD

I did the following to test the patch :
1. Import an mp3 file with no ID3 tags (removed with EasyTag and checked with id3info)
2. Select this track in banshee
3. Open the track with the track editor
4. Do not modify anything
5. Click Save

After this, id3info shows that the file now has the tags "Album Title"="Unknown Album" and "Artist name"="Unknown Artist".

If I understood correctly, the purpose of your patch was to leave them empty in this case ?

(In reply to comment #11)
> I did the following to test the patch :
> 1. Import an mp3 file with no ID3 tags (removed with EasyTag and checked with
> id3info)
> 2. Select this track in banshee
> 3. Open the track with the track editor
> 4. Do not modify anything
> 5. Click Save
> 
> After this, id3info shows that the file now has the tags "Album Title"="Unknown
> Album" and "Artist name"="Unknown Artist".
> 
> If I understood correctly, the purpose of your patch was to leave them empty in
> this case ?
> 

Doh! really? Yes, they should be empty. I thought that, by not showing them in the track editor, it would save them as empty, but didn't test. I'll correct the patch. Thanks for testing.

Bertrand, I haven't been able to reproduce the behaviour you mention on comment#11.

What do you see when you do step 3? Do you see any "Unknown XXX" string in the track editor or is all empty?

Well, forget my comment. It turns out that my file had tags (ID3v2.4 ?) that id3info and easytag didn't see and banshee did. Those tags contain "Unknown Artist" and "Unknown Album", so I didn't notice.
Sorry about the confusion !

So I don't have an untagged mp3 file on my system to test your patch, and I don't trust any tagging programs anymore. ;)

Well, you can test with any file that has an empty field. The "Unknown XXX" strings are never saved to the file.

BTW, trying to empty fields myself in order to test the patch, I also found bug 549414 :(

So, i managed to find a file with no id3 tags.
With the patch applied, I imported it, and I have the following issues :

1. When I open the track editor, I have the following message repeated several times :

(Nereid.exe:30457): Pango-CRITICAL **: pango_layout_set_text: assertion `length == 0 || text != NULL' failed

2. After clicking "Save" in the track editor I have the following message :

[Warn  21:22:45.622] Caught an exception - Object reference not set to an instance of an object (in `Mono.Data.SqliteClient')
  at Mono.Data.SqliteClient.SqliteDataReader.GetString (Int32 i) [0x00000] in /var/tmp/portage/dev-lang/mono-1.9.1/work/mono-1.9.1/mcs/class/Mono.Data.SqliteClient/Mono.Data.SqliteClient/SqliteDataReader.cs:544 
  at Banshee.CoverArt.CoverArtJob.Run () [0x000b8] in /home/lorentz/Projets/banshee/src/Extensions/Banshee.CoverArt/Banshee.CoverArt/CoverArtJob.cs:133 


By the way, the MonoDevelop .mdp files were migrated to .csproj, so the patch needs to be updated for that.

Created attachment 118478 [details] [review]
New patch

It addresses:
- New extension .csproj
- Pango critical.

But I couldn't reproduce the Sqlite exception, could you ellaborate Bertrand?
BTW: I have now made a patch from src/, but the problem with this is that it doesn't include the ChangeLog.

There's bit of misunderstanding : patches should be made from the toplevel source directory, the one containing "src/", not from inside the "src/" directory.

I couldn't reproduce my Sqlite exception with the new patch, so I guess it was unrelated, or was fixed by something else.

Looks good to me, thanks !

The patch doesn't apply cleanly to trunk anymore.

Created attachment 123923 [details] [review]
new patch

This applies on trunk, and is done in the banshee folder.

Andres, thanks for all your work on this, but I don't like the basic idea - it is a lot of extra complexity and on-the-fly string calculations for a problem I don't think is a problem.  What is so bad about writing out 'Unknown Artist' etc until the user bothers to correct it?

And if that's your only problem, then just modify with a tiny bit of code the logic where we write out the metadata to s/Unknown *// first.  You could do that with a patch to core, or ideally w/ a patch to core that lets extensions hook into the write-metadata process.  Does that interest you, or should I close this WONTFIX?

(In reply to comment #21)
> Andres, thanks for all your work on this, but I don't like the basic idea - it
> is a lot of extra complexity and on-the-fly string calculations for a problem I

On the fly string calculations? I'm just moving every hardcoded "Unknown*" string to a centralized place. Please tell me if I'm missing something.


> don't think is a problem.  What is so bad about writing out 'Unknown Artist'
> etc until the user bothers to correct it?

Think about batch-updates: if you forget to remove the text of the field, a new value gets saved when you didn't wanted to. And this affects ordering in devices (for example, you would get that the "Unknown Album" is an album made by a lot of artists; not to mention that it causes other bugs like 550238).


> And if that's your only problem, then just modify with a tiny bit of code the
> logic where we write out the metadata to s/Unknown *// first.  You could do

I can propose a patch like that, but I really can't understand why you guys would accept such a hack. I mean, this would break for every artist/album/song that starts with "Unknown". I know it's not typical, but the algorithm would be a hack.

Can we have more people to comment on this one? The CC list has grown a bit.

(In reply to comment #22)
> (In reply to comment #21)
> > Andres, thanks for all your work on this, but I don't like the basic idea - it
> > is a lot of extra complexity and on-the-fly string calculations for a problem I
> 
> On the fly string calculations? I'm just moving every hardcoded "Unknown*"

You mean replacing every String.Empty element in every row for the "Unknown*" string? I can improve the patch making the 3 Unknown* strings static. I was just advocating a cleaner approach that makes the DB backend store an empty string when there's an empty field.

(In reply to comment #22)
> > And if that's your only problem, then just modify with a tiny bit of code the
> > logic where we write out the metadata to s/Unknown *// first.  You could do
> 
> I can propose a patch like that, but I really can't understand why you guys
> would accept such a hack. I mean, this would break for every artist/album/song
> that starts with "Unknown". I know it's not typical, but the algorithm would be
> a hack.

I've found another reason for not doing this hack: storing "Unknown*" strings in the DB is just plain wrong, because firstly it's i18n dependent. Let's say you upgrade your OS and with a different language (non-native english speakers like me do this frequently) but copy your database from the previous OS: you would end up with mixed "Unknown*" items. Therefore the "Unknown*" strings creation should be done at runtime and never be stored IMO.

Sorry for flooding the bug guys.

Unless there's some huge technical issue I'm not understanding, then as a user I think I agree with Andrés.  I'd really rather not have songs with no album data added to a nonexistant "Unknown Album" album. There are a lot of songs out there, Internet releases in particular, which *don't* belong to an album.  Having an album tag at all is false data.  More importantly, if tags are getting saved, Banshee's behavior should not impact what other software or portable players are going to do when sorting the same files by passing them fake info.

Created attachment 127746 [details] [review]
Improved version of the patch

I've improved the patch forcing the new class to call Catalog.GetString only once, instead of n times. Thanks Gabriel for reminding me about these efficiency problems, if you see more, please tell.

(In reply to comment #26)
> Created an attachment (id=127746) [edit]
> Improved version of the patch
> 
> I've improved the patch forcing the new class to call Catalog.GetString only
> once, instead of n times. Thanks Gabriel for reminding me about these
> efficiency problems, if you see more, please tell.

Andrés, your patch has been added as a text/plain attachment. It's probably a good idea to re-submit it as a patch so that it appears in BGO reports developers are running.

Bertrand, thanks for fixing it, I didn't know the attachment type can be changed.

Created attachment 129765 [details] [review]
Do not insert fake metadata into the database/file tags

With the new sorting code, the patch does not need to be nearly so complex. Simply add `Display*` properties, and use them for sorting / display.

Thanks for updating the patch John. I've tested it and it works.

FYI: if this bug is fixed, it would fix for free bug 569379 as well.

Created attachment 129815 [details] [review]
Do not insert fake metadata into the database/file tags (v2)

I updated the last patch to include database migration (from "Unknown..." strings to NULL values).
I also refactored it a bit to avoid some duplicated stuff.

Comment on attachment 129815 [details] [review]
Do not insert fake metadata into the database/file tags (v2)

>+            string unknown_artist_locale = Catalog.GetString (unknown_artist);
>+            Execute (String.Format ("UPDATE CoreArtists SET Name = null WHERE Name = '{0}'", unknown_artist_locale));

Wouldn't this cause a syntax error if the translated artist includes single quotes? Try Execute ("UPDATE ... WHERE Name = ?", unknown_artist_locale).

(In reply to comment #32)
> (From update of attachment 129815 [details] [review] [edit])
> >+            string unknown_artist_locale = Catalog.GetString (unknown_artist);
> >+            Execute (String.Format ("UPDATE CoreArtists SET Name = null WHERE Name = '{0}'", unknown_artist_locale));
> 
> Wouldn't this cause a syntax error if the translated artist includes single
> quotes? Try Execute ("UPDATE ... WHERE Name = ?", unknown_artist_locale).


I don't get it. Why someone would translate a string that doesn't have quotes into one that have them? Ok, if you want to be super-paranoid... ;)

Andrés(In reply to comment #33)
> I don't get it. Why someone would translate a string that doesn't have quotes
> into one that have them? Ok, if you want to be super-paranoid... ;)
> 
Andrés, it's never a good idea to construct queries using string concatenation. SQL injection attacks may be not an issue in case of Banshee, but they are bad enough to have this habit.

Just always use parameters :)

(In reply to comment #34)
> Andrés(In reply to comment #33)
> > I don't get it. Why someone would translate a string that doesn't have quotes
> > into one that have them? Ok, if you want to be super-paranoid... ;)
> > 
> Andrés, it's never a good idea to construct queries using string
> concatenation. SQL injection attacks may be not an issue in case of Banshee,
> but they are bad enough to have this habit.
> 
> Just always use parameters :)

Haha, paranoia all around :)

Good point, however in this case it doesn't apply because the value doesn't come from the user input, so SQL injection is 0% probable ;)

(In reply to comment #35)
> Haha, paranoia all around :)
> 
> Good point, however in this case it doesn't apply because the value doesn't
> come from the user input, so SQL injection is 0% probable ;)
> 
Improbable != impossible ;) Plus, that code could be copy/pasted by someone to actually handle the user input. 

Besides, paranoia is a valuable survival skill in our trade :)

Furthermore, you never know what translators manage to come up with…

Created attachment 129856 [details] [review]
Do not insert fake metadata into the database/file tags (v3)

Addressed all the comments. Thanks for the suggestions guys :)

> I don't get it. Why someone would translate a string that doesn't have quotes
> into one that have them? Ok, if you want to be super-paranoid... ;)
Single quotes are used as punctuation in many languages, and even letters in some. As far as I'm concerned, it's user-generated input.

You should never do Catalog.GetString (unknown_artist) - in this case it will work b/c "Unknown Artist" etc are already explicitly translated elsewhere, but it's not a good habit/assumption.  Even better would be to use the existing static translated strings (ArtistInfo.UnknownArtist etc).  Maybe could make them public static readonly?  And rename to UnknownArtistName etc.

Also, instead of doing two queries per, replace Name = ? with Name IN (?, ?) in one query per.

+            get {
+                return StringUtil.MaybeFallback (Name, UnknownArtist);
+            }

and the like should be one single line.

null_select_command is unnecessary - just pass a null to the select_command and it'll do the same thing.  So no changes should be necessary to DatabaseArtistInfo.cs and DatabaseAlbumInfo.cs except "album.Title = null;
" and the "?? Display*" fallbacks, right?

The Unknown* values get sorted appropriately, but can I search for them with "Unknown Artist"?  That is, are the *Lowered (SearchKey) values using the temporary/"fictional" data?

"Unknown Genre" is really long, should we just show them as blank?  Also makes sense since we don't sort them under U.

(In reply to comment #40)
> You should never do Catalog.GetString (unknown_artist) - in this case it will
> work b/c "Unknown Artist" etc are already explicitly translated elsewhere, but
> it's not a good habit/assumption.  Even better would be to use the existing
> static translated strings (ArtistInfo.UnknownArtist etc).  Maybe could make
> them public static readonly?  And rename to UnknownArtistName etc.

Sounds good.


> Also, instead of doing two queries per, replace Name = ? with Name IN (?, ?) in
> one query per.
> 
> +            get {
> +                return StringUtil.MaybeFallback (Name, UnknownArtist);
> +            }
> 
> and the like should be one single line.
> 
> null_select_command is unnecessary - just pass a null to the select_command and
> it'll do the same thing.  So no changes should be necessary to
> DatabaseArtistInfo.cs and DatabaseAlbumInfo.cs except "album.Title = null;
> " and the "?? Display*" fallbacks, right?

Mmm, John introduced this command. John?


> The Unknown* values get sorted appropriately, but can I search for them with
> "Unknown Artist"?  That is, are the *Lowered (SearchKey) values using the
> temporary/"fictional" data?

Yes, they are searchable (well, last time I tried, I'll check again with the new versions of the patches after the SearchKey commits).

> "Unknown Genre" is really long, should we just show them as blank?  Also makes
> sense since we don't sort them under U.

Aye, I prefer it blank too.

Thanks Gabriel, I'll address the comments this evening and post a new patch.


(In reply to comment #39)
> > I don't get it. Why someone would translate a string that doesn't have quotes
> > into one that have them? Ok, if you want to be super-paranoid... ;)
> Single quotes are used as punctuation in many languages, and even letters in
> some. As far as I'm concerned, it's user-generated input.

True true :)  Even English has a lot of single quotes (but not in this case).

> Also, instead of doing two queries per, replace Name = ? with Name IN (?, ?) in
> one query per.
I don't understand what this means. Clarification, please?

> null_select_command is unnecessary - just pass a null to the select_command
> and it'll do the same thing.  So no changes should be necessary to
> DatabaseArtistInfo.cs and DatabaseAlbumInfo.cs except "album.Title = null;
> " and the "?? Display*" fallbacks, right?
In SQL, "<anything> = NULL" is always NULL. To compare against NULL, the special cases "IS NULL" and "IS NOT NULL" are required.

For example, an SQLite session:

sqlite> .nullvalue NULL
sqlite> select 'a' = 'a';
1
sqlite> select 'a' = 'b';
0
sqlite> select 'a' = null;
NULL
sqlite> select 'a' is null;
0
sqlite> select 'a' is not null;
1

> The Unknown* values get sorted appropriately, but can I search for them with
> "Unknown Artist"?  That is, are the *Lowered (SearchKey) values using the
> temporary/"fictional" data?
Ah, good point. The `*Lowered` properties should be updated to use `Display*`.

I wonder if a new version counter should be introduced for keeping track of search keys. This bug, and bug 573484, require incrementing the database version just to refresh the search keys. They will likely change even more, as the key generation algorithm is tweaked.

Created attachment 130095 [details] [review]
Do not insert fake metadata into the database/file tags (v4)

(In reply to comment #42)
> > Also, instead of doing two queries per, replace Name = ? with Name IN (?, ?) in
> > one query per.
> I don't understand what this means. Clarification, please?

UPDATE ... WHERE Name = 'A'
UPDATE ... WHERE Name = 'B'
is the same to
UPDATE ... WHERE Name IN ('A', 'B')

I've done it on this new patch.


> > null_select_command is unnecessary - just pass a null to the select_command
> > and it'll do the same thing.  So no changes should be necessary to
> > DatabaseArtistInfo.cs and DatabaseAlbumInfo.cs except "album.Title = null;
> > " and the "?? Display*" fallbacks, right?
> In SQL, "<anything> = NULL" is always NULL. To compare against NULL, the
> special cases "IS NULL" and "IS NOT NULL" are required.
> 
> For example, an SQLite session:
> 
> sqlite> .nullvalue NULL
> sqlite> select 'a' = 'a';
> 1
> sqlite> select 'a' = 'b';
> 0
> sqlite> select 'a' = null;
> NULL
> sqlite> select 'a' is null;
> 0
> sqlite> select 'a' is not null;
> 1

Then I left your new commands intact in this new patch.


> > The Unknown* values get sorted appropriately, but can I search for them with
> > "Unknown Artist"?  That is, are the *Lowered (SearchKey) values using the
> > temporary/"fictional" data?
> Ah, good point. The `*Lowered` properties should be updated to use `Display*`.

True, I corrected this in this new patch.

> I wonder if a new version counter should be introduced for keeping track of
> search keys. This bug, and bug 573484, require incrementing the database
> version just to refresh the search keys. They will likely change even more, as
> the key generation algorithm is tweaked.

I've also added *Lowered updates. It's not the same operation as the one in bug 573484 though (because there the param to HYENA_SEARCH_KEY is the value of another column, here not).


(In reply to comment #41)
> (In reply to comment #40)
> > You should never do Catalog.GetString (unknown_artist) - in this case it will
> > work b/c "Unknown Artist" etc are already explicitly translated elsewhere, but
> > it's not a good habit/assumption.  Even better would be to use the existing
> > static translated strings (ArtistInfo.UnknownArtist etc).  Maybe could make
> > them public static readonly?  And rename to UnknownArtistName etc.
> 
> Sounds good.
> 

Done in this patch.


> > Also, instead of doing two queries per, replace Name = ? with Name IN (?, ?) in
> > one query per.

Done in this patch.

> > +            get {
> > +                return StringUtil.MaybeFallback (Name, UnknownArtist);
> > +            }
> > 
> > and the like should be one single line.

Done in this patch.


> > null_select_command is unnecessary - just pass a null to the select_command and
> > it'll do the same thing.  So no changes should be necessary to
> > DatabaseArtistInfo.cs and DatabaseAlbumInfo.cs except "album.Title = null;
> > " and the "?? Display*" fallbacks, right?

Not done, see above.


> 
> > The Unknown* values get sorted appropriately, but can I search for them with
> > "Unknown Artist"?  That is, are the *Lowered (SearchKey) values using the
> > temporary/"fictional" data?
> 
> Yes, they are searchable (well, last time I tried, I'll check again with the
> new versions of the patches after the SearchKey commits).

Corrected in this patch, see above.


> > "Unknown Genre" is really long, should we just show them as blank?  Also makes
> > sense since we don't sort them under U.
> 
> Aye, I prefer it blank too.

Done in this patch.

Thanks Andres, looking good.

We don't need the DisplayGenre stuff at all do we?  Let's just remove it entirely and use the Genre property (also in the GengreField change).

In ArtistInfo.cs there is a 3 line property getter.

Change UnknownAlbum to UnknownAlbumTitle and UnknownArtist to UnknownArtistName, please.

Not positive it'll catch anything, but I think it won't hurt: please change all the "IN (?, ?)" to "IN (NULL, '', ?, ?)"

Just those nitpicks and I think this is ready.  Thanks, guys!

> Not positive it'll catch anything, but I think it won't hurt: please change all
> the "IN (?, ?)" to "IN (NULL, '', ?, ?)"

sqlite> select NULL IN (NULL, '');
NULL

If you'd like to check for values that are already NULL, you have to use:

<field> IN ('', ?, ?) OR <field> IS NULL

Oh dear, sorry I'm being so dense.  :)

Thanks all for the comments.

(In reply to comment #44)
> Thanks Andres, looking good.
> 
> We don't need the DisplayGenre stuff at all do we?  Let's just remove it
> entirely and use the Genre property (also in the GengreField change).

DisplayGenre is there because it's defined in ITrackInfo and TrackInfo implements it. And I'm not sure if I should remove it from ITrackInfo because that seems to be used by DBUS. How should I proceed then?

Ok, yeah looks like it's important for FileNamePattern as well.  So keep your change to it in TrackInfo, but the GenreField change can be removed, right?

Created attachment 130164 [details] [review]
Do not insert fake metadata into the database/file tags (v5)

Addresses the last comments.

Created attachment 130168 [details] [review]
Do not insert fake metadata into the database/file tags (v6)

Hugh, we forgot to update the unit tests! I hope this is the last one :)

(In reply to comment #44)
> Thanks Andres, looking good.
> (...)
> Just those nitpicks and I think this is ready.  Thanks, guys!

Hey Gabriel, how does the last patch look? Sorry that I didn't said it explicitly: I addressed all the last comments. Just in case:


(In reply to comment #48)
> Ok, yeah looks like it's important for FileNamePattern as well.  So keep your
> change to it in TrackInfo, but the GenreField change can be removed, right?

Yep, I removed the change to GenreField and conserved DisplayGenre.


(In reply to comment #44)
> In ArtistInfo.cs there is a 3 line property getter.

Fixed as well.


> Change UnknownAlbum to UnknownAlbumTitle and UnknownArtist to
> UnknownArtistName, please.

Fixed.


> Not positive it'll catch anything, but I think it won't hurt: please change all
> the "IN (?, ?)" to "IN (NULL, '', ?, ?)"

Included.


So, can this be committed? And, if yes, could someone do it on behalf of me? :)
Thanks!

Thanks so much guys, this is a really good patch and a really solid improvement to Banshee.  Committed!