Improve message in pre-0.92 file dpi upgrade

Patch with hyperlinks activated.

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"

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

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).

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...

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.

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 :)

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();
                     ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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))

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.

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.

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

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.

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 ...").

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

@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

Thanks su_v for the feedback!

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?

ported to trunk r15729