Improve resizing options page UI #2520

twodoorcoupe · 2024-06-28T09:13:52Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences:
Improved the image resizing options page ui as suggested by @Aerozol in Add more options to image resizing #2515.

Problem

JIRA ticket (optional): PICARD-XXX

Solution

Now the new ui looks like this:

Action

Additional actions required:

Update Picard documentation (please include a reference to this PR)
Other (please specify below)

rdswift

Overall, this is a nice improvement.

I still have a concern that we are inconsistent in the way that missing / unselected scaling dimensions are handled between the various processing options. For example, with the "Crop to fit" and "Stretch to fit" methods an unspecified dimension is taken to be that dimension from the source image, while an unspecified dimension in the "Maintain aspect ratio" method is taken to be an infinitely large number. This inconsistency could lead to confusion and unexpected results, and thus needs to be made very clear to the user (in my opinion). I'm not sure that the examples in the processing option tooltips make that inconsistency sufficiently clear.

Personally, I think having to specify target dimensions for both directions in all cases is much cleaner. In each processing case, the final image would fit within the target dimensions specified.

picard/ui/forms/ui_options_cover_processing.py

rdswift · 2024-06-28T15:43:52Z

Just to clarify my earlier comment a bit, typically when a program asks for target dimensions for an image it is both dimensions (at least in my experience) and specifies the (maximum) canvas size into which the final image is placed. Depending on the scaling process used, one (or both) of the final dimensions of the resulting image may be less than the corresponding target dimension. To only specify one dimension is unusual.

twodoorcoupe · 2024-06-28T16:36:37Z

Just to clarify my earlier comment a bit, typically when a program asks for target dimensions for an image it is both dimensions (at least in my experience) and specifies the (maximum) canvas size into which the final image is placed. Depending on the scaling process used, one (or both) of the final dimensions of the resulting image may be less than the corresponding target dimension. To only specify one dimension is unusual.

I understand. I guess I should probably wait for @zas' opinion on this, since he was the one who initially proposed allowing to ignore one of the dimensions.

rdswift · 2024-06-28T16:50:59Z

I understand. I guess I should probably wait for @zas' opinion on this, since he was the one who initially proposed allowing to ignore one of the dimensions.

I'll support whatever the consensus is. I was just expressing my opinion. I think that the functionality that you've added to Picard in terms of image processing is a great improvement, and will be well received by the community. I want to let you know that, to help offset some of the frustration you might be feeling with my UI comments. Thanks for all your work on this.

rdswift · 2024-06-28T21:48:09Z

@twodoorcoupe,

I hacked together a few suggestions regarding my comment above (and a few other things) into a commit on my fork of the repository: 337e80e (replacing commit ~~7236a8e~~ which had errors)

Feel free to use as much or as little as you want from it. I actually tried to submit it as a pull request to your fork of the repository, but for some unknown (to me) reason, your fork doesn't appear in the list of repositories on the pull request form.

Aerozol · 2024-06-28T22:17:07Z

Great work!
I think there are two changes to be made, the first of which may address one of @rdswift's concerns;

Width and height should be called 'Maximum width' and 'Maximum height', and should be radio buttons, not checkboxes (only one should be selectable, if the ratio is being maintained*)
Rather than 'don't enlarge', let's go with 'Enlarge' and default to unchecked (Currently it can be translated to "Do don't enlarge" haha)

*When 'stretch' or 'crop' is selected in the drop down, then the current menu in the screenshot is valid (width and height should both be set).
Hopefully I've imagined everything correctly!

- Create new `picard/const/cover_processing.py` file to hold new `COVER_RESIZE_MODES` and `COVER_CONVERTING_FORMATS` constants. - Create `COVER_RESIZE_MODES` constant list of modes to display in the combo box. Each entry is a named tuple comprised of 'mode', 'title' and 'tooltip' to keep all information together for ease of maintenance. - Create `COVER_CONVERTING_FORMATS` constant list of allowed formats to display in the combo box. - Dynamically populate the resize modes and image formats combo boxes from the `COVER_RESIZE_MODES` and `COVER_CONVERTING_FORMATS` constants to facilitate changes in the options or display order without having to recompile the UI file. - Fix i18n `N_()` function missing on some tooltips. - Add missing copyright notice for Giorgio Fontanive in `picard/const/defaults.py` - Minor formatting changes.

rdswift · 2024-06-29T04:49:04Z

I agree that perhaps identifying them as "Maximum" dimensions might help clarify. In addition, it may be as simple as including a statement on the settings page that says something like:

Note that if a maximum size is not specified for a horizontal or vertical dimension:

in the case of the "Crop to fit" and "Stretch to fit" modes, the size of the source image for that dimension will be used;
in the case of the "Maintain aspect ratio" mode, resizing will not be constrained in that dimension.

It still makes me squirm a little doing the resizing based on a dimension over which the user has no control (taking the size from the source image), but I guess if the user is dumb enough to select the "Crop" or "Stretch" options and not specify both dimensions they deserve what they get. (Looks like my nasty side is showing through. 😉)

EDIT: Here's another thought for consideration... If a dimension is not specified, instead of using the corresponding dimension from the source image, how about using the same value specified for the other dimension? In other words, if only one dimension is specified make the target a square.

zas · 2024-06-29T05:56:18Z

If having one dimension set is confusing, I have no issue with getting rid of it.
Combination of source image proportions and current options leads to a high number of cases.

Changes suggested by @rdswift in 337e80e look good to me.

Aerozol · 2024-06-30T03:45:08Z

I suspect that only implementing part of my suggestion (the drop downs), without changing the menus, has ended up not helping all that much. This is a full mockup with all three menus, where I feel the UI is clear:

Now that I think about it though, it is could indeed be useful to specify max height and width (I wonder why Adobe has just gone with one):

I don't fully follow the discussion re. "taking the size from the source image". I assume my mockups here aren't taking that into account - I would expect both sizes to have to be filled out, for crop and stretch.

rdswift · 2024-06-30T15:26:28Z

I don't fully follow the discussion re. "taking the size from the source image". I assume my mockups here aren't taking that into account - I would expect both sizes to have to be filled out, for crop and stretch.

That was my main point. The current UI only requires you to specify one dimension (optionally leaving the other dimension unspecified) for all three processing cases ("crop", "stretch", and "maintain aspect ratio"). As I understand it, if only one dimension is specified, the other dimension for the "crop" and "shrink" options is taken to be that dimension from the source image. To illustrate, if you have a source image that is 400x200 and the user has specified either "crop" or "stretch" processing but has only provided a target horizontal size as 100, the current processing will take the vertical size of the source image (200) as the target vertical size. With the original image being:

the resulting 100x200 target image for the "crop" processing would be:

Using the same settings and source image but changing the processing to "stretch", the resulting 100x200 target image would be:

If the scaling option is "maintain aspect ratio" and only one target dimension is specified, the other target dimension is ignored (as an infinitely large number). This is using a different default for a missing dimension than that used for the "crop" and "stretch" cases, but is really only evident/relevant if an image is being scaled up. Using our original 400x200 image and only specifying a horizontal target dimension of 100, the resulting 100x(infinity) image would look like:

My thinking is that the user needs to understand what's happening and what defaults are being applied for missing dimensions, so that they don't end up with unexpected results. Another option is to always require both target dimensions to be specified if the "crop" or "stretch" processing option is selected.

Aerozol · 2024-06-30T20:09:57Z

Thanks @rdswift. I think the confusion is solved in my full mockups?

e.g. Makes it clear that only one dimension can be followed with 'maintain aspect ratio', and does away with setting a dimension to null for 'crop' and 'stretch'. I don't know if the functionality that is lost by removing the null dimension (thanks for the screenshots!) has a use case.

rdswift · 2024-06-30T21:38:18Z

Makes it clear that only one dimension can be followed with 'maintain aspect ratio',

But you can currently set targets for both dimensions for "maintain aspect ratio", and it can make sense to do so. For example, if you want your final image to keep the same aspect ratio, and to be no larger than 1000 in either direction, but ensure that at least one of the dimensions is no smaller than 1000:

Source	Target H	Target V	Final Image	Comment
2000x1000	1000	Not Set	1000x500	Okay
2000x1000	Not Set	1000	2000x1000	Too large horizontally
2000x1000	1000	1000	1000x500	Okay
1000x2000	1000	Not Set	2000x1000	Too large vertically
1000x2000	Not Set	1000	500x1000	Okay
1000x2000	1000	1000	500x1000	Okay

This could be the use case that makes sense of allowing both dimensions to be specified for the "maintain aspect ratio" option, as @twodoorcoupe has it currently implemented. I think it would be a shame to remove this.

The way I look at this is that the target dimensions specified define the (maximum) size of the frame into which the output image must fit. If the mode is "maintain aspect ratio" it is okay to remove one of the bounds because the other bound limits the size. In both the "crop" and "shrink" cases, both dimensions need to be defined because they don't have the benefit of a constant aspect ratio to determine the missing dimension.

My point is that if the user isn't specifying these dimensions, they need to clearly understand how the missing dimension will be determined. Currently the missing dimensions are determined by using the corresponding dimension from the source image, over which the user generally has no control, so they may result in an unexpected final product. Personally, I'd like to force the user to specify both dimensions when using the "crop" and "shrink" modes, rather than automatically selecting a default value outside of the user's control.

At the same time, I can understand the simplicity of only specifying one dimension in the "maintain aspect ratio" scenario if the the user doesn't care about the final size of the other dimension. An example use case might be an image of booklet pages where perhaps only a height is specified to provide some consistency while accommodating images that are different widths (such as an image with one page vs an image with two or more pages in the same image).

All of this tremendous flexibility already exists in @twodoorcoupe's code. I think it would be a shame to remove or disable any of it. I would just like to find a way to make it clear to the user how the settings are used (or derived) so that they can make intellegent choices and maximize the benefit of the functionality (and not end up with unexpected results).

At the very least this is looking like it's going to fill an entire section of the Picard User Guide, which unfortunately a very small percentage of the users actually read.

rdswift · 2024-06-30T21:55:12Z

Another thought... If we want to keep the option of not specifying one of the dimensions, perhaps when that setting is unchecked the spinbox can be hidden and replaced with a label stating something like "Dimension not considered" for the "maintain aspect ratio" option, and "Taken from source image" for the "crop" and "stretch" options. That still allows the flexibility, but makes it clear to the user what to expect.

twodoorcoupe · 2024-07-01T08:31:49Z

Thank you so much for your help folks, especially @rdswift!

Regarding the optional dimensions, if we want to allow to ignore a dimension only when aspect ratio is being kept, maybe it's more intuitive to consider them as separate resize modes, like "Scale to width" and "Scale to height".

I think this simplifies both the ui and the code. Here's an example of what I mean:

The tooltip makes it clear that the aspect ratio is also kept for these two options.

Then, when either scale to width or scale to height is selected, the ignored dimension is disabled:

Otherwise, I can try @Aerozol's suggestion and remove the width and height checkboxes when the resize mode is crop or stretch.

Aerozol · 2024-07-01T08:50:29Z

I think having separate options for 'scale to width' and 'scale to height' is quite an elegant solution - anyone confused by the complexity of the 'maintain aspect ratio' setting can select those, and rest easy.

Leaving the greyed out options there presumably gives quite a smooth transition between settings, as well? e.g. no change in positioning for the options. Probably pointing out the obvious, but for the greyed out options I would replace '1000' with nothing, or maybe a dash.

twodoorcoupe · 2024-07-01T09:56:47Z

Leaving the greyed out options there presumably gives quite a smooth transition between settings, as well? e.g. no change in positioning for the options

Yes this is the case

Probably pointing out the obvious, but for the greyed out options I would replace '1000' with nothing, or maybe a dash.

Done. I preferred making them show nothing as the code was much easier

zas

It looks good, just few comments.

picard/const/cover_processing.py

picard/const/defaults.py

picard/ui/options/cover_processing.py

rdswift · 2024-07-01T14:13:38Z

Regarding the optional dimensions, if we want to allow to ignore a dimension only when aspect ratio is being kept, maybe it's more intuitive to consider them as separate resize modes, like "Scale to width" and "Scale to height".

Great idea! I think this resolves all of my concerns. Clear to the user what to expect, and no automatic defaults over which they have no control. Well done!

The only (minor) thing that I throw out there for consideration is to possibly show the "Scale to width" and "Scale to height" options indented or proceeded with a hyphen or something to indicate that they are subset options of the "Maintain aspect ratio" option. Something like:

Maintain aspect ratio
  - Scale to width
  - Scale to height
Crop to fit
Stretch to fit.

Probably not necessary, but it might make it a bit clearer that the aspect ratio will be maintained for those options.

twodoorcoupe · 2024-07-02T08:06:09Z

The only (minor) thing that I throw out there for consideration is to possibly show the "Scale to width" and "Scale to height" options indented or proceeded with a hyphen or something to indicate that they are subset options of the "Maintain aspect ratio" option.

Not sure how to do this properly, I think adding spaces or tab characters might not always align the indented options correctly due to the font not being mono spaced. Maybe I could use an em space with \u2003 instead?

This is what it looks like as of now with space, hyphen, space.

zas · 2024-07-02T08:45:38Z

IMHO, formatting / indenting to indicate the option preserves aspect ratio is a bad idea, and it may not translate very well.

What about using a radio button instead and changing the list:

[x] Preserve aspect ratio
[Scale / Scale to width / Scale to height]

[ ]  Preserve aspect ratio
[Crop to fit / Stretch to fit]

Or even radio buttons in one group but displayed in 2 sets:

Aspect ratio preserved:
 [ ] Scale    [ ] Scale to width     [X] Scale to height

Aspect ratio not preserved:
 [ ] Crop to fit   [ ] Stretch to fit

Dunno, the indentation approach doesn't look right to me.

@Aerozol what do you think?

picard/ui/options/cover_processing.py

twodoorcoupe · 2024-07-02T09:16:27Z

IMHO, formatting / indenting to indicate the option preserves aspect ratio is a bad idea, and it may not translate very well.

Maybe it's best to just keep the drop down list and not indent anything.

If it's not clear enough that those two options keep the aspect ratio, maybe we can find different names like "scale width (keep ratio)" or something more explicit. There's already the tooltip that explains what the option does as well.

zas · 2024-07-02T09:24:01Z

Maybe it's best to just keep the drop down list and not indent anything.

Yes, likely.

zas

LGTM

rdswift · 2024-07-02T14:55:45Z

Maybe it's best to just keep the drop down list and not indent anything.

I'm okay with that. It was only a minor suggestion. Certainly not a show-stopper. For most people it will be a "set it once and forget it" option anyway, and we can explain it thoroughly in the documentation (that nobody reads).

phw

@twodoorcoupe This UI is a great improvement. This addresses my concerns about the complexity, while allowing even more options.

Thanks @rdswift and @Aerozol also.

twodoorcoupe added 2 commits June 28, 2024 09:15

Add explicit method for getting qimage to ProcessingImage

2357b2c

Improve image resizing options UI

6aa6317

rdswift reviewed Jun 28, 2024

View reviewed changes

picard/ui/forms/ui_options_cover_processing.py Outdated Show resolved Hide resolved

rdswift force-pushed the improve_processing_ui branch from 7236a8e to 6aa6317 Compare June 28, 2024 21:15

zas requested review from phw and zas June 30, 2024 16:56

Add scale to width and scale to height resize modes

d0670b3

Make spinboxes show nothing when disabled

aa49786

zas requested changes Jul 1, 2024

View reviewed changes

picard/const/cover_processing.py Outdated Show resolved Hide resolved

picard/const/cover_processing.py Outdated Show resolved Hide resolved

picard/const/defaults.py Outdated Show resolved Hide resolved

picard/ui/options/cover_processing.py Outdated Show resolved Hide resolved

zas requested changes Jul 2, 2024

View reviewed changes

picard/ui/options/cover_processing.py Show resolved Hide resolved

Improve processing constants and resize mode combobox

3d2cf7f

twodoorcoupe force-pushed the improve_processing_ui branch from f20d23a to 3d2cf7f Compare July 2, 2024 11:11

zas approved these changes Jul 2, 2024

View reviewed changes

phw approved these changes Jul 3, 2024

View reviewed changes

zas merged commit ad09a5a into metabrainz:master Jul 3, 2024
44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve resizing options page UI #2520

Improve resizing options page UI #2520

twodoorcoupe commented Jun 28, 2024 •

edited by rdswift

Loading

rdswift left a comment

rdswift commented Jun 28, 2024

twodoorcoupe commented Jun 28, 2024

rdswift commented Jun 28, 2024

rdswift commented Jun 28, 2024 •

edited

Loading

Aerozol commented Jun 28, 2024 •

edited

Loading

rdswift commented Jun 29, 2024 •

edited

Loading

zas commented Jun 29, 2024

Aerozol commented Jun 30, 2024 •

edited

Loading

rdswift commented Jun 30, 2024

Aerozol commented Jun 30, 2024

rdswift commented Jun 30, 2024 •

edited

Loading

rdswift commented Jun 30, 2024

twodoorcoupe commented Jul 1, 2024 •

edited

Loading

Aerozol commented Jul 1, 2024

twodoorcoupe commented Jul 1, 2024

zas left a comment

rdswift commented Jul 1, 2024

twodoorcoupe commented Jul 2, 2024

zas commented Jul 2, 2024 •

edited

Loading

twodoorcoupe commented Jul 2, 2024

zas commented Jul 2, 2024

zas left a comment

rdswift commented Jul 2, 2024

phw left a comment

Improve resizing options page UI #2520

Improve resizing options page UI #2520

Conversation

twodoorcoupe commented Jun 28, 2024 • edited by rdswift Loading

Summary

Problem

Solution

Action

rdswift left a comment

Choose a reason for hiding this comment

rdswift commented Jun 28, 2024

twodoorcoupe commented Jun 28, 2024

rdswift commented Jun 28, 2024

rdswift commented Jun 28, 2024 • edited Loading

Aerozol commented Jun 28, 2024 • edited Loading

rdswift commented Jun 29, 2024 • edited Loading

zas commented Jun 29, 2024

Aerozol commented Jun 30, 2024 • edited Loading

rdswift commented Jun 30, 2024

Aerozol commented Jun 30, 2024

rdswift commented Jun 30, 2024 • edited Loading

rdswift commented Jun 30, 2024

twodoorcoupe commented Jul 1, 2024 • edited Loading

Aerozol commented Jul 1, 2024

twodoorcoupe commented Jul 1, 2024

zas left a comment

Choose a reason for hiding this comment

rdswift commented Jul 1, 2024

twodoorcoupe commented Jul 2, 2024

zas commented Jul 2, 2024 • edited Loading

twodoorcoupe commented Jul 2, 2024

zas commented Jul 2, 2024

zas left a comment

Choose a reason for hiding this comment

rdswift commented Jul 2, 2024

phw left a comment

Choose a reason for hiding this comment

twodoorcoupe commented Jun 28, 2024 •

edited by rdswift

Loading

rdswift commented Jun 28, 2024 •

edited

Loading

Aerozol commented Jun 28, 2024 •

edited

Loading

rdswift commented Jun 29, 2024 •

edited

Loading

Aerozol commented Jun 30, 2024 •

edited

Loading

rdswift commented Jun 30, 2024 •

edited

Loading

twodoorcoupe commented Jul 1, 2024 •

edited

Loading

zas commented Jul 2, 2024 •

edited

Loading