I was experimenting with this and saw that sorting changes do not apply immediately, but show an info bar with a reload button. It seems to me that it could be applied immediately, since it can be easily undone. Am I missing something?

(In reply to Davi from comment #1)
> I was experimenting with this and saw that sorting changes do not apply
> immediately, but show an info bar with a reload button. It seems to me that
> it could be applied immediately, since it can be easily undone. Am I missing
> something?

Never do that. :·)

From a user point of view, that’s not a good news to see the list you’re browsing being reordered. Depending on how it’s done, you’re forced to read again the list, or you could miss an item (and so you’re forced to read again the list). And think of this malware script that change the order multiple time per second…

There’s of course this special case of the “/ca/desrt/dconf-editor/sort-case-sensitive” key, where 1) changing this key won’t change it’s index, and 2) if the user toggles something there, she or he knows what should happen; but let’s not do an exception there, as it demonstrates to user the application behaviour.

From a development perspective, that’s also a bad idea to reorder a list without user consent. Because, for example, what happens if the list is reordered when there’s a visible right-click popover? Ensuring there’s user input allows ensuring the window is in a predictable state, with nothing selection-related visible.

So, the “sort-folder-first” option should depend of such an info bar (and I think I’ve written the message so it could use the same).

(In reply to Arnaud B. from comment #2)

> Never do that. :·)

It's a dogma, then? :)

> From a user point of view, that’s not a good news to see the list you’re
> browsing being reordered. Depending on how it’s done, you’re forced to read
> again the list, or you could miss an item (and so you’re forced to read
> again the list).

Unless you are the one who issued the order? Then that would be the exact news you would be waiting for, right? And you can undo it almost effortlessly, bringing the order to exactly the same it was before.

> And think of this malware script that change the order
> multiple time per second…

A malware that can change your settings? It could do a lot worse.
 
> There’s of course this special case of the
> “/ca/desrt/dconf-editor/sort-case-sensitive” key, where 1) changing this key
> won’t change it’s index, and 2) if the user toggles something there, she or
> he knows what should happen; but let’s not do an exception there, as it
> demonstrates to user the application behaviour.


> From a development perspective, that’s also a bad idea to reorder a list
> without user consent. 

Again, how could that be without user consent? The only way to change a UI setting should be from user action. 

> Because, for example, what happens if the list is
> reordered when there’s a visible right-click popover? Ensuring there’s user
> input allows ensuring the window is in a predictable state, with nothing
> selection-related visible.

It seems to me that you are expecting a situation where the setting is changed by something that is not a user action. I'm assuming that should never happen ever this setting.

> So, the “sort-folder-first” option should depend of such an info bar (and I
> think I’ve written the message so it could use the same).

If such action would be exposed in i.e. the hamburger menu, then for this case the info bar would not be shown?

(In reply to Davi from comment #3)
> (In reply to Arnaud B. from comment #2)
> > And think of this malware script that change the order
> > multiple time per second…
> 
> A malware that can change your settings? It could do a lot worse.

Surely. In fact, the related “dogma” is more or less that an application should never ever allow something to happen that could let the user not knowing what to do.

> > From a development perspective, that’s also a bad idea to reorder a list
> > without user consent. 
> 
> Again, how could that be without user consent? The only way to change a UI
> setting should be from user action. 

#!/bin/bash

echo "I'm a diabolic malware"
while true; do
  gsettings set ca.desrt.dconf-editor.Settings sort-case-sensitive true
  sleep 0.2
  gsettings set ca.desrt.dconf-editor.Settings sort-case-sensitive false
  sleep 0.2
done

> If such action would be exposed in i.e. the hamburger menu, then for this
> case the info bar would not be shown?

Exactly. I consider that editing “ca.desrt.dconf-editor.Settings” keys is, like for other schemas, dealing with internals of the related application. The dconf-editor window should react to a gsettings change as an application that has seen an internal option change. It doesn’t know that when editing keys in “/ca/desrt/dconf-editor/”, it’s (usually) its own options that are edited.

To answer the next questions, I’m not sure how important it is to expose sort options in the UI, let’s say I don’t mind if it looks great and don’t take place in the headerbar. :·) And note that if that happens, the UI could expose less sort options than the gsettings key allow.

(In reply to Arnaud B. from comment #4)
> (In reply to Davi from comment #3)
> > (In reply to Arnaud B. from comment #2)
> > > And think of this malware script that change the order
> > > multiple time per second…
> > 
> > A malware that can change your settings? It could do a lot worse.
> 
> Surely. In fact, the related “dogma” is more or less that an application
> should never ever allow something to happen that could let the user not
> knowing what to do.
> 
> > > From a development perspective, that’s also a bad idea to reorder a list
> > > without user consent. 
> > 
> > Again, how could that be without user consent? The only way to change a UI
> > setting should be from user action. 
> 
> #!/bin/bash
> 
> echo "I'm a diabolic malware"
> while true; do
>   gsettings set ca.desrt.dconf-editor.Settings sort-case-sensitive true
>   sleep 0.2
>   gsettings set ca.desrt.dconf-editor.Settings sort-case-sensitive false
>   sleep 0.2
> done

Now you've got me even more confused. We cannot defend against a malware with that kind of power. It could simply reset or scramble all our settings. It's like wearing a hood under a hurricane: your roof is flying, but your hair is in place.

Also, we are showing the info bar when the action is triggered **inside the app itself**. That seems like burdening (a little, I concede) the user because it might be a malware, even when we can see it is not!

> > If such action would be exposed in i.e. the hamburger menu, then for this
> > case the info bar would not be shown?
> 
> Exactly. I consider that editing “ca.desrt.dconf-editor.Settings” keys is,
> like for other schemas, dealing with internals of the related application.
> The dconf-editor window should react to a gsettings change as an application
> that has seen an internal option change. It doesn’t know that when editing
> keys in “/ca/desrt/dconf-editor/”, it’s (usually) its own options that are
> edited.

I've never seen an app that deals differently with out-of-band settings modifications. They just either apply the setting immediately or apply it on next refresh/reload/reopen/etc.. It seems to me like a big effort to maintain a complex mechanic for a corner case. And in my opinion, we should not fight malware on that level, where the attack surface is the largest.

> To answer the next questions, I’m not sure how important it is to expose
> sort options in the UI, let’s say I don’t mind if it looks great and don’t
> take place in the headerbar. :·) And note that if that happens, the UI could
> expose less sort options than the gsettings key allow.

In my opinion, sorting folders before keys is very important. It seems very conter-intuitive if the user must discover the dconf-editor settings schema/location to do that.

(In reply to Davi from comment #5)
> Now you've got me even more confused. We cannot defend against a malware
> with that kind of power. It could simply reset or scramble all our settings.
> It's like wearing a hood under a hurricane: your roof is flying, but your
> hair is in place.

I like to handle all corner-cases correctly, and it is probably why I’m happy to play with settings API like gsettings/dconf. Because there are many corner-cases, but people always feel that their use of the settings API is the correct one. It’s so easy to only deal with your application’s data…

> I've never seen an app that deals differently with out-of-band settings
> modifications. They just either apply the setting immediately or apply it on
> next refresh/reload/reopen/etc.. It seems to me like a big effort to
> maintain a complex mechanic for a corner case.
<
> Also, we are showing the info bar when the action is triggered **inside the
> app itself**. That seems like burdening (a little, I concede) the user
> because it might be a malware, even when we can see it is not!

On other views than “/ca/desrt/dconf-editor/” (that is a special case):
 * Applying a immediate refresh of the list on settings change is a bad behaviour (don’t forget the “there’s a right click popover visible” case).
 * There’s no refresh/reload button because there’s no need for it, as the application reacts to the gsettings change. Else, one would be needed.

On “/ca/desrt/dconf-editor/”: there’s no way to handle that on that special view, when triggering a specific key, something that happens elsewhere won’t happen because were’re here. That’s where an overkill “complex mechanic” could be, and it isn’t there. Feel free to add it, but if you think a setting is important, it would be better to add something in the application UI.

> In my opinion, sorting folders before keys is very important. It seems very
> conter-intuitive if the user must discover the dconf-editor settings
> schema/location to do that.

Yeps, “/ca/desrt/dconf-editor/” keys are not designed to be edited there by anyone like if it was a configuration panel, but by people who agree to deal with dconf-editor internals; there are internals options, like every other schemas. Visible options should be displayed in the UI, like for every other app.

Created attachment 363419 [details] [review]
Add setting to sort folders first or last

First tentative patch. I tried to imitate the inner workings of the sort-case-sensitive setting. If this is the right direction, I can refactor/clean up the code in a next iteration.

(No computer)

I’d have probably tried to save the state of each sort option to not query both each time one changes, but I’ll admit it’s highly unimportant.

A little thing that I’d prefer: a mixed/first/last enum could be used for other sort options than this one, so please name it a more general way.

And it’s better to add braces to:
if
{
  if
    foo
  else
    bar
}
else if
{
  ...
}
else
  one_ligne_func ();

Thanks!

(In reply to Arnaud B. from comment #8)
> (No computer)
> 
> I’d have probably tried to save the state of each sort option to not query
> both each time one changes, but I’ll admit it’s highly unimportant.

I'll try to refactor it a bit.

> A little thing that I’d prefer: a mixed/first/last enum could be used for
> other sort options than this one, so please name it a more general way.

On top of my head I cannot find a good not-too-generic name. I though "PreSorting", maybe. Any ideas?

> And it’s better to add braces to:
> if
> {
>   if
>     foo
>   else
>     bar
> }
> else if
> {
>   ...
> }
> else
>   one_ligne_func ();

Ok.

Created attachment 363517 [details] [review]
Add setting to sort folders first or last

Refactor sorting options, encapsulate into a SortingOptions class. Fix re-querying GSettings all the time.

Created attachment 363518 [details] [review]
Fix showing refresh view warning when showing a key

I think?

Created attachment 363532 [details] [review]
Add setting to sort folders first or last

Fix an issue where the refresh warning would not trigger. I had to disable the require_sorting flag for now (i.e. always true), because I could not figure an easy way to check if sorting is needed using both criteria.

Created attachment 363533 [details] [review]
Fix showing refresh view warning when showing a key

Correct my stupid mistake in the previous iteration (and use an already existing method)

(With a computer)

> On top of my head I cannot find a good not-too-generic name. I though
> "PreSorting", maybe. Any ideas?

Even “SortOptions” would be good enough for me.

> I had to disable the require_sorting flag for now (i.e. always true),
> because I could not figure an easy way to check if sorting is needed
> using both criteria.

It would be better to keep such a flag. Having an application that asks me to reload the view when there’s nothing to reload would feel bad. You could just duplicate the ListStore, sort it with new options and compare one by one to the old one; it doesn’t need to be “fast”.

Other things:
 * in the gschema file, don’t write “Defaults to “mixed”.” in the description; so we could change the default without having to edit the comment;
 * more generally, I don’t understand why in the last iteration, you prefered a SortingOptions object. As long as it does the job, I’m not really ‘regardant’ on how it’s done. There’s probably a reason I don’t think about; but it looks ‘fatter’ than the previous static method with booleans.

(In reply to Arnaud B. from comment #14)
> Even “SortOptions” would be good enough for me.

It seems too generic to me. The same name could apply to any sorting-related setting, while not conveying information about its values. I went with "MergeType" for now: merge folders first, merge folders last or merge folders mixed. What do you think? Could that be postponed for when the need arises?

> It would be better to keep such a flag. Having an application that asks me
> to reload the view when there’s nothing to reload would feel bad. You could
> just duplicate the ListStore, sort it with new options and compare one by
> one to the old one; it doesn’t need to be “fast”.

In this iteration I managed to simplify this a bit. Now it does not saves any flags or old sorting parameters. Whenever there is a change in some sorting option, need_sorting simply checks if the current directory is already sorted with respect to the new options. The warning shows if and only if re-sorting would move some entry.
 
> Other things:
>  * in the gschema file, don’t write “Defaults to “mixed”.” in the
> description; so we could change the default without having to edit the
> comment;

Fixed.

>  * more generally, I don’t understand why in the last iteration, you
> prefered a SortingOptions object. As long as it does the job, I’m not really
> ‘regardant’ on how it’s done. There’s probably a reason I don’t think about;
> but it looks ‘fatter’ than the previous static method with booleans.

It is mostly to keep track of changes in the sorting-related settings and moving together the options to the sorting function (sometimes I'm kinda OO-freak too). 

I've been trying to refactor these GCompareDataFunc but I'm getting these warnings (because I'm return'ing them):


[1/12] Compiling Vala source ../../../../../../../../Pessoal/dconf-editor/editor/bookmarks.vala ../../../../../../../../Pessoal/dconf-editor/editor/dconf-editor.vala ../../../../../../../../Pessoal/dconf-editor/editor/dconf-model.vala ../../../../../../../../Pessoal/dconf-editor/editor/dconf-view.vala ../../../../../../../../Pessoal/dconf-editor/editor/dconf-window.vala ../../../../../../../../Pessoal/dconf-editor/editor/key-list-box-row.vala ../../../../../../../../Pessoal/dconf-editor/editor/modifications-revealer.vala ../../../../../../../../Pessoal/dconf-editor/editor/pathbar.vala ../../../../../../../../Pessoal/dconf-editor/editor/registry-info.vala ../../../../../../../../Pessoal/dconf-editor/editor/registry-view.vala.
../../../../../../../../Pessoal/dconf-editor/editor/dconf-model.vala:839.24-839.51: warning: copying delegates is not supported
../../../../../../../../Pessoal/dconf-editor/editor/dconf-model.vala:841.24-841.53: warning: copying delegates is not supported
../../../../../../../../Pessoal/dconf-editor/editor/dconf-model.vala:846.24-846.50: warning: copying delegates is not supported
../../../../../../../../Pessoal/dconf-editor/editor/dconf-model.vala:848.24-848.52: warning: copying delegates is not supported
../../../../../../../../Pessoal/dconf-editor/editor/dconf-model.vala:853.24-853.51: warning: copying delegates is not supported
../../../../../../../../Pessoal/dconf-editor/editor/dconf-model.vala:855.24-855.53: warning: copying delegates is not supported
Compilation succeeded - 6 warning(s)


Despite these, everything works just fine. Am I missing something?

Comment on attachment 363532 [details] [review]
Add setting to sort folders first or last

Refactored the checks for the need to reload the key list.

Created attachment 363755 [details] [review]
Add setting to sort folders first or last

Refactored the checks for the need to reload the key list.

(In reply to Davi from comment #15)
> It seems too generic to me. The same name could apply to any sorting-related
> setting, while not conveying information about its values. I went with
> "MergeType" for now: merge folders first, merge folders last or merge
> folders mixed. What do you think? Could that be postponed for when the need
> arises?

In fact, I thought it could be made generic to any sorting-related setting. :) But you’re right, as long as the names “mixed”, “last” and “first” do not change (as it’s saved in the backend as a string), and the enum do not change order, renaming the enum could be done when needed. So go with that, it’s good for me.

> In this iteration I managed to simplify this a bit. Now it does not saves
> any flags or old sorting parameters. Whenever there is a change in some
> sorting option, need_sorting simply checks if the current directory is
> already sorted with respect to the new options. The warning shows if and
> only if re-sorting would move some entry.

I haven’t gone deep in the understanding of why the code is like that, but the patch looks great. It generates warnings, but it looks great. ^^

> I've been trying to refactor these GCompareDataFunc but I'm getting these
> warnings (because I'm return'ing them):
> 
> [...]
> 
> Despite these, everything works just fine. Am I missing something?

I’ve played with Vala since some times now, but I don’t have a clear understanding on all the trickeries it has. I remember there are things in the Vala tutorial regarding delegates[1] and closures[2]; you might else want to go on GNOME IRC to ask on #vala. I think it’s important to correct these warnings.

Also, there are two warnings at the application start:
  ** (dconf-editor:2438): CRITICAL **: 05:47:54.704: sorting_options_is_key_model_sorted: assertion 'model != NULL' failed
I’m sure you’ll find something for that. ;)

[1] https://wiki.gnome.org/Projects/Vala/Tutorial#Delegates
[2] https://wiki.gnome.org/Projects/Vala/Tutorial#Anonymous_Methods_.2BAC8_Closures

(In reply to Arnaud B. from comment #18)
> I haven’t gone deep in the understanding of why the code is like that, but
> the patch looks great. It generates warnings, but it looks great. ^^

Fixed the warnings. I went for a more "conventional" solution, i.e. explicit classes and method calls instead of delegates/lambdas. One might say it cluttered the code a bit, but ceased the compiler warnings.
 
> Also, there are two warnings at the application start:
>   ** (dconf-editor:2438): CRITICAL **: 05:47:54.704:
> sorting_options_is_key_model_sorted: assertion 'model != NULL' failed
> I’m sure you’ll find something for that. ;)

Oops. Fixed.

Created attachment 363831 [details] [review]
Add setting to sort folders first or last

- Stop copying delegates
- Fix null pointer

I've just found a bug on master where the reload banner shows if the sorting settings change, even while showing key details. Clicking the reload button prints the warning:

** (dconf-editor:7235): CRITICAL **: registry_view_reload: assertion '!is_not_browsing_view ()' failed

I think the solution is simply not to show the banner in this situation, since the view is sort-independent. A simple check before showing the banner seems to fix it.

(In reply to Davi from comment #20)
> Created attachment 363831 [details] [review] [review]
> Add setting to sort folders first or last
> 
> - Stop copying delegates
> - Fix null pointer

Double line on editor/dconf-model.vala:101, “new blank line at EOF” says `git am` also for editor/dconf-model.vala:903. The SettingComparator interface/objects is a little overkill, but it’s good enough for me to push. Do you have other things you want to change?

(In reply to Davi from comment #21)
> I've just found a bug on master where the reload banner shows if the sorting
> settings change, even while showing key details. Clicking the reload button
> prints the warning:
> 
> ** (dconf-editor:7235): CRITICAL **: registry_view_reload: assertion
> '!is_not_browsing_view ()' failed

Oops.

> I think the solution is simply not to show the banner in this situation,
> since the view is sort-independent. A simple check before showing the banner
> seems to fix it.

Completely agree. Are you working on a patch?

(In reply to Arnaud B. from comment #22)
> Double line on editor/dconf-model.vala:101, “new blank line at EOF” says
> `git am` also for editor/dconf-model.vala:903. The SettingComparator
> interface/objects is a little overkill, but it’s good enough for me to push.

Fixed.

> Do you have other things you want to change?

No, I'm good.
 
> Completely agree. Are you working on a patch?

Yes. Attaching now. It is merged on top of the other patch, is that ok?

Created attachment 363844 [details] [review]
Add setting to sort folders first or last

Fix excessive new lines

Created attachment 363845 [details] [review]
Fix showing reload banner in key properties view

Comment on attachment 363844 [details] [review]
Add setting to sort folders first or last

Committed, thanks. I pushed also a patch for two small subsequent style fixes[1].

[1] https://git.gnome.org/browse/dconf-editor/commit/?id=5ff0e5b1423d137286bf9d2629c35d63b6823f85

Comment on attachment 363845 [details] [review]
Fix showing reload banner in key properties view

Looks good, thanks.

I think all here has been done, so closing. Many thanks for your work. :)