#3 various [hidden] preferences editor

closed-fixed
interface (8)
5
2011-07-28
2011-06-07
Dimitar Zhekov
No

An extra Edit -> Preferences tab for editing the hidden prefs (renamed to various). All prefs are safe to change AFAICT (checked them one by one), but a few require restart. The screenshot should be self-explanatory.

Discussion

1 2 > >> (Page 1 of 2)
  • Matthew Brush
    Matthew Brush
    2011-06-07

    This is really cool. It would be better if you could edit the TreeView directly instead of needing the extra widgets at the bottom though.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-08

    Indeed. But I don't have enough experience with gtk+ to create a cell-editor with mixed widgets (and possibly a cell pop-up menu with a "Reset" entry), and am not sure it's worth so much effort - these prefs are not changed often. But if the lead devs put the in-place editing as a condition to include the patch, I'll try to write one.

     
  • Note for Enrico and/or Nick: I'm not sure about reviewing the prefs part, so if you can, please do :)

    A few comments without really much testing, so it's mostly a first impression/patch read:

    1) you forgot to rename stash_group_set_write_once() to stash_group_set_various() in stash.h:38. This leads to missing prototypes in calling code. You can use -Werror-implicit-function-declaration GCC flag to detect those.

    2) similarly, you forgot to include ui_utils.h in stash.c

    3) we use doc comments only for things that are port of the plugin API, use simple comments otherwise.

    4) there are a few typos in some comments (e.g. "variuos")

    5) not sure, but in prefs.c:1227, shouldn't you take care of allow_always_save?

    6) scrolled window should have "automatic" policy on both scrollbars

    7) stash.c:988: string should be translatable (at least in French we put a space before the colon)

    8) stash.c:1230: please, no. If you really don't want to support reentrency (OK, it's not *strictly* needed here), be honest and make this global outside the function, so at least it's visible (e.g. directly in stash.c:693 making StashEditorData an anonymous struct).

    9) the statusbar pref change isn't correct because it breaks if the user had the default value (empty) saved.

    Finally, I second codebrainz's remark: editing would better be done in the value column. The thing I think is the most problematic with the current UI (apart the fact it'd be better friendly with the editing in-place) is that the editing widget position isn't static, which makes the UI look crappy :/

    I did a small testcase for in-place editing, and actually it isn't that complicated to do: simply pack all possible value renderers in the value column, and show them conditionally depending on whether the value is of their type [1].
    Joined is the test code I wrote if you want it as an example. The only problem for Geany is that GtkCellRendererSpin was introduced in 2.10… but if we finally happen to bump to 2.12 as Enrico wanted/suggested, it wouldn't be a problem anymore.

    [1] Or of course, the "hard" way: write a proxy cell renderer. gconf-editor does this, so you may take inspiration from them if you prefer to go this way.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-14

    Thanks for the review, Colomban.

    1) 2) ACK. On top of that, ui_widget_set_tooltip_text() takes GtkWidget * not GtkButton *. It would have been nice to have -Wimplicit-function-declaration or even -Wall as default.

    3) ACK.

    4) Couldn't find anything but "variuos" - perhaps my english is not that good?

    5) Not any more. I recently sent a small fix for always save, and ui_save_buttons_toggle() handles it now.

    6) ACK.

    7) ACK, also some other strings.

    8) That's because I wanted sed visible only to the relevant callbacks. But since reentrancy is easy - why not? :)

    9) Yes... I changed the default to the real translatable string, but it'll not be replaced automatically in the existing configuration files. Any suggestions? Editing an empty string will not be convinient.

    And thanks a lot for the sample, I didn't even knew where to start. It'll still take some time.
    Proxy cell renderer - not with my knowledge of gtk.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-18

    Screenshot with in-place editing

     
    Attachments
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-18

    Here's the new version. Inline editing, fixes for 1-9 (and there may new bugs, of course), some other improvements.

     
  • Looks quite good!
    Again, a few remarks:

    1) Maybe rename "Various" to "Advanced" or something?

    2) You can use GtkCellRendererSpin now we depend on GTK 2.12

    3) Tons of warnings on the CLI! (GtkCellRendererToggle has no property "editable", it's "activatable")

    4) Statusbar pref saving still isn't prefect... We'd better not save it if it is the default, because then the user would benefit of the updates, and it would keep being translated normally (e.g. in the same language as the UI, not the one used on the first run)... difficult problem.

    5) the algorithm behind stash_tree_update() looks a slow to me, can't we do something more linear?

    I've made a "review patch" that fixes some of the problems, and highlights some other, attached as various-prefs-5850-review.diff.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-22

    Thanks again. Now:

    1) Makes no difference to me. I vaguely remember a discussion about that, with Enrico (Nick?) agreeing about these being "settings that don't fit in any other category" or something...

    2) I wrote it with 3 renderers first, as in your multitypes example, but the problem is that all non-Text renderers are slow. We have 15 boolean settings out of 20, so not much can be done about the checkers, and putting another 5 of them is not a big deal. However, 20 extra spins really kill the performance.

    A proxy renderer will do, but gconf-editor's is 450+ lines without decent integer support, and with at least 1 buggy signal. Not exactly inspiring.

    3) ACK.

    4) Yes, but how to do that without too many hacks?

    About i18n - if you right-click statusbar_template and choose Reset, that'll set a current-language default value. Well you can't reset it at the moment, since that's not implemented...

    5) It does ~3390 full updates per second on my 2GHz CPU. Even with 200 settings instead of 20, that'll still be ~34 updates per second. Compared to the non-Text renderers "speed", 'tis zilch.

    5850-review.diff: applied without the gint/spin-related parts, replaced the stash_tree_update() comment with a short explanation about the speed.

    One problem I noticed: if you edit an int/string, and choose OK/Apply without confirming the new value first, it will be reverted. Most inline editors work in content windows (tables, subtitles, gconf), so canceling is the safe choice, but we have a dialog with dedicated Cancel button. Maybe it's a good idea to try g_connecting to entry's focus-out and confirm the value.

     
  • 1) right, it seems I even participated to this discussion (while already suggesting advanced)… hum, forget it :D

    2) Slow? How "slow" is it? Is it really noticeable? The SpinButton provides quite convenient ways to change the value (like keyboard shortcuts -- pgup, pg down, up, down --, arrows, etc.)

    4) See the ML, but I guess just show the empty value is OK

    5) Forget it. Anyway it'd even not be a problem it it'd took half a second to proceed -- we do this only once when applying anyway. So forget my remark, it was more about improving the algorithm than really about slowness.

    About "unvalidated" setting on update) Well... yes, true. I wonder if there isn't a way to force editing validation of cell renderers, that could be done in stash_tree_update_pref(). gtk_cell_editable_editing_done() should be the one.
    However if it's too complicated to do, maybe it's not that important.

    BTW, I'd better see the stash_tree_update_pref() function moved just before stash_tree_update() since it's only used in this last function.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-25

    2) Hmmm, not that slow at all - about the same as another five visible and activatable checks. I probably messed up something in my initial (not published) implementation, it was nearly 2 times slower with integers and quite noticable. Clearing "sensitive" may also help a bit.

    4) OK, that's the easiest thing to do.

    And, when editing int/string, clicking a button (or another page name) confirms the value. :) Probably requires 2.20+ to work. Someone with an old gtk+ should try it, there may be CLI warnings.

    Since the default status_template is now empty, and we assume that editing Various will require the manual, I don't think a Reset button makes sense any more. The regular settings have no such extra anyway.

    And so, 5850-d should be the final version, except for any bugs or warnings.

     
  • It's me again!

    2) cool then, I definitely prefer the experience with the spin button :)

    About saving on focus leave, I changed the implementation quite a bit, because it needed GTK 2.20 to work properly (without it it wasn't possible to cancel at all). Additionally, though the patch should be self-explanatory on these points:
    - integer settings don't seem to need the hack, so I removed it for them
    - I used gtk_cell_editable_editing_done() in :focus-out-event, and it seems to handle cancellation perfectly well.
    I tested the whole patch with both Debian Sid (GTK 2.24.4) and Lenny[1] (GTK 2.12.12) and it seems to work fine on both :)

    Finally, I have two more remarks (sorry for not having formulated them before, but just came to my mind now):

    1) Documentation needs an update. Could you do it? (please :D)
    2) Maybe a link on the pref page to the documentation of these preferences would be a good thing?

    Ah, and we use an indentation width of 4, so if there is some alignment it should be done for this width ;) (I think of the inclusions at the top of stash.c)

    Anyway, good job! With the few changes the patch I attach introduces, I think it's OK to commit -- apart the two little things I mentioned above.

    [1] Virtualization's good :p

     
    • assigned_to: nobody --> colombanw
     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-26

    Yes, spin renderer doesn't need the hack, and is out of sync with text renderer. Probably spin's focus out is executed before text's, and gets the canceled flag before text has the chance to set it. I wonder if we should rely on that - looks more like a bug to me.

    gtk+ says that remove-widget must be emitted after editing-done. Now, we know that text renderer's focus-out emits one, but...

    BTW, why do you speak about validation? From what I see, the text rendered simply installs an entry focus-out signal and unconditionally sets editing canceled (though it's a mystery to me what happens another part of the list is clicked). Anyway, I'd like to know if there's anything else, and either make the comments more clear ("magically" bugs me), or remove the hack altogether, as it only affects statusbar_template.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-26

    Forget it, I'm removing the hack, because it blocks entry's popup menu. Yes, we can trap the popup populate and unmap signals, but that's a bit too much. The spin renderer menu is blocked too, which is probably a gtk+ bug, but at least typing a digit or two (in our case) needs no input method changes or unicode characters.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-26

    OK, changed the documentation, except for a Various page screenshot - my theme and fonts are quite different.

    About the link - unfortunately, glade 2.12.2 does not seem to support such a widget. Of course, I can add one programatically. Or maybe set the hbox spacing to 0 and add a prefix label, followed by a link and a suffix label, as in labels-with-link.png - looks better than all-blue, indented text. RFC.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-26

    updated geany.txt, removed the focus-out hack

     
  • No problem for the screenshot, Enrico will take care of it short before the release. Adding a placeholder image may be a good idea though. (done in the patch)

    Well, forget it. It's not really doable with GTK < 2.18 (which supports links in labels), and anyway we have the Help button -- just make this one work. I did it in the patch, it's fine.

    For the doc, great! I just don't really like the way whether the pref needs restart & co is exposed, and I have an alternative suggestion (in the attached patch). Any remarks?

    Once again I join a review patch, but we should be close to the final version :)

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-27

    > we have the Help button -- just make this one work.

    Really... Well, since the manual states which settings need restart in much more detail, I replaced the restart notice with a suggestion to read the documentation. Maybe remove it alltogether? Having a Help button should be enough for anybody... though I haven't used one in years, since the help is usually something like "enter the file name in the File name field".

    > I just don't really like the way whether the pref needs restart
    > is exposed, and I have an alternative suggestion (in the attached
    > patch). Any remarks?

    In the text version, the first two cells read to me as "Applies immediately to new documents", especially because they are the first ones. Swapped them.

    > we should be close to the final version

    Aye. We tried a lot of things, feels like time to settle.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-27

    applied 5861-g-review, small changes

     
  • > Really... Well, since the manual states which settings need restart in
    > much more detail, I replaced the restart notice with a suggestion to read
    > the documentation. Maybe remove it alltogether? Having a Help button should
    > be enough for anybody...

    Yes, maybe just remove it. Though, I think that maybe the warning about restarting can be relevant since all other prefs in the dialog don't need restart. Or maybe both infos to something like "Some preferences needs restart to take effect, please refer to the documentation" or "Some preferences needs restart to take effect. Please refer to the documentation for further details" ?

    I really don't like the "click the Help button" you put in the last version, looks tricky to me ^^
    Ah, and we should use a GeanyWrapLabel for that label so it don't make the dialog expand or something. My suggested (cosmetic) changes are in the joined patch. The string may be easily improvable, not sure. Maybe we can fix this a little later anyway if we get some better ideas (though it'd be better to bet it right before translators start translating it).

    > though I haven't used one in years, since the help
    > is usually something like "enter the file name in the File name field".

    True, it took a long time before I even notice the button myself :D So sad we're used not to use such help integration...

    > In the text version, the first two cells read to me as "Applies
    > immediately to new documents", especially because they are the first ones.
    > Swapped them.

    OK, makes sense.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-29

    Since both of us had problems noticing the Help button, I think pointing the user to it makes some sense. But if the message requires a wrap label, taking more vertical space from the list, I'd rather remove it. So cutting the text a bit, instead of applying 5861-h, it's a matter of taste/opinion. Hopefully the translations will fit.

    (BTW it's "Use", not "click" - I'm a pre-GUI person.:)

    Now, there's nothing else left, so I say commit it, and let the developers and interested users decide if/what message would be the best. For me, nothing/anything short and simple will do.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2011-06-29

    release candidate

     
  • Finally committed. Sorry for the long delay, and thank for the good work!

     
1 2 > >> (Page 1 of 2)