GNOME Bugzilla – Bug 694975
Fix moving when mixing sorted and unsorted providers
Last modified: 2013-04-02 19:48:16 UTC
If you reset GSettings, or set to [], and then go to the search panel and try to move a provider that is not sorted explicitly, you'll see it jump at the beginning if moving up, and somewhere in between if moving down. The patches here include a semantic change that needs coordination with gnome-shell. See bug 694974.
Created attachment 237771 [details] [review] search: don't keep the last panel last in the sort order Previously, we kept the last panel in the configured order last, after non configured ones. This created sort orders that did not reflect the visual order, and caused problems when moving.
Created attachment 237772 [details] [review] search: fix moving unsorted applications Search providers can either have an explicitly configured sort order, or be in the default alphabetical order based on their name. Previously, when moving we would always assume that all providers had a sort order, so moving an unsorted provider would put at the top. The right thing to do, instead, is to walk the list in visual order and assign an appropriate sort order, that is then flushed to gsettings.
Review of attachment 237771 [details] [review]: "when moving". When moving what? Please be more explicit, explain which sort of sort orders would be created in those cases.
Review of attachment 237772 [details] [review]: I'd need far more coffee to review that. ::: panels/search/cc-search-panel.c @@ +250,1 @@ + /* For simplicity, first set all sort orders to the previously visible state, "For simplicity's sake, set all sort orders to the previously visible state first,"
Created attachment 238009 [details] [review] search: don't keep the last panel last in the sort order Previously, we kept the last panel in the configured order last, after non configured ones. This would assign the last panel a sort order in the hash table that would place it in the middle, but visually sort it last, and that caused problems when moving it (it would jump into an apparently random position in the middle, according to the sort ID, and would move the nearby provider too).
Review of attachment 238009 [details] [review]: "This would assign the last panel a sort order" should be: "This would assign a sort order to the last panel" Looks fine to commit after that.
Review of attachment 237772 [details] [review]: ::: panels/search/cc-search-panel.c @@ +262,2 @@ { + idx = 1; Explain the value here. @@ +265,3 @@ + } + + while (last_good_app != target_app) I don't like these sort of while loops. Can you make this into a for loop? I also don't like the pointer comparison when the original app-info is lost in code.
(In reply to comment #7) > Review of attachment 237772 [details] [review]: > > @@ +265,3 @@ > + } > + > + while (last_good_app != target_app) > > I don't like these sort of while loops. Can you make this into a for loop? > > I also don't like the pointer comparison when the original app-info is lost in > code. Uhm... I can't think of another way to put it. A for() would have weird conditions, because the comparison is in app ids, but the advancement is in a list.
Created attachment 238536 [details] [review] search: fix moving unsorted applications Search providers can either have an explicitly configured sort order, or be in the default alphabetical order based on their name. Previously, when moving we would always assume that all providers had a sort order, so moving an unsorted provider would put at the top. The right thing to do, instead, is to walk the list in visual order and assign an appropriate sort order, that is then flushed to gsettings. Anyway, before committing the first patch, I'll wait to change the sort order in gnome-shell too.
Review of attachment 238536 [details] [review]: Looks fine.
Review of attachment 238009 [details] [review]: See https://bugzilla.gnome.org/show_bug.cgi?id=694974#c5 - I think we should do this. Up to you to decide whether only in master or in 3.8 as well.
Attachment 238009 [details] pushed as 3969c81 - search: don't keep the last panel last in the sort order Attachment 238536 [details] pushed as 1627fdd - search: fix moving unsorted applications I'm pushing to master and gnome-3-8, because this was approved by Bastien before the branch, and because I believe it is worth to fix in 3.8, with new apps featuring a search provider.