Improve message in pre-0.92 file dpi upgrade

Bug #1659229 reported by Mc on 2017-01-25
20
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Inkscape
Low
Mc
0.92.x
Low
Mc

Bug Description

The current dialog has several UX drawbacks : non-technical people might find it too technical, it gives almost no hint on which option to prefer, and "Ignore" might be an ignored option, while it would in fact be the better option for anyone not wanting to send his/her drawing to print.

Ideally, we should rework on it to perform some autodetections and context (maybe based on http://imgur.com/h6uEOHm ), but since there is hard freeze for .92.1 we could at least try to reword it for 0.92.1

================

Relevant IRC parts from tonight:

22:35 < Mc> I might want to land a string change
22:38 < Mc> to add something like "<b>What to do now?</b> If your document is intended to have a size in pixels, choose Ignore. If you wanted it to keep some size in real-life units, choose to set viewbox."
22:53 < Mc> I'm not for removing the technical explanation
22:54 < Mc> just add some rule of thumb for people not knowing what it implies for them
22:53 < Mc> I'm not for removing the technical explanation
22:54 < Mc> just add some rule of thumb for people not knowing what it implies for them
23:20 < deevad> Mc: su_v ; maybe http://www.peppercarrot.com/extras/temp/2017-01-24_screenshot_231857_net.jpg ? with "more informations" you can fold or unfold , so it does't look like a dangerous text on the way of the user ?
23:21 < Mc> well, in your case, none of the options work :p
23:21 < Mc> you want what's currently "ignore"
23:21 < deevad> "auto-convert" might sound a bit dangerous ; "auto-adapt"
23:22 < deevad> ignore sounds dangerous too ; as if I'm missing to do something important
23:27 < deevad> http://www.peppercarrot.com/extras/temp/2017-01-24_screenshot_232619_net.jpg :) I have now 0.92.1pre0 running for test ! thanks Mc again for the guidance.
01:48 < Mc> maybe http://imgur.com/gFYZCBH
01:49 < Mc> (for now "Advanced" contains Tav's text)
01:51 < Mc> (also, I think it could be a good idea to maybe add a link (in "Advanced") to a page on our website where we would have leisure to add more information about this change)
01:53 < Mc> (current diff : https://paste.fulltxt.net/rzD0zFVbTOt9 )
02:02 < Mc> http://imgur.com/3JxeQP1
02:07 < bryce> does this sound at all accurate?
02:07 < bryce> Scaling behavior has changed in this version of Inkscape.
02:07 < bryce> How would you like to update this old file to current?
02:07 < bryce> 1. Resize the printed page to fit drawing
02:07 < bryce> 2. Resize the drawing to fit the printed page
02:07 < bryce> 3. This is for screen display only, so disable scaling
02:14 < su_v> bryce: your first item is not what actually happens - the page is not changed. the viewBox scales the content to match the intended physical size
02:14 < su_v> AFAIU
02:15 < su_v> only if the file was entirely defined in px (e.g. based on default template from 0.48), the width, height are scaled (and the viewBox set to the old values)
02:17 < su_v> if the width, height are e.g. in 'mm' (A4 template from 0.48), then it only needs a viewBox
02:17 < bryce> if you have a more info... link then there is no need to make mention of CSS compliance; that can be moved to the full explanation page
02:22 < Mc> bryce: would it be a faq item ?
02:22 < su_v> Mc: basically, the first and the second option both scale the drawing, just using different methods (viewBox, or scale each drawing element) - the intent is that a page defined in physical units keeps the exact size (and
              the content is scaled to match its relation to the page size)
02:23 < Mc> it's scaled so that 96 user units = 1 external inch
02:23 < Mc> right ?
02:27 < bryce> 1. Shrink or enlarge drawing elements in relation to the page, keeping physical dimensions constant; or 2. Keep drawing elements constant with respect to the page, changing dimensions as needed; or 3. Do not apply
               scaling corrections to the drawing.
02:27 < su_v> but no prompt.
02:27 < bryce> more accurate?
02:28 -!- bastianilso [~<email address hidden>] has quit [Quit: bastianilso]
02:28 < su_v> 1 and 2 are the same
02:28 -!- Ede_123 [~Miranda@55d4473c.access.ecotel.net] has quit [Quit: Miranda NG! Smaller, Faster, Easier. http://miranda-ng.org/]
02:29 < su_v> (the methods of the two options differ internally - viewBox for scaling, or scale each object)
02:29 < su_v> AFAIU
02:31 < su_v> each method can have drawbacks (can expose bugs in inkscape; might not be supported well by what might post-process SVG file)
02:33 < su_v> scaling via viewBox is more stable I think (except that some extensions might produce incorrect results in such files); scaling elements of complex drawings (clones, masks, clips, etc.) doesn't always keep appearance (bugs)
02:34 < Mc> (hence recommendation to use "fit page")
02:34 < su_v> scaling elements is often preferred by anyone who does not want transforms or other attributes like viewBox, but simply wants the values in element attributes to represent the actual position or length
02:35 < su_v> Mc: yes, I agree that it is "recommended" (current "set 'viewBox'"), if scaling is needed.
02:36 < su_v> I do understand though why (some) users would prefer to scale each element instead.
02:38 < su_v> I just have the impression that the difference that now is made (on that proposal) does not exist: both options aim to preserve the physical size of the page area, and the original relation of the content to that page area.
02:42 < su_v> if you have an A4 page defined in px (based on 90 px per inch), and draw a rect which is 10.5 cm wide; the result of either option should be that the page area still is 21cm wide (defined in px based on 96 px per inch),
              and the rect measures 10.5cm in width (half the page)

=================

My current diff:

https://paste.fulltxt.net/81o741QbUrMzR

Tags: ui Edit Tag help

Related branches

Mc (mc...) on 2017-01-25
Changed in inkscape:
milestone: none → 0.92.1
status: New → In Progress
importance: Undecided → Low
jazzynico (jazzynico) on 2017-01-25
tags: added: ui
jazzynico (jazzynico) wrote :

Patch with hyperlinks activated.

Eduard Braun (eduard-braun2) wrote :

I don't like the new wording at all... It doesn't improve upon the status-quo (i.e. normal users won't understand it better) and makes some technical explanations less precise (i.e. technically experienced users will understand it less).

Some important points:
* "physical size" vs. "real-life size" - I honestly don't know what a "real-life" size should be. My display and TV have "real-life" sizes, yet they're pixel based. For every one working with UI design on computers pixels are very reals sizes. My recommendation: Keep "physical size" and explain it better (i.e. things that are meant to appear in a fixed size on a page of paper / are required to have a specific absolute size like a ruler)
* "Set 'viewBox'" vs. "Fit page to new size" - How is the page fitted? It's unexplained and therefore unclear. Tell the whole story: "Scale whole page as a group (without modifying size of any single objects; objects will remain unscaled internally)".
* "Scale elements" vs. "Scale drawing to fit page" - Does this mean the page remains unscaled? Better: "Scale page and all graphic elements individually (all objects will be scaled to the desired size separately)"
* "Ignore" vs. "My drawing is px-sized" - Both do not really explain what happens: "Do not rescale (graphic remains unscaled which might result in a changed physical page size while pixel based dimensions will stay constant)"

The parts I added in parentheses () above are meant as additional information (probably to be included in the message or alternatively as pop-up text) but I think it's necessary to better explain the individual options rather than to explain the background of the change in to much detail. For the user the only important information is "Physical size of one pixel has changed resulting in a different scaling of the image"

jazzynico (jazzynico) wrote :
Mc (mc...) wrote :

Proposed patch for implementing http://i.imgur.com/YF7MCWl.png (mizmo's proposal)
(also cf mailing-list relevant discussion)

Mc (mc...) wrote :
Hachmann (marenhachmann) wrote :

Do people really say 'information are'? According to all grammar sites I've now visited, that's not standard English, but more like German and French (I know it comes from Mairin, so maybe some local language quirk?)...

> The scale\n of the artwork relative to the document size may not be accurate.

When I read this, I think that my artwork isn't scaled, but only the page becomes bigger in relation to it, so a bit like like in the first row in the attached image (but I didn't get an answer on the mailing list, so I still don't know if I understand it correctly now).

su_v (suv-lp) wrote :
Download full text (4.7 KiB)

Patch from comment 6 (last version, 2017-01-26 19:43:10 UTC) tested with lp:inkscape/0.92.x r15346 on Ubuntu 14.04.5 LTS (GNOME desktop) and on OS X 10.7.5 (GTK+/X11):

1) Refactored code:
* Local tests [1] confirm so far that the refactored internal feature produces the same result as unpatched r15346 (the patch combines two separate code blocks which used in part duplicated code). The correlation between the new (and nested) radio choices and the numbering of the 'result' remains odd though AFAICT (and error prone I guess when edited later by other contributors): result 3 is radio choice 1 ("recommended"), result 2, 3 are radio choice 2_1, 2_2.
The refactoring of the internal logic should be reviewed code-wise by the original author (Tav).
* FIXME: console warnings when the dialog opens:
(inkscape:11741): Gtk-CRITICAL **: gtk_box_pack: assertion 'child->parent == NULL' failed
* TODO: re-test/clarify whether legacy guides/grids/3dboxes are intended to be converted/adjusted to user units as needed (internal detail) if the document is kept as is ('Ignore' in original dialog) - I did not pay attention to this aspect while testing the patch.

2) User information:
I understand that the text provided in the dialog is now discussed with UX experts and users on the 'inkscape-devel' mailing list. I will therefore refrain from commenting on the specifics, apart from aspects which are mentioned below in the notes about usability.

3) Usability:
Initial impression: The dialog based on the proposed patch feels a bit "over-engineered" with its multiple hidden sections and nested radio choices, slightly more complicated to use and (based on my experience during testing) more prone to wrong or unintended choices than the old one (with one checkbox, and three simple buttons):
* The effect of the current active choice for physical output can't be quickly grasped because there is too much text (of similar length and wording) for each choice: The user each time has to reassure herself whether the selected option is indeed the intended method. Memorizing which one was used last time when a affected file was opened may be difficult.
* The secondary level of radio choices (for physical output) should be indented one level, in correspondence with the logic/relation of the dialog's options.
* The nested structure of radio choices requires more action (more clicks) to access e.g. the feature labeled 'Scale elements' in the old dialog.
* Keyboard navigation is more complicated with the new dialog: the user has to focus attention entirely on the dialog, and remember where to use <Tab> and where cursor keys are required, and in which sequence.
* The label 'Convert' of the only remaining button is incorrect for the recommended "default" action (digital artwork for screen display) - the file is opened as is (no conversion happens).
* There is no "default" action to quickly proceed (with <Enter>).
* There is no defined behavior for 'Cancel' (<Esc>). Maybe Inkscape should cancel opening the file altogether, or - this needs to be discussed - open the file without any conversion (and without creating a backup file, independent whether that option is checked or not).

5) 'More...

Read more...

su_v (suv-lp) wrote :

Minor detail: the proposed patch (intended for the stable release branch lp:inkscape/0.92.x) introduces C++11 extensions when compiled with (experimental) GTK3 backend:

../../src/file.cpp:468:21: warning: 'auto' type specifier is a C++11 extension [-Wc++11-extensions]
                    auto content = scaleDialog.get_content_area();

C++11 is no strict requirement for Inkscape 0.92.x (it is detected and builds are configured accordingly as needed depending on requirements in dependent libraries (C++ bindings)).

The (experimental) GTK3 build (GTK+ 3.18.9, 3.20.5) also has this console message (same for the 'New from Templates' dialog, and the unpatched dpi-change dialog):
Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.

Mc (mc...) wrote :

Warning fixed, renamed convert into "OK", added commandline option, added pref to remember last choice, set focus on "OK", moved update code into file-update.cpp, called it from a place where it'll be called even in commandline mode. I consider it's (feature-wise) ready for 0.92.1, and further refinements to the text or UI can be discussed in the ~40h left before string freeze. If someone wants to address the shrinking or window priority, feel free to :)

Mc (mc...) wrote :
su_v (suv-lp) wrote :

Updated patch to fix (experimental) GTK3 build of 0.92.x attached - please review.

  CXX file-update.o
../../src/file-update.cpp:366:22: error: no viable conversion from 'Gtk::Box *' to 'Gtk::Box'
            Gtk::Box content = scaleDialog.get_content_area();
                     ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Eduard Braun (eduard-braun2) wrote :

Adding a screenshot with 1659229-dialog-v5-gtk3-fix.diff applied taken on Windows 10.

Is this how it's supposed to look?
I'm missing a bit of indentation for the two "physical" selection options.

On 2017-01-29 13:16 (+0100), Eduard Braun wrote:
> Is this how it's supposed to look?
> I'm missing a bit of indentation for the two "physical" selection options.

Yes, that's how it currently is implemented (no indentation of the
nested radio option). Mc mentioned in this regard on irc:
> [3]memorizing is done, indent is not (I don't know how (yet))

su_v (suv-lp) wrote :

Latest patch from Mc (comment 11,12) tested successfully on Ubuntu 14.04.5 LTS and OS X 10.7.5 with lp:inkscape/0.92.x r15346.

TODOs for 0.92.1:
* Code review.
* Text review by UX experts, users (mailing list).

TODOs for 0.92.2:
* Indent the secondary level of radio choices (for physical output).
* Shrink dialog size when collapsing expanded sections.
* Support non-gui invocations via command line (export to SVG and other formats).
* Enforce stay-on-top state for dialog on all platforms (cf. Clip Art import dialog).

The new command line options provided by the patch have been summarized in comment 1 of
* Bug #1659489 “Add command line options for dpi change”
  https://bugs.launchpad.net/inkscape/+bug/1659489
I propose to discuss details about the implementation of the command line options separately in bug #1659489.

Bryce Harrington (bryce) wrote :

I did some review work on the patch, but it feels like it needs further attention. Unfortunately, this was already pretty late in the freeze, and now we're past schedule on the release, so I think it's best if this patch be left for 0.92.2. It's a good solid patch though and definitely needs inclusion, but better if it not be rushed in.

I've broken the patch down into three sub-patches for easier review. The first is a mechanical refactoring with no logic changes to move routines from file.cpp to file-update.cpp. I picked a different function name. This also does some minor whitespace cleanup to try and improve consistency with surrounding code. I may have missed some whitespace but otherwise this patch is pretty good to go.

Second is the dialog replacement itself. I rebased this on top of the previous patch but didn't get a chance to do a line by line review; I've indicated some specific TODO's that need attention.

Third is the command line to go with the dialog. As I understand it, this needs expanded on to allow its use in no-gui mode. It also needs a man page entry, and I've left a few other TODO's.

Importantly, all the above also need to be landed on trunk. That could be done while waiting for 0.92.1 to be finished.

Martin Owens (doctormo) wrote :

On Thu, 2017-02-02 at 03:23 +0000, Bryce Harrington wrote:
> I did some review work on the patch, but it feels like it needs
> further attention.  Unfortunately, this was already pretty late in
> the freeze, and now we're past schedule on the release, so I think
> it's best if this patch be left for 0.92.2.  It's a good solid patch
> though and definitely needs inclusion, but better if it not be rushed
> in.

If the updates are just the visual widgets and no functionality, then
having an improved version in 0.92.1 would be good no matter what we do
to make it better in 0.92.2 right?

Is there a down side?

Best regards, Martin Owens

Jabiertxof (jabiertxof) wrote :

This patch add a wider window and have less height, indent the two optional (2) radio buttons and auto grow-shirnk the dialog on usage.

Also now don't need fixed line breaks in the descriptions what is good for translators and diferent default font sizes/themes.

Please I coulden't test it on Windows & Mac. Only tested in Debian Stretch with GNOME 3.

su_v (suv-lp) wrote :

Patch from comment 18 (1659229-dialog-v7-plus3patches.diff) tested with lp:inkscape/0.92.x r15355 on OS X 10.7.5:
1) The indentation of the secondary level of radio option is fixed (thx!).
2) The dialog height shrinks if one of the expanded sections is collapsed (thx!).
3) The minimal width of the dialog seems to have grown by > 50%, with lots of empty space to the right of the radio options (see attached screenshot). IMvHO the initial paragraph and the expanded 'More details...' section are harder to read (the lines are too long). Could the dialog support some sort of of dynamic layout for the text and allow resizing to a smaller width?
4) There's a small typo where previous line breaks had been joined (missing space character: "... we needto make it compatible ...").

Jabiertxof (jabiertxof) wrote :

Hi su_v. Thanks for review the patch!

Just fix the typos and make the window tinny. Tested with 800x600 and up. With 800x600 resolution you need to contract the more info expander to apply.

About make it resizeable: Is the only way i find with lots of trys to make the dialog shirnk on "more info" collapse

su_v (suv-lp) wrote :

@Jabiertxof - possibly the text content (e.g. in the 'More Details' section) will change for the final version depending on what is being agreed upon in the discussion among users on the inkscape-devel list [1]. This might also change the amount of text to fit into the expanded sections.
Since the dialog height now shrinks back to the minimal height when collapsing an expanded section, I'm personally a bit less worried about the max height (for 800x600 resolution) - overall, (IMvHO) the dialog proportions with 1659229-dialog-v8-plus3patches.diff seem more balanced (screenshot attached).

--
[1] http://inkscape.13.x6.nabble.com/UX-help-tt4978575.html#none

Jabiertxof (jabiertxof) wrote :

Thanks su_v for the feedback!

Jack Underwood (ocean-wolf) wrote :

In reply to the png image of comment #21, I find the text much improved, apart from the last radio button which reads like gobbledegook:

"The accuracy of the physical unit size and position values of objects in the file is most important."

did you mean something like:
"The precise physical (printed) location of objects within this file is most important."

Also with the previous option, do clips, masks, filters, clones, etcetera fall into a category? Saying "such as" indicates that the elements you describe next are just examples, so it would feel nice to have this tidied up into a simple adjective like "effect elements such as...". As there exists only a finite number of these elements, can't the dialog auto-detect whether such elements exist within the file and tailor the message accordingly, i.e. leave out this option if the user doesn't have any of these elements within the file?

jazzynico (jazzynico) on 2017-02-16
Changed in inkscape:
milestone: 0.92.1 → 0.93
Mc (mc...) wrote :

ported to trunk r15729

Changed in inkscape:
assignee: nobody → Mc (mc...)
status: In Progress → Fix Committed
To post a comment you must log in.