"indent" attribute in extension parameters adds indentation between children (i.e. checkbox and its label))

Bug #1662035 reported by Patrick Storz on 2017-02-06
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Low
Patrick Storz

Bug Description

If the "indent" attribute is used on extension parameters this does not only add indentation to the UI element for this parameter but also between possible children of this UI element.

This is because indentation is implemented as additional "padding" in calls to "Gtk::Box::pack_start".

Patrick Storz (ede123) wrote :

I suggest to move the actual indentation from each separate "parameter.cpp" to extension.cpp instead (there we can indent the whole box consisting of any UI elements a parameter is potentially made of).

@jazzynico: Subscribing you as you initially implemented the indentation feature in r10529 [1].

If you don't have any complaints or want to work on it yourself (assigning me for now) I'll see what i can do, probably tomorrow.

[1] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/10529

Changed in inkscape:
assignee: nobody → Eduard Braun (eduard-braun2)
Patrick Storz (ede123) wrote :

Adding a quick screenshot of the issue for reference

su_v (suv-lp) on 2017-02-06
tags: added: extensions-plugins ui
jazzynico (jazzynico) wrote :

No problem, you can work on it. Thanks!

Changed in inkscape:
importance: Undecided → Low
milestone: none → 0.93
status: New → Triaged
Patrick Storz (ede123) wrote :

Here's a first version of a patch for this issue.

The diff is a bit bulky as I did some reformatting of constructor signatures plus some automatic whitespace fixes (sorry about that), however changes in functionality should be minimal:
- Most importantly every "Parameter" now has an "_indent" member variable (specifying the indentation level; set in "Parameter::make()" (see parameter.cpp)
- Indentation is achieved by using "set_margin_left()" on the parameter's widget. There are only two occurrences (one in prefdialog.cpp; the other in notebook.cpp) unless I missed anything.

The most visible changes which I hope to discuss:
- Previously the "indent" attribute often had no effect at all, e.g. for notebooks but also for parameters with 'appearance="full"'. These conditionals are not (yet) implemented but I'm unsure if I should actually add them back since I find this behavior not only unpredictable but also unnecessary: Why not let the extension author decide freely which parameters to indent? This removes a lot of complexity and makes it much easier to use the attribute. In fact in all cases where internal extensions specify indentation it has no effect (don't know if this was intentional...).
- "description"s always used an indentation level of at least one (even if no indentation was specified). While this might make sense in some cases (where descriptions are used to explain parameters) I guess it's also mostly unexpected among extension authors. In my opinion "description" parameters should behave like all other parameters. After all case mentioned before would be the perfect use case for indentation and I guess there are many cases where a default indentation would be undesirable.

Let me know what you think and if/what I should change in the patch!

Patrick Storz (ede123) wrote :

Any comments on the patch?

Hachmann (marenhachmann) wrote :

(no comment on the patch, but thanks for highlighting the attribute - it's documented now :) )

Patrick Storz (ede123) wrote :

Thanks ;)
Had that on my todo list but wanted to wait for the patch to land (as it determines for which elements this parameter is actually valid).

Either way it's good that it is documented now as I only learned of this parameter by accidentally stumbling across it in the Inkscape source by using Google (I had noticed that the "boolean" parameter I added in r15483 for the Scour extension didn't line up with a "description").

Patrick Storz (ede123) wrote :

I went ahead and pushed an updated version of the patch with the fix for this bug [1].
I also did some more improvements in commits [2-4].

Let me know if any of the UI changes (they should be marginal, though) do not work out nicely for you or if I regressed anything.

I'm also still planning some smaller updates, so input is welcome.

[1] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15508
[2] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15509
[3] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15510
[4] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15511

Changed in inkscape:
status: Triaged → Fix Committed
Hachmann (marenhachmann) wrote :

@Eduard: in the .cpp files, there are a couple more of those, do you happen to know if they are used at all?

E.g. I couldn't find what 'gui-tip' (sounds like tooltip, but that's gui-description), 'scope' and 'ext' do, and if they are relevant to script extensions at all (or maybe just to built-in ones, or not at all accessible from 'outside').

Patrick Storz (ede123) wrote :

@Maren: Those members date date back to before the beginning of time (i.e. before the bzr repository was created). As far as I can see they're completely unused so we might as well remove them in future...

Hachmann (marenhachmann) wrote :

@Eduard: thank you for doing some code archaeology :)
So documenting them doesn't make sense. Good to know for sure (been looking for gtk calls that involve those variables, but there weren't any I could find)!

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers