GNOME Bugzilla – Bug 790528
Proposal for improving layout of properties view
Last modified: 2017-12-03 15:59:15 UTC
The current layout, using width-chars/max-width-chars is not very responsive. The proposed patch uses the recently added ListBox row wrapper to limit the width of rows in a more flexible way. The patch also change the editor for text values (KeyEditorChildDefault) to use a TextView instead of an Entry for more space. There is some bug that prevents the TextView from gaining focus from clicks, so that needs further investigation. This is not in the patch, but would there be a problem in moving away from a ListBox and using a normal Grid for the properties view?
Created attachment 363961 [details] [review] Preliminary patch
(In reply to Davi from comment #0) > The current layout, using width-chars/max-width-chars is not very > responsive. The proposed patch uses the recently added ListBox row wrapper > to limit the width of rows in a more flexible way. Looks great! Let’s go that way. > The patch also change the editor for text values (KeyEditorChildDefault) to > use a TextView instead of an Entry for more space. There is some bug that > prevents the TextView from gaining focus from clicks, so that needs further > investigation. Please, in another patch. I’m not convinced exactly for that one, the TextView is sometimes useful, sometimes highly overkill. It might be interesting only for some of the types (the one that contain arrays? or arrays of things that include a string? to be decided). > This is not in the patch, but would there be a problem in moving away from a > ListBox and using a normal Grid for the properties view? What are the limitation you have with the current method? I usually like generic things, as that allows reusing code and forces to have in mind the global picture of the application functions.
(In reply to Arnaud B. from comment #2) > Looks great! Let’s go that way. I separated the patches. > Please, in another patch. I’m not convinced exactly for that one, the > TextView is sometimes useful, sometimes highly overkill. It might be > interesting only for some of the types (the one that contain arrays? or > arrays of things that include a string? to be decided). I might be missing something, but I don't see what is to lose by showing a larger text space. I think it should be used for any type that can potentially grow beyond what fits into a single line. Editing a large text in a single line is very cumbersome. I usually copy the content to gedit, work on the text, then copy back to dconf-editor. A few more visible lines would have solved it. > What are the limitation you have with the current method? I usually like > generic things, as that allows reusing code and forces to have in mind the > global picture of the application functions. None, except the bug with the TextView focus :). https://bugzilla.gnome.org/show_bug.cgi?id=789676
Created attachment 363971 [details] [review] Row width patch Separating patches
Created attachment 363972 [details] [review] Change default editor to TextView Separating patches
(In reply to Davi from comment #4) > Created attachment 363971 [details] [review] [review] > Row width patch Please don’t do the “greyed-label”⇒“dim-label” change, it looks bad in :backdrop. :) Another patch if you’re interested in working on where there’s a “Fixme”. Note that the code is easily fixable, the third arg is not a warning string but a key type usually, you can use "<hack>" or something like that if you want. But there’s more work to do, as the label would not be updated when the delayed change is applied or cancelled.
(In reply to Arnaud B. from comment #6) > (In reply to Davi from comment #4) > > Created attachment 363971 [details] [review] [review] [review] > > Row width patch > > Please don’t do the “greyed-label”⇒“dim-label” change, it looks bad in > :backdrop. :) > > Another patch if you’re interested in working on where there’s a “Fixme”. > Note that the code is easily fixable, the third arg is not a warning string > but a key type usually, you can use "<hack>" or something like that if you > want. But there’s more work to do, as the label would not be updated when > the delayed change is applied or cancelled. Sorry about those. I sometimes get myself carried away and make some rushed changes that sneak into my commits. It's some work to weed out unrelated modifications and sometimes some things slip through. Now I've checked that the patch *only messes with the row widths*.
Created attachment 364065 [details] [review] Row width patch
(In reply to Davi from comment #8) > Created attachment 364065 [details] [review] [review] > Row width patch When shrinking the window at minimal size, the separator doesn’t touch the borders of the window; there’s a padding left and right. It’s not highly important, but I think it would be great to have the same visual language than in the browse view.
Created attachment 364162 [details] [review] Row width patch Remove left/right padding in rows
There’s always a small padding! (smaller!)
Created attachment 364165 [details] [review] Row width patch No more padding!!11
Comment on attachment 364165 [details] [review] Row width patch This one looks good (and without padding!!), it’s pushed, thanks. :) Let’s go back to the TextView story then…
(In reply to Davi from comment #3) > I might be missing something, but I don't see what is to lose by showing a > larger text space. I think it should be used for any type that can > potentially grow beyond what fits into a single line. I have a problem in ‘/ca/desrt/dconf-editor/saved-view’, that contains here and now “/ca/” at the just-beginning of a big white rectangle, and that just looks bad. Basically, the “string” type is rarely used for more than one word/path/url/id/keyboard shortcut, that are one-line only data, and making easy to have EOL in there just feels like a bad idea. At the opposite, I do agree that with the “array of string” type, a TextView would be for half the use cases a big improvement. It would not with the ‘(ssm(dd))’ type used by GWeather, so only when there’s an array; including in ‘/org/gnome/Weather/Application/locations’ (even if you really never want to edit that manually, it’s an ‘av’ type). I do agree it would help for ‘/org/gnome/desktop/interface/gtk-color-palette’, but that key format just looks bad. For ‘/org/gnome/gnome-system-monitor/proctree/columns-order’, the white sea is overkill; it’s an ‘ai’ type, array but no string. Some apps also use ‘ai’ for window size (Polari, notably). For ‘/org/gnome/maps/last-viewed-location’, the utility of a TextView might be discussed; it’s an ‘ad’ type. For ‘/org/gnome/settings-daemon/plugins/color/night-light-last-coordinates’, it’s not really useful, it’s a ‘(dd)’ type. For ‘/org/gtk/settings/color-chooser/custom-colors’, it would be better; it’s an ‘a(dddd)’ type. So, a fast check says that a TextView would be useful for half of the ‘as’ types, and for most of the others ‘a*’ types (but not ‘ai’). Maybe using an TextView each time there’s an array would be quite good enough –and I think that people should not have window size in an ‘ai’ anyway. So, I think it would be great to create a new KeyEditorChild, for things with an array [of whatever]. > > > This is not in the patch, but would there be a problem in moving away > > > from a ListBox and using a normal Grid for the properties view? > > > What are the limitation you have with the current method? > > None, except the bug with the TextView focus :). > https://bugzilla.gnome.org/show_bug.cgi?id=789676 It’s workaroundable (trusting the bug report), so let’s try not to change all for that. In that case, I usually add a comment with a link to the gtk+ bug next to the workaround.
Created attachment 364223 [details] [review] Add some padding back We took away too much padding, so some of it was bound to come back. KeyEditorChildDefault was too padding-less.
Comment on attachment 364223 [details] [review] Add some padding back Ok.
(In reply to Arnaud B. from comment #14) > [...] > > So, a fast check says that a TextView would be useful for half of the ‘as’ > types, and for most of the others ‘a*’ types (but not ‘ai’). Maybe using an > TextView each time there’s an array would be quite good enough –and I think > that people should not have window size in an ‘ai’ anyway. > > So, I think it would be great to create a new KeyEditorChild, for things > with an array [of whatever]. Ok. > It’s workaroundable (trusting the bug report), so let’s try not to change > all for that. In that case, I usually add a comment with a link to the gtk+ > bug next to the workaround. I could not figure what the workaround would be. Do you have any ideas?
Created attachment 364302 [details] [review] TextView based editor for array types
(In reply to Davi from comment #18) > Created attachment 364302 [details] [review] [review] > TextView based editor for array types Use the TextView for any type that contains an array (“"a" in type_string”), not just for arrays (“type_string.has_prefix("a")”). (In reply to Davi from comment #17) > I could not figure what the workaround would be. Do you have any ideas? I’ll have a look.
Created attachment 364408 [details] [review] TextView based editor for array types - Apply for every type with an "a" - Some cleanups (Still no fix for the focus issue)
(Needs a little more right margin.)
Created attachment 364568 [details] [review] Fix. The fix was given in the demonstration patch[1] of bug 789676, you can include it in your patch. [1] https://bug789676.bugzilla-attachments.gnome.org/attachment.cgi?id=362588
Created attachment 364577 [details] [review] TextView based editor for array types + right margin - Fix the focus issue with the TextView - Wrap the TextView inside a ScrolledWindow - Add some more right margin to Entry and TextView editors
Yay. It would be great to have a keyboard shortcut to apply a value being modified. Usually it’s just “Return”, maybe here “<Alt>Return” as there’s no use of EOL (but maybe the opposite would be better). Also, the TextView should have a little more white padding, even “padding:0.1em 0.15em;” looks better. Oh, and –I forgot–, add a link in comment to the gtk+ bug of the textview, so we can find back the reason of these two weird lines. ^^