Overhaul image resize dialog
ClosedPublic

Authored by rkflx on Apr 22 2018, 8:22 PM.

Details

Summary

D12300 added two percentage spinboxes, making the dialog look somewhat
crowded. It was also unclear which of the fields affected the image's
height, and which the width.

To polish the dialog and make it easier and faster to use, the following
changes were implemented:

  • Change window title to "Resize Image".
  • Remove redundant intro text.
  • Add width and height labels.
  • Don't use an italic font face for the current size.
  • Align everything to the center of the dialog.
  • Add "px" suffix to all sizes.
  • Use 99,999 instead of 100,000 for the maximum size to optimize the width of the spinboxes.
  • Drop redundant "Percentage" label.
  • Don't show unnecessary decimals in the percentage spinboxes.
  • Change maximum value for percentage to a more even 1000%.
  • If "Keep aspect ratio" is enabled, disable the then unnecessary right percentage spinbox.
  • Use a single grid layout instead of multiple sub-layouts.
  • Prevent resizing the dialog, as with the default size everything should fit just fine.
  • Set proper tabstops.

Depends on D12300

Test Plan

Resize dialog (press +R to open) should work as before, and
the checkbox should affect the state of the height percentage spinbox.

Before:

After:

Diff Detail

Repository
R260 Gwenview
Branch
resize-dialog-overhaul
Lint
No Linters Available
Unit
No Unit Test Coverage
rkflx requested review of this revision.Apr 22 2018, 8:22 PM
rkflx created this revision.
rkflx added a subscriber: huoni.Apr 22 2018, 8:23 PM

It occured to me that making @huoni rebase his patch on my overhaul would only create unnecessary work, so I've just gone ahead and based my patch on D12300 instead.

Also, as you can see, compared to the mockup I left out the frame for Current size. This way there is a somewhat unpleasant visual gap between the label and the number, but it also means the new size is easier recognizable and there is less visual noise, which I deemed to be more important. Let me know if you dislike it so much that I should change it back.

Looks like this change resulted in the dialog gain a maximize button. Intentional?

rkflx added a comment.Apr 22 2018, 9:12 PM

Looks like this change resulted in the dialog gain a maximize button. Intentional?

That's how KWin works: Once you can resize the dialog (see summary), there will be a maximize button.

Right, because I see that you made it horizontally resizable. What was the reason for that?

rkflx added a comment.Apr 22 2018, 9:30 PM

Right, because I see that you made it horizontally resizable. What was the reason for that?

https://phabricator.kde.org/D12300#inline-62557

I would prefer a non-resizable dialog whose UI can't possibly be too small. Reverting the combobox size change might yield that. But it's a minor thing and I won't press, so this is the last I'll say on the subject.

rkflx added a comment.EditedApr 22 2018, 9:33 PM

Hm, maybe I should change it back to a fixed size? Let's wait what people think.

Is there any HIG recommendation for this?

Reverting the combobox size change might yield that.

Not sure what you mean by that?

Hm, maybe I should change it back to a fixed size? Let's wait what people think.

Is there any HIG recommendation for this?

Reverting the combobox size change might yield that.

Not sure what you mean by that?

Sorry, wrong term. In this diff, you made the spinboxes narrower. If that change were reverted, then there's even less of a chance that content inside them would ever be cut off, and therefore even less of a reason to ever need to horizontally resize the dialog, so then the dialog size could be made fixed.

rkflx added a comment.EditedApr 22 2018, 9:38 PM

Hm, maybe I should change it back to a fixed size? Let's wait what people think.

Is there any HIG recommendation for this?

Reverting the combobox size change might yield that.

Not sure what you mean by that?

Sorry, wrong term. In this diff, you made the spinboxes narrower. If that change were reverted, then there's even less of a chance that content inside them would ever be cut off, and therefore even less of a reason to ever need to horizontally resize the dialog, so then the dialog size could be made fixed.

No, Qt makes the spinboxes narrower, because it adapts them to the maximum size (at least in theory). I won't revert that change. Normally everything should fit, that's why it probably does not make sense to allow resizing (so maybe I'll revert that part).

Question is: Is Qt always right, or are there situations where users might want more space?

rkflx updated this revision to Diff 32847.Apr 22 2018, 9:46 PM
rkflx edited the summary of this revision. (Show Details)

Revert allowing to resize horizontally.

Let's wait what people think.

No need to wait for that, @huoni originally set a fixed size, after all.

Revert allowing to resize horizontally.

Let's wait what people think.

No need to wait for that, @huoni originally set a fixed size, after all.

We're definitely not losing any functionality, considering resizing the dialog before our patches had no benefit.
And sizeHint/minimumSizeHint should always be big enough to fit the maximum value + suffix.

Resizing before:

huoni added inline comments.Apr 23 2018, 3:02 AM
lib/resize/resizeimagedialog.cpp
51–52

I found the proper (and less cryptic) way to do this:

mainLayout->setSizeConstraint(QLayout::SetFixedSize);

From the docs, QLayout::SetFixedSize means:

The main widget's size is set to sizeHint(); it cannot be resized at all.

rkflx updated this revision to Diff 32927.Apr 23 2018, 10:25 PM
rkflx edited the summary of this revision. (Show Details)
  • Rebase
  • Use more elegant resize prevention (thanks, @huoni!).
  • Put blurb in commit message back in, as my patch is adding it and not @huoni's.

Resizing before:

Yes, but I fixed it to expand the spinboxes instead (in my first iteration, at least). Anyway, "no resizing" it is.

rkflx marked an inline comment as done.Apr 23 2018, 10:26 PM

Looks good. :)
Only arc patch fails:

Cherry Pick Failed!
Exception 
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D12300'
[...]
huoni added a comment.Apr 24 2018, 6:44 AM

Looks good. :)
Only arc patch fails:

Cherry Pick Failed!
Exception 
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D12300'
[...]

I think that's because I've updated D12300, and this hasn't been rebased.

rkflx added a comment.Apr 24 2018, 7:15 AM

Looks good. :)
Only arc patch fails:

Cherry Pick Failed!
Exception 
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D12300'
[...]

I think that's because I've updated D12300, and this hasn't been rebased.

It's not so much about that you've updated the other revision, it's more about that you landed it. Arcanist does not fail when trying to apply my patch, it fails already when trying to apply the dependent revision, i.e. your D12300:

Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D12300'

STDOUT
On branch arcpatch-D12458
You are currently cherry-picking commit c3be1d86.

nothing to commit, working tree clean


STDERR
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Cherry-picking a patch which already has been applied only works when specifying --allow-empty, which Arcanist does not do ATM. We might have to look into whether we can change some defaults here, or if it can be changed upstream…

@muhlenpfordt For now, just use --skip-dependencies.

rkflx added a comment.EditedApr 24 2018, 7:36 AM

it fails already when trying to apply the dependent revision, i.e. your D12300

Ah, this is interesting: Only issuing arc patch D12300 works, because it attaches the branch to a parent revision before the commit, which does not happen if it is applied as a dependent revision. Hm…

@muhlenpfordt For now, just use --skip-dependencies.

I applied the raw diff but that works too. ;)

huoni added a comment.Apr 24 2018, 8:01 AM

Cherry-picking a patch which already has been applied only works when specifying --allow-empty, which Arcanist does not do ATM. We might have to look into whether we can change some defaults here, or if it can be changed upstream…

@muhlenpfordt For now, just use --skip-dependencies.

Thanks for the explanation.

lib/resize/resizeimagedialog.cpp
166–174

Now that the GUI disables the Height Percentage when the aspect ratio is locked, this code is not needed.
Perhaps replace it with a comment?

it fails already when trying to apply the dependent revision, i.e. your D12300

Ah, this is interesting: Only issuing arc patch D12300 works, because it attaches the branch to a parent revision before the commit, which does not happen if it is applied as a dependent revision. Hm…

Analysis continues in T8506, in case you are interested.

lib/resize/resizeimagedialog.cpp
166–174

Are you sure? As far as I can see this would break updating the width percentage when changing the pixel height.

huoni added inline comments.Apr 24 2018, 11:09 PM
lib/resize/resizeimagedialog.cpp
166–174

You're right (without testing to make sure). These Boolean guards are so confusing I confused myself!

rkflx added inline comments.Apr 24 2018, 11:13 PM
lib/resize/resizeimagedialog.cpp
166–174

Yeah, it is confusing. I spent a lot of time today trying to improve readability by using QSignalBlocker, but while this worked in 95% of the cases, ultimately I failed at that.

Let's not touch it and hope it will just work now…

rkflx marked 4 inline comments as done.Apr 24 2018, 11:13 PM
huoni accepted this revision.Apr 25 2018, 7:07 AM

Well the patch LGTM. Only one small quibble.
I suppose I will accept it!

lib/resize/resizeimagewidget.ui
41–43

I'm guessing this, and all the other value property tags were added by Qt Designer?
I think we should probably remove them since they are unnecessary.

This revision is now accepted and ready to land.Apr 25 2018, 7:07 AM
rkflx added inline comments.Apr 25 2018, 7:11 AM
lib/resize/resizeimagewidget.ui
41–43

I added them, so the dialog would look somewhat reasonable with an example in the visual editing mode. Otherwise it was a bit hard to see how the final dialog would look like.

As this value is overwritten by Gwenview later on anyway, I could not find any situation where this is causing harm.

But I can remove it if you want?

huoni added inline comments.Apr 25 2018, 7:14 AM
lib/resize/resizeimagewidget.ui
41–43

My reasoning was that future devs might wonder why the values are set when they are just overwritten. And less 'code noise' so to speak.
But your reasoning also makes sense.

In the end I don't mind. As you said it does no harm. I'll leave it up to you :)

rkflx updated this revision to Diff 33095.Apr 25 2018, 7:10 PM
  • Remove example values in favour of defaults
  • Rebase
This revision was automatically updated to reflect the committed changes.

Sorry, I'm late...
Just noticed a small issue with the units inside the spinboxes. If I press End the cursor jumps beyond px or % - I expected it stopping after the last digit.

rkflx added a comment.Apr 28 2018, 8:13 AM

Just noticed a small issue with the units inside the spinboxes. If I press End the cursor jumps beyond px or % - I expected it stopping after the last digit.

In general that's how it is implemented in Qt, we are only setting the suffix. It's not ideal, but nothing for Gwenview to fix easily. Might as well be a bug in Qt (see also https://stackoverflow.com/questions/40756382/editing-behavior-of-backspace-key-of-qdoublespinbox).