After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 790524 - Cannot easily find scheduled changes in delayed mode
Cannot easily find scheduled changes in delayed mode
Status: RESOLVED FIXED
Product: dconf-editor
Classification: Other
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: dconf-editor maintainer(s)
dconf-editor maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-11-18 00:47 UTC by Davi
Modified: 2018-01-31 04:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patches (7.01 KB, patch)
2017-11-18 00:47 UTC, Davi
none Details | Review
Simpler, less disruptive patch (22.89 KB, patch)
2017-11-20 16:28 UTC, Davi
none Details | Review
Simpler, less disruptive patch (21.79 KB, patch)
2017-11-22 07:35 UTC, Davi
none Details | Review
Simpler, less disruptive patch (22.19 KB, patch)
2017-11-23 01:04 UTC, Davi
none Details | Review
Simpler, less disruptive patch (27.56 KB, patch)
2017-11-23 23:45 UTC, Davi
none Details | Review

Description Davi 2017-11-18 00:47:06 UTC
If I enter delayed mode and start making changes through different paths, I cannot easily keep track of what changes were scheduled.

I'll attach a patch-set that adds a menu button to the delayed mode action bar, which brings a popover with a list of the scheduled changes. Activating a row moves the main view to that path. Each row also has a "cancel this change" button. The patch-set also makes some hopeful improvements to the action bar:

- Move the cancel button to the other side, far away from the apply button, to avoid misclicking;
- Change the cancel button to have the "Cancel" label. The [x] looks like a "dismiss this warning" action;
- Change the label of the apply button from "Apply" to "Apply Now", to better convey the idea of time in the delay. Also, never change into an image.
Comment 1 Davi 2017-11-18 00:47:39 UTC
Created attachment 363956 [details] [review]
Proposed patches
Comment 2 Arnaud B. 2017-11-18 06:53:43 UTC
Wow, there’s work done here.

I’m not exactly happy with the “action bar” in any way, but as I’m not happy either with my other ideas, it’s going to be there for some times. So I’ll agree it would be great to improve it, I’ve only done the strict minimum here, and your patches are clearly a big progress.

I’ll have a thinking about that, when I’ll have time.
Comment 3 Arnaud B. 2017-11-20 14:05:07 UTC
So, I’ve tried to remember how I came to the current design of the action bar, and there’s one important thing –in my opinion– that your new design proposition is breaking: in fact, the main use of this action bar is not for delayed settings (even if it has being designed with that in mind), but for the confirmation when you’re editing a value in the properties view (in the default “always-confirm-implicit” behaviour at least).

In that (main) use case:
 * the popover feels a little overkill as there’s only one change, but that’s not a big problem;
 * the text in the button where the popover is attached doesn’t feel at its place, centered and in a button;
 * the cancel/apply buttons around the action bar have a design that contradict the idea that the action bar will disappear on its own if you change path.

About the buttons iconing/labelling:
 * I agree with you that the [×] is not clear as a “Cancel” action, but I didn’t found a better solution with the current design philosophy;
 * “Apply Now” (should be “now” IIRC the current typography rules applied in the project) feels great; but it translates in “Appliquer maintenant” in french, that is really long; checking a little, looks like it’s a more or less french-only problem, so it could be resolved by a comment for translators;
 * as long as there’s long text in the action bar (it’s impossible to know how long it could be in some translations), I’d prefer to keep the iconification of the “Apply” action button on small windows.

So. First, I’ll happily test a patch with the popover only, linked to a square button at the left (with an up arrow icon, for example), that doesn’t change the current philosophy of the action bar. For other things, there’s more design thinking needed.
Comment 4 Davi 2017-11-20 16:21:29 UTC
(In reply to Arnaud B. from comment #3)
> So, I’ve tried to remember how I came to the current design of the action
> bar, and there’s one important thing –in my opinion– that your new design
> proposition is breaking: in fact, the main use of this action bar is not for
> delayed settings (even if it has being designed with that in mind), but for
> the confirmation when you’re editing a value in the properties view (in the
> default “always-confirm-implicit” behaviour at least).
>
> In that (main) use case:
>  * the popover feels a little overkill as there’s only one change, but
> that’s not a big problem;
>  * the text in the button where the popover is attached doesn’t feel at its
> place, centered and in a button;
>  * the cancel/apply buttons around the action bar have a design that
> contradict the idea that the action bar will disappear on its own if you
> change path.

I agree that these two use cases are different and probably deserve different UIs. The popover has not much use in the "always-confirm-implicit" behavior. Since the action bar is used in both, it was "easier" to leave it there, though.
 
> About the buttons iconing/labelling:
>  * I agree with you that the [×] is not clear as a “Cancel” action, but I
> didn’t found a better solution with the current design philosophy;
>  * “Apply Now” (should be “now” IIRC the current typography rules applied in
> the project) feels great; but it translates in “Appliquer maintenant” in
> french, that is really long; checking a little, looks like it’s a more or
> less french-only problem, so it could be resolved by a comment for
> translators;
>  * as long as there’s long text in the action bar (it’s impossible to know
> how long it could be in some translations), I’d prefer to keep the
> iconification of the “Apply” action button on small windows.

In my tests the iconification saved so little space that it didn't matter (while resizing/restyling an actionable item hit me as somewhat confusing). I think the real issue is the length of the message. Maybe there are ways to reduce it? I thought of maybe merging dconf/gsettings change count. Is it really meaningful to distinguish them (even in face of the hopefully more descriptive popover)?

> So. First, I’ll happily test a patch with the popover only, linked to a
> square button at the left (with an up arrow icon, for example), that doesn’t
> change the current philosophy of the action bar. For other things, there’s
> more design thinking needed.

Ok. I'll attach a less disruptive patch :).
Comment 5 Davi 2017-11-20 16:28:30 UTC
Created attachment 364059 [details] [review]
Simpler, less disruptive patch

Simply add a [^] menu button for the popover, and only for true delayed mode.
Comment 6 Arnaud B. 2017-11-21 05:02:33 UTC
(In reply to Davi from comment #4)
> I agree that these two use cases are different and probably deserve different
> UIs.

Might be or not the way to go to have two completely different UIs. But one thing at the time, and the popover work will be useful anyway.

> The popover has not much use in the "always-confirm-implicit" behavior. Since
> the action bar is used in both, it was "easier" to leave it there, though.

It’s hard to know if it’s better to make the button disappear, or to add a “simpler” version of the popover specially designed for when not in delayed mode. As the current proposition doesn’t bug me, it could be kept that way, that was just a proposition in case you want to test other things.

> In my tests the iconification saved so little space that it didn't matter

It’s [Appliquer] in French and [Anwenden] in German (two usual tests to do), the gain feels (always) more important there.

> (while resizing/restyling an actionable item hit me as somewhat confusing).

That’s probably a matter of taste, but I do like to say people “hey, we’re not forgetting about small screens, we use a responsive design, like on the Web”.

> I think the real issue is the length of the message. Maybe there are ways to
> reduce it? I thought of maybe merging dconf/gsettings change count. Is it
> really meaningful to distinguish them (even in face of the hopefully more
> descriptive popover)?

One of the problems is the “Modifications will be applied if you change path." one, I don’t see how to reduce it. :) Merging the counter might be a way to go, but I don’t feel comfortable to forget to notify when one of the keys is going to be deleted. Yeps, there’s a head of the hydra here.

> Ok. I'll attach a less disruptive patch :).

Great, that helps.

(In reply to Davi from comment #5)
> Created attachment 364059 [details] [review] [review]
> Simpler, less disruptive patch
> 
> Simply add a [^] menu button for the popover, and only for true delayed mode.

Less popover padding, I think, as in the “Bookmarks” popover. Also, I’m not exactly happy with the default “easy to miss” look of the button, but I don’t know what to do; maybe a more explicit icon, like an eye, would be better there?

A little special thing, Key.cool_text_value_from_variant() answers “rien (nothing)” or similar in most translations of “nothing”, as requested for the historical (and current) use cases of this function. So I see on a set-to-default-value row “rien (nothing) (valeur par défaut)” (well, the last is “default value” for now, as it’s not translated), and it’s hard to parse. Not sure what to do there, and it’s too late in the morning here to think of that.

I know you like the “dim-label” class :D, but it has a quite special “meaning” (it’s with a headerbar at background), and you already add a “list-row-header” specific class added to the same item; so I’d better go in adding a coloring rule in this class (warning with :backdrop), or to use “greyed-label” as already defined.

In delayed_setting_row_compare(), you can just compare the keyN.full_name, no? and you can put an assert_not_reached() or similar instead of (or before) the “return 0;”

Some coding style details:
 * set “key_name_label.label” outside of the “if” (before it), it applies to all the cases;
 * no empty string addition in “… + (gsettings_key.is_default ? _(" (default value)") : "")”, make an “if” condition.

There may be other things, but that’s all for now.
Comment 7 Davi 2017-11-22 07:29:40 UTC
(In reply to Arnaud B. from comment #6)
> (In reply to Davi from comment #4)
> > I agree that these two use cases are different and probably deserve different
> > UIs.
> 
> Might be or not the way to go to have two completely different UIs. But one
> thing at the time, and the popover work will be useful anyway.
> 
> > The popover has not much use in the "always-confirm-implicit" behavior. Since
> > the action bar is used in both, it was "easier" to leave it there, though.
> 
> It’s hard to know if it’s better to make the button disappear, or to add a
> “simpler” version of the popover specially designed for when not in delayed
> mode. As the current proposition doesn’t bug me, it could be kept that way,
> that was just a proposition in case you want to test other things.
> 
> > In my tests the iconification saved so little space that it didn't matter
> 
> It’s [Appliquer] in French and [Anwenden] in German (two usual tests to do),
> the gain feels (always) more important there.
> 
> > (while resizing/restyling an actionable item hit me as somewhat confusing).
> 
> That’s probably a matter of taste, but I do like to say people “hey, we’re
> not forgetting about small screens, we use a responsive design, like on the
> Web”.
> 
> > I think the real issue is the length of the message. Maybe there are ways to
> > reduce it? I thought of maybe merging dconf/gsettings change count. Is it
> > really meaningful to distinguish them (even in face of the hopefully more
> > descriptive popover)?
> 
> One of the problems is the “Modifications will be applied if you change
> path." one, I don’t see how to reduce it. :) Merging the counter might be a
> way to go, but I don’t feel comfortable to forget to notify when one of the
> keys is going to be deleted. Yeps, there’s a head of the hydra here.
> 
> > Ok. I'll attach a less disruptive patch :).
> 
> Great, that helps.
> 
> (In reply to Davi from comment #5)
> > Created attachment 364059 [details] [review] [review] [review]
> > Simpler, less disruptive patch
> > 
> > Simply add a [^] menu button for the popover, and only for true delayed mode.
> 
> Less popover padding, I think, as in the “Bookmarks” popover. 

Ok.

> Also, I’m not
> exactly happy with the default “easy to miss” look of the button, but I
> don’t know what to do; maybe a more explicit icon, like an eye, would be
> better there?

I'm not very comfortable with picking an icon. In my opinion, icons can only replace text when they are very, very standard. The current arrow icon is unmistakably an indicator for a menu/popover, so I'm good with it. The content of the popover can be learned from context, or just by clicking it. I imagine nobody will think that clicking the arrow button might have some unexpected consequence.

> A little special thing, Key.cool_text_value_from_variant() answers “rien
> (nothing)” or similar in most translations of “nothing”, as requested for
> the historical (and current) use cases of this function. So I see on a
> set-to-default-value row “rien (nothing) (valeur par défaut)” (well, the
> last is “default value” for now, as it’s not translated), and it’s hard to
> parse. Not sure what to do there, and it’s too late in the morning here to
> think of that.

The design for the rows is sketchy, so there are many improvements to be made. My intention was to give an overview of what are the scheduled changes, but maybe that level of detail is not needed, since clicking the row brings the full properties view.

> I know you like the “dim-label” class :D, but it has a quite special
> “meaning” (it’s with a headerbar at background), and you already add a
> “list-row-header” specific class added to the same item; so I’d better go in
> adding a coloring rule in this class (warning with :backdrop), or to use
> “greyed-label” as already defined.

I think dim-label is just a half-opaque label. I don't know what other meaning it has, but it is used in many situations inside GNOME apps, for things that should call less attention, like listbox row headers and descriptions. The greyed-label is not the same thing, as it does not reduce contrast. I did some research, but could not figure out what the :backdrop means. Do you have any pointers no that?

> In delayed_setting_row_compare(), you can just compare the keyN.full_name,
> no? 

It is not the same thing. Think of these changed keys, sorted by full_name:

/org/gnome/shell/always-show-log-out
/org/gnome/shell/app-switcher/current-workspace-only
/org/gnome/shell/command-history

Now, sorting them by parent first, then name, gives:

/org/gnome/shell/always-show-log-out
/org/gnome/shell/command-history
/org/gnome/shell/app-switcher/current-workspace-only

I think it is better to group keys by parent folder, since they are more likely to be related.

> and you can put an assert_not_reached() or similar instead of (or
> before) the “return 0;”

I guess, since there must be no duplicates in the list, but this function represents an order relation, which should be well-defined for any two items. I'm not sure the ListStore will not compare an item to itself in the sorting algorithm (probably not, though).

> Some coding style details:
>  * set “key_name_label.label” outside of the “if” (before it), it applies to
> all the cases;

Ok.

>  * no empty string addition in “… + (gsettings_key.is_default ? _(" (default
> value)") : "")”, make an “if” condition.

Ok.

> There may be other things, but that’s all for now.
Comment 8 Davi 2017-11-22 07:32:06 UTC
Oops, forgot to strip non-replied-to parts from comment above, sorry.
Comment 9 Davi 2017-11-22 07:35:34 UTC
Created attachment 364169 [details] [review]
Simpler, less disruptive patch

- Coding style improvements
- Apply same padding as other popovers
- Use RegistryPlaceholder
Comment 10 Arnaud B. 2017-11-22 14:29:25 UTC
(In reply to Davi from comment #7)
> I'm not very comfortable with picking an icon. In my opinion, icons can only
> replace text when they are very, very standard. The current arrow icon is
> unmistakably an indicator for a menu/popover, so I'm good with it. The
> content of the popover can be learned from context, or just by clicking it.

Instead of marking the button “flat”, mark it “circular”? Not saying it’s the perfect solution, I just find a little strange to have the text of the action bar beginning “somewhere at the right of the (small, tiny) arrow”, without understandable placement. So I’m looking either for a bigger icon, or for a real button indication.

> > A little special thing, Key.cool_text_value_from_variant() answers “rien
> > (nothing)” or similar in most translations of “nothing”, as requested for
> > the historical (and current) use cases of this function. So I see on a
> > set-to-default-value row “rien (nothing) (valeur par défaut)” (well, the
> > last is “default value” for now, as it’s not translated), and it’s hard to
> > parse. Not sure what to do there, and it’s too late in the morning here to
> > think of that.
> 
> The design for the rows is sketchy, so there are many improvements to be
> made. My intention was to give an overview of what are the scheduled
> changes, but maybe that level of detail is not needed, since clicking the
> row brings the full properties view.

Well, I think the level of detail given is great, and the look is correct for a first try. There’s just a weirdness in the presentation due to a quite specific functionality of Key.cool_text_value_from_variant() only visible in non-english locales (that respect the translators comments).

> I think dim-label is just a half-opaque label. I don't know what other
> meaning it has, but it is used in many situations inside GNOME apps, for
> things that should call less attention, like listbox row headers and
> descriptions.

The “dim-label” class is good enough here, but it needs to be carefully used: it’s playing on opacity, so depending on the theme used, on the background and some font color, the text where it applies could easily become unreadable.

> The greyed-label is not the same thing, as it does not reduce
> contrast.

The “greyed-label” class is defined with colors from the current theme. There are bugs when themes do not implement these colors, but that’s clearly a theme problem then. The result is clearly more “predictable”, it’s a label using color that are always readable (by definition, even if it’s not enforced anywhere) on usual “white” and “grey” backgrounds or equivalent on other themes than Adwaita.

> I did some research, but could not figure out what the :backdrop
> means. Do you have any pointers no that?

Oh, sorry, I thought you knew about that. The “:backdrop” pseudo-class is applied when a window is not the one “on top” of the others in the WM. In Adwaita, it gives less contrast; that’s why using “dim-label” is usually a bad idea, in :backdrop the text is already less readable because of less contrasty with the background, and if “dim-label” is applied and the background is grey, it finishes unreadable.

> It is not the same thing. Think of these changed keys, sorted by full_name:
> 
> /org/gnome/shell/always-show-log-out
> /org/gnome/shell/app-switcher/current-workspace-only
> /org/gnome/shell/command-history
> 
> Now, sorting them by parent first, then name, gives:

Oh, yes, you’re right. I was focusing on “why not avoiding a .parent call here”, and forgot about this case.

> > and you can put an assert_not_reached() or similar instead of (or
> > before) the “return 0;”
> 
> I guess, since there must be no duplicates in the list, but this function
> represents an order relation, which should be well-defined for any two
> items. I'm not sure the ListStore will not compare an item to itself in the
> sorting algorithm (probably not, though).

As you want.

Some things:
 * no keyboard shortcut in the popover, would be a cool improvement;
 * <Ctrl>Delete and <Ctrl>Return (and maybe others) act on the selection in browse view, even if the popover is displayed;
 * maybe the font used for the key name should be a little bolder/darker/different than the one for the diffs.
Comment 11 Davi 2017-11-23 01:00:55 UTC
(In reply to Arnaud B. from comment #10)
> Instead of marking the button “flat”, mark it “circular”? Not saying it’s
> the perfect solution, I just find a little strange to have the text of the
> action bar beginning “somewhere at the right of the (small, tiny) arrow”,
> without understandable placement. So I’m looking either for a bigger icon,
> or for a real button indication.

Maybe we could go with "view-more-symbolic"? I think it is usually associated with revealers, but it could work with a popover too. For now I removed the "flat" style to separate it somewhat from the following message.
 
> Well, I think the level of detail given is great, and the look is correct
> for a first try. There’s just a weirdness in the presentation due to a quite
> specific functionality of Key.cool_text_value_from_variant() only visible in
> non-english locales (that respect the translators comments).

I'm not sure how to handle this. Maybe use a dim-label for the "(default value)", so it becomes more clearly an indicator and not part of the value? Or put some spacing between it and the value, or remove it entirely, since it is a technicality?

> The “dim-label” class is good enough here, but it needs to be carefully
> used: it’s playing on opacity, so depending on the theme used, on the
> background and some font color, the text where it applies could easily
> become unreadable.
> 
> [...]

If I understand correctly, I should not use dim-label when the content is on a "grey" background, is that it?

> Some things:
>  * no keyboard shortcut in the popover, would be a cool improvement;

Do you mean to add a new shortcut to toggle the popover?

>  * <Ctrl>Delete and <Ctrl>Return (and maybe others) act on the selection in
> browse view, even if the popover is displayed;

Should it guard against those actions or dismiss the popover and put focus back to the browser view?

>  * maybe the font used for the key name should be a little
> bolder/darker/different than the one for the diffs.

I've added some more padding and used a different alignment. If you think that is not enough, we might go with bold key name, but it gave me a bit of "cramped" feeling.
Comment 12 Davi 2017-11-23 01:04:32 UTC
Created attachment 364247 [details] [review]
Simpler, less disruptive patch

- Improve spacing and recognition of the key name in each row
- Remove "flat" from menu icon
Comment 13 Arnaud B. 2017-11-23 05:14:09 UTC
(In reply to Davi from comment #11)
> Maybe we could go with "view-more-symbolic"? I think it is usually
> associated with revealers, but it could work with a popover too. For now I
> removed the "flat" style to separate it somewhat from the following message.

Yeps, I would prefer “view-more-symbolic”, flat or not, circular or not.

> I'm not sure how to handle this. Maybe use a dim-label for the "(default
> value)", so it becomes more clearly an indicator and not part of the value?

Might be an idea, yes. Needs testing. ;)

> Or put some spacing between it and the value, or remove it entirely, since
> it is a technicality?

I think it’s important to display this information in a way or another (being default is more important than the value itself).

> If I understand correctly, I should not use dim-label when the content is on
> a "grey" background, is that it?

As a general rule, you should not use a CSS class wherever it’s not designed to be used. And basically, “dim-label” is for “color:black;” text to become “grey as a subtitle” on a headerbar. If your text could inherit another color than “color:black; opacity:100%;” then using “dim-label” will result in weird things somewhere/sometimes.

There’s no CSS rule for theme designers to target specifically headers of a ListBox (I think), that’s why it’s not quite important in that case (neither in the keys-list); but it’s sometimes easier to just make a CSS class that we can design for our use cases, than to reuse one that could lead to unreadable text if not used carefully.

> Do you mean to add a new shortcut to toggle the popover?

I think it would be great. And I was more thinking of a new shortcut for cancelling pending changes (I like to offer a keyboard way to do things that could be done with mouse; for accessibility).

I was thinking of another functionality for this popover, but it might be added later/discussed elsewhere. Maybe that instead of allowing direct cancelling of a pending change by a button in the row, it would be better to allow selecting one or more rows (checkbox? or just use the ListBox selection allowing more than one item to be selected?) and add two “Cancel” and “Apply Now” button in the popover directly; so you can dismiss/validate part of the pending changes.

> >  * <Ctrl>Delete and <Ctrl>Return (and maybe others) act on the selection in
> > browse view, even if the popover is displayed;
> 
> Should it guard against those actions or dismiss the popover and put focus
> back to the browser view?

Guard against those actions, and if possible all others.

> I've added some more padding and used a different alignment. If you think
> that is not enough, we might go with bold key name, but it gave me a bit of
> "cramped" feeling.

Rows are quite high, but that’s ok for me.

Other thing: I don’t understand the use of a GtkFrame in the popover, it probably could be removed.
Comment 14 Davi 2017-11-23 23:43:54 UTC
(In reply to Arnaud B. from comment #13)
> Yeps, I would prefer “view-more-symbolic”, flat or not, circular or not.

Done.
 
> > I'm not sure how to handle this. Maybe use a dim-label for the "(default
> > value)", so it becomes more clearly an indicator and not part of the value?
> 
> Might be an idea, yes. Needs testing. ;)
> 
> > Or put some spacing between it and the value, or remove it entirely, since
> > it is a technicality?
> 
> I think it’s important to display this information in a way or another
> (being default is more important than the value itself).

I've made it bold. Please, test it.
 
> As a general rule, you should not use a CSS class wherever it’s not designed
> to be used. And basically, “dim-label” is for “color:black;” text to become
> “grey as a subtitle” on a headerbar. If your text could inherit another
> color than “color:black; opacity:100%;” then using “dim-label” will result
> in weird things somewhere/sometimes.
> 
> There’s no CSS rule for theme designers to target specifically headers of a
> ListBox (I think), that’s why it’s not quite important in that case (neither
> in the keys-list); but it’s sometimes easier to just make a CSS class that
> we can design for our use cases, than to reuse one that could lead to
> unreadable text if not used carefully.

I think if a theme expects "dim-label" to have any other meaning than "dim label", it should be a theme bug. It is certainly used in many more places than the headerbar, so I think that any theme designer will be careful with that class (similar to "suggested-action", for example). On the other hand, if we define our own CSS class, it means theme designers will have one more app-specific class to track (but only after some user detects it and reports it to Dconf Editor first, ofc :).

> > Do you mean to add a new shortcut to toggle the popover?
> 
> I think it would be great. And I was more thinking of a new shortcut for
> cancelling pending changes (I like to offer a keyboard way to do things that
> could be done with mouse; for accessibility).

Ok. I can't think of any meaningful key combination right now, but I've added some random one (Ctrl+U) for testing. Delete will cancel the focused change.

> I was thinking of another functionality for this popover, but it might be
> added later/discussed elsewhere. Maybe that instead of allowing direct
> cancelling of a pending change by a button in the row, it would be better to
> allow selecting one or more rows (checkbox? or just use the ListBox
> selection allowing more than one item to be selected?) and add two “Cancel”
> and “Apply Now” button in the popover directly; so you can dismiss/validate
> part of the pending changes.

Seems like a good idea to me. We could test that later, after the basic functionality matures.
 
> Guard against those actions, and if possible all others.

Ok. 

Now I think the way key shortcuts are handled is starting to show its limitations. I could add some "ifs" in the DConfWindow.on_key_pressed_event, but that will not scale (and it is quite complicated already). Is there any reason why every shortcut is handled in the top-level window, bypassing the focus/grab machinery? Would it be of interest to spread these actions each to its appropriate context, or even redo them as GActions and employ the "enabled" property and other nice facilities?

Take a look at the "Delete" handling in ModificationsRevealer. What do you think?

> Other thing: I don’t understand the use of a GtkFrame in the popover, it
> probably could be removed.

Yes, just learned to use <property name="shadow-type">etched-in</property>.
Comment 15 Davi 2017-11-23 23:45:39 UTC
Created attachment 364300 [details] [review]
Simpler, less disruptive patch

- Keyboard shortcuts
- Redo "Default value"/"Erased key" indicator
- Other small improvements
Comment 16 Arnaud B. 2017-11-24 01:09:12 UTC
(In reply to Davi from comment #14)
> I've made it bold. Please, test it.

It’s great when the key is waiting for a reset, so the boldy “Default value” is the important information: you’re reseting the key. But it’s bad when “Default value” is the current value of the key, here I fail to parse the row on first sight.

In fact, more I’m thinking of that, more I want not to display the information about the current value of the row. Apart the key name, there are two important informations:
 * what kind of modification is done (“Default → Custom”/“Custom → New custom”/“Custom → Default”/“DConf key → Edited”/“DConf key → Erased”);
 * the new value.
What the current value is doesn’t look so important.

> I think if a theme expects "dim-label" to have any other meaning than "dim
> label", it should be a theme bug. It is certainly used in many more places
> than the headerbar, so I think that any theme designer will be careful with
> that class (similar to "suggested-action", for example).

The problem is not that “dim-label” might have another meaning that an opacity change. The problem is that as “dim-label” is playing with opacity instead of plain color, if you apply it in a context that the theme designer didn’t think of, you can finish with a bad result.

The “suggested-action” class at the opposite won’t be applied on many widgets/in many contexts, so it’s easy for designers to check it’s working as expected everywhere it will be used.

> if we define our own CSS class, it means theme designers will have one more
> app-specific class to track (but only after some user detects it and reports
> it to Dconf Editor first, ofc :).

Not if it’s defined with named colors (see “greyed-label”), they have no work to do.

> Ok. I can't think of any meaningful key combination right now, but I've
> added some random one (Ctrl+U) for testing. Delete will cancel the focused
> change.

<Ctrl>I like “Informations”? No preferences for now, might need a GNOME designer input.

> Now I think the way key shortcuts are handled is starting to show its
> limitations. I could add some "ifs" in the DConfWindow.on_key_pressed_event,
> but that will not scale (and it is quite complicated already).

Agree, but that’s a different work.

> Is there any reason why every shortcut is handled in the top-level window,
> bypassing the  focus/grab machinery? Would it be of interest to spread these
> actions each to its appropriate context,

Keyboard shortcuts are hard to get right, notably when playing with a searchbar, some popovers, some shortcuts that have different meanings depending on the context (“Menu”), etc. I’m not sure it would be better to dispatch all these behaviours on various places in the code (if that’s what you’re talking about), but I don’t even know how other applications are doing. I have in mind to first split the current code in various functions, depending on the UI context; and then see if that’s enough to become again manageable. Needs work, yes. But don’t stress for now for having a shortcut defined elsewhere in the code, that’s the case sometimes.

> or even redo them as GActions and employ the "enabled" property and other
> nice facilities? 

That would probably be fun and clearer. I plan to use more and more GAction, but first I’d like to know how not to expose them/some on dbus. Just need some research, probably.
Comment 17 Arnaud B. 2018-01-31 04:05:36 UTC
I pushed an adaptation of your patch to current code. I plan to rework the look a little, but I’m a little short on the timing, as we enter in beta freeze on next Monday; I wanted to assure that at least the basic function would be in the 3.28, that will be released in March. The dismissed part of your initial request will be handled in “Rework RegistryInfo[1],” one day, certainly. ^^ Thanks for the bug report and your work on that!

[1] https://bugzilla.gnome.org/show_bug.cgi?id=791777