GNOME Bugzilla – Bug 573484
Searches with multiple spaces should be shrinked to one space
Last modified: 2009-03-21 01:42:08 UTC
Please describe the problem: While discussing on bug 570312, we found out a special case that would be interesting to fix on string searches. Steps to reproduce: 1. Have an album called "A B". 2. Create a new smartplaylist with the album filter to be "A B " (yes, two spaces in between and a space at the end). Actual results: The search doesn't yield results. Expected results: Banshee should be a bit smarter. Does this happen every time? Yes. Other information:
Created attachment 129692 [details] [review] fix Includes unit tests.
Why are you incrementing the database version ? Re-running Migrate_23 will probably just fail.
(In reply to comment #2) > Why are you incrementing the database version ? Because if the SearchKey() function changes, the StringQueryValue fields change as well. Look at the patch committed in bug for an example 528498. > Re-running Migrate_23 will probably just fail. Why? Migrate_26 does exactly the same operation as Mig_23, because the fields that use SearchKey are the same. So I thought it would be better to reuse.
> (In reply to comment #2) > ... Look at the patch committed in bug for an example 528498. Sorry, I meant to say "Look at the patch committed in bug 528498 for an example".
Ah, got it : you actually want to call Migrate_24, that's the one which does the updates using HYENA_SEARCH_KEY. The version number got changed when the patch in bug #528498 was committed.
Created attachment 129751 [details] [review] fix v2 (In reply to comment #5) > Ah, got it : you actually want to call Migrate_24, that's the one which does > the updates using HYENA_SEARCH_KEY. > The version number got changed when the patch in bug #528498 was committed. > Oh, that's true! Thanks for catching that. I've updated the patch fixing this and adding one more test (which caused me to change slightly the algorithm again!) :)
Created attachment 129767 [details] [review] Collapse spaces in `SearchKey()` The tests would be better placed in `Hyena/Hyena/Tests/StringUtilTests.cs` with the other `SearchKey`-related tests. This will also let you avoid mixing unrelated SQL stuff into the test. Did you mix in another change to `SearchKey()` in your patch? The actual changes required are minimal, and I don't understand what's going on in your version.
(In reply to comment #7) > Created an attachment (id=129767) [edit] > Collapse spaces in `SearchKey()` Thanks for your patch! > The tests would be better placed in `Hyena/Hyena/Tests/StringUtilTests.cs` with > the other `SearchKey`-related tests. This will also let you avoid mixing > unrelated SQL stuff into the test. Mmm, agreed. > Did you mix in another change to `SearchKey()` in your patch? The actual > changes required are minimal, and I don't understand what's going on in your > version. What happened is that I implemented it firstly (v1) using the "previous_was_space" bool, then I realised the patch v1 didn't trim spaces if there was punctuation character stripped in between ("a % b" would result in "a b" instead of "a b"), so I refactored it and found myself setting the previous_was_space in all the places where there was an Append operation, so I decided to centralized the Append operation in just one place, using a helper "to_append" nullable var. But, now that I see your version, I like your approach better :) The only disadvantage of your version is that it wouldn't work if someone in the future adds a space or a tab as a value of an entry in the ignored_special_cases dictionary. So maybe Gabriel will combine our patches again to cover this hypothetical case? :)
> The only disadvantage of your version is that it wouldn't work if someone in > the future adds a space or a tab as a value of an entry in the > ignored_special_cases dictionary. So maybe Gabriel will combine our patches > again to cover this hypothetical case? :) I don't think this would be a problem. Remember that "continue" will cause the rest of the loop body to be skipped. Spaces and tab characters would never be checked against `ignored_special_cases`.
(In reply to comment #9) > > The only disadvantage of your version is that it wouldn't work if someone in > > the future adds a space or a tab as a value of an entry in the > > ignored_special_cases dictionary. So maybe Gabriel will combine our patches > > again to cover this hypothetical case? :) > I don't think this would be a problem. Remember that "continue" will cause the > rest of the loop body to be skipped. Spaces and tab characters would never be > checked against `ignored_special_cases`. I don't mean spaces/tabs as keys of `ignored_special_cases`, but *values* (so, not checked, but returned).
Created attachment 129802 [details] [review] Collapse spaces in `SearchKey()` (v2) > I don't mean spaces/tabs as keys of `ignored_special_cases`, but *values* (so, > not checked, but returned). Ah, good point. New patch to fix this potential issue.
It looks good to me. Should it be committed or we need more reviews?
Created attachment 131060 [details] [review] Collapse spaces in `SearchKey()` (v3) * Change Trim() to TrimStart(). * Update the database migration for new behavior of Title/Name database fields introduced in bug 528493. * Add a comment for the interaction of special case and whitespace detection.
Thanks John, looks good.