The crop tool was lacking the ability to choose the ratio of the current image.
This patch adds:
BUG: 236970
The ratio settings in Normal and Advanced are independant of
one another, in prep for future patch which will have these remember the
chosen setting.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
So my preference here would not be to make it the default setting in basic mode. The reason being that people not using Advanced mode might not want to constrain the proportions to those of the current image, and shouldn't have to figure out that they need to open the Advanced settings to turn that off.
Since this is bound to be a popular and frequently used--and toggled--feature, perhaps we could put a checkbox to control it in the Basic view, and then put it in the Ratio menu in the Advanced view, the way you currently have it, and then have the Advanced view remember the last-used setting.
Yeah thought as much regarding default. Good idea to have a toggle in basic view. Should we also remember the last used basic view setting? That would satisfy the users that want Current Image as default.
What do you think of this mockup? Opinion on the wording?
Advanced view remember the last-used setting
Do you mean store this in the config, or just remember between toggling Advanced on and off?
Is it a good idea to have the ratio potentially change when enabling Advanced? What if the user draws a Freeform crop, then wants to tweak the position/size using the Advanced spin boxes. They would have to re-do it if it automatically loads the last-used ratio.
Yes, I think we should definitely remember the setting's status in Basic view. I think the phrase "Constrain proportions to current image" would be more natural.
I believe whether Advanced is active is already remembered, so I was referring to the last-used entry in the drop-down menu.
I'll have to think about your last comment.
Just an idea and probably out of focus here: Should we add some kind of shortcut to e.g. square and/or image dimension selection with Ctrl/⇧ + mouse drag like some image manipulation programs do?
An interesting topic and not straightforward, but here is how I imagine it should work:
This should resolve most of your concerns, and it would then also solve BUG: 391757 which was reported just today.
In case these suggestions are agreed upon, I'd suggest to split the implementation into multiple patches.
I would not worry about that, because we already change around the crop box quite a bit when changing ratios. There is no good solution to this.
Sadly that's much too long. Adding the checkbox will already increase the horizontal window size in normal mode.
Sounds like a good solution to me. Regarding splitting this up, does the following separation make sense?
Sounds good from a high level, adapt as needs evolve. Perhaps you'll find dependent revisions useful (you can just use arc feature instead of the equivalent but wordy git ... --track ...).
BTW, I didn't mean you have to implement this all by yourself (unless you want to, of course). You could also try to get others to help you.
When the next image switched its dimensions from portrait to landscape and vice versa, the last selected ratio should switch automatically too
This is going to require some re-working of how we store the different ratios I think.
When loading the config we can find the corresponding ratio from the combobox, check whether the ratio is portrait or lanscape and switch accordingly. But this would also switch the dimensions of Current Image, This Screen, etc. In other words, there's no way to determine whether a ratio is under the Portrait or Landscape heading in the combobox without a re-work of how we manage the ratios.
I guess my question is, is it worth re-working how we store the ratios in order to accommodate this? I'm thinking something like a list of custom objects that contain some metadata in addition to a ratio would be necessary.
Any other ideas?
The QComboBox stores QSizeF values as user data. Maybe it's possible to search for QSizeF::transposed() on a portrait/landscape change?
IMHO a basic change/rewrite of storing and handling the combo box entries stands in no relation to the benefits.
QSizeF::transposed() simply returns a new object with width/height swapped. I don't see how that would help determine which combobox items were under which Landscape/Portrait heading.
IMHO a basic change/rewrite of storing and handling the combo box entries stands in no relation to the benefits.
Sorry not sure what you mean. Are you saying that a change/rewrite is fine to go ahead with regardless?
I figured it would be a long shot ;) I'd say focus on the rest first, this was probably more of an optional idea…
Essentially this would need storing whether a given custom or predefined ratio was meant for a landscape or a portrait image (or rather normalize it to one variant), and inverting the ratio once an image has a different orientation.
The underlying question is what users will expect: I'd imagine for cropping to ISO/A4/… automatically changing the ratio would be handy. On the other hand, in other cases this might be annoying.
Anyway, I split this off into a bug for now, so we won't get too sidetracked here:
Bug 391884 – Investigate whether crop ratios should be inverted when image orientation changes
The underlying question is what users will expect: I'd imagine for cropping to ISO/A4/… automatically changing the ratio would be handy. On the other hand, in other cases this might be annoying.
I considered the same thing. As stated in the bug, I think we should leave this out for now, and re-visit once all other changes are implemented.
> BTW, I didn't mean you have to implement this all by yourself (unless you want to, of course). You could also try to get others to help you.
No need :)
I've submitted all changes (plus a small bug fix I noticed) in a dependency chain:
D11202: Add ability to constrain crop tool to current image ratio
D11378: Save last used crop ratio settings
D11379: Fix crop ratio combobox not updating crop when text didn't change
D11380: Lock crop ratio to current rect when holding Ctrl/Shift
Sorry for giving you more work @rkflx ! No rush :)
Thanks for splitting up your patches, this makes them so much easier to review ;) Let's see if we can make the 18.04 Beta on Thursday (only bugfixes allowed after that), but I also don't want to rush this so it might only end up in 18.08…
Sorry for giving you more work
Sorry for setting the red light on all four patches :D (It's not that bad, though…)
Most things worked really great when testing, here are some niggles to make the feature perfect (apologies if an issue belongs to a different Diff, perhaps some points can even be resolved later):
| lib/crop/cropwidget.cpp | ||
|---|---|---|
| 73–103 | Could you instead refactor static QSize screenRatio() into static QSize ratio(QSize)? | |
| 133–134 | Could be written as else if, but is this whole nesting really necessary? You always return, so this could be simplified: if() return if() return return | |
| 134 | This is the same as q->restrictToImageRatio(), isn't it? | |
| 229–230 | When changing code, let's directly use the "modern" connect syntax. | |
| 234–235 | Please use the "modern" connect syntax here too. | |
| 354–357 | Do you really need this as a separate function? applyRatioConstraint should also be slot and thus could be connected directly. | |
Sorry for setting the red light on all four patches :D (It's not that bad, though…)
That's okay, thanks for taking the time to review!
I've addressed all your issues I believe. I'm not certain about a couple of things though:
I don't mind missing the 18.04 feature freeze, happy to take this one slow :)
| lib/crop/cropwidget.cpp | ||
|---|---|---|
| 134 | I added that method in a different patch. I will update said patch, changing this line to use the new method. | |
| 181 | This is a hack. Apparently the autosizing doesn't take into account the clear button, but hardcoding a value here is obviously not ideal. | |
| 354–357 | I was following the example of slotRatioComboBoxEditTextChanged. I have now removed both, and connected them strait to applyRatioConstraint instead. | |
The way I am updating the ratios when the image rect changes (when rotating)
Well, it breaks for custom ratios (might be related to D11378#229012). Apart from that, it does not look too wrong. Maybe you could only update the value for Current Image?
| lib/crop/cropwidget.cpp | ||
|---|---|---|
| 73 | As this does not touch any member variables, could this be kept static like screenRatio was before? | |
| 75 | return size / divisor should work too. | |
| 105 | Is size() available for mDocument? | |
| 147 | Is size() available for mDocument? | |
| 181 |
I guess that's because of the way the combobox is turned into a lineedit.
…and for "ISO" it's not even working for me (perhaps you have a different clear icon?). Ideally this should be fixed in Qt, for now let's change this to 24 and be done with it. (There might be another way involving findChild and the QLineEditIconButton I discovered in GammaRay, but the minimumSizeHint of that one was 32, so too large.) It works fine for me on HiDPI (QT_SCALE_FACTOR=1.5 gwenview), because the values you set are in "virtual" pixel, so they will be scaled up too.
I'd assume this meant QFontMetrics. Anyway, the icon seems to be the problem, and not the text. | |
| 354–357 | No problem, that's a good strategy in general ;) However, I feel sometimes the old code is quite verbose and too nested. | |
Thanks again, a few code tricks I missed there.
There is a small bug where if you open the crop tool in Advanced, leave the Ratio blank, and rotate an image, the Ratio changes to the "Current Image" and the crop adjust accordingly.
I figured this would be fixed in D11378 anyway, so have left it.
EDIT: latest diff actually fixes this.
| lib/crop/cropwidget.cpp | ||
|---|---|---|
| 181 |
Didn't know that! Also noticed the clear button icon itself doesn't scale anyway.... | |
The patch series is really coming together nicely, only some finishing touches left.
| lib/crop/cropwidget.cpp | ||
|---|---|---|
| 181 |
Weird, for me it does scale perfectly, but I'm also only testing with the default Breeze icon set. I don't think that's something for this patch to figure out. | |
| 337–338 | After adding the item for "Current image", you could count() and store the index in a constant. It's not perfect, but better than assuming nobody will ever change things around. Grepping for a C++ construct is always easier than searching for a random comment you are not even aware of. | |
| lib/crop/cropwidget.cpp | ||
|---|---|---|
| 337–338 | That does sound like a better solution. Though I don't see how we can make it a constant, given we need to declare and initialize the variable separately. | |
Thanks @huoni for this nice feature and following through with all the changes and separate patches… It's a nice addition which I think should ship in the Beta. There is still the RC and the final release, so enough time to fix things in case we missed a bug.
Not sure when you'll be around again, otherwise I can land it for you around 23:00 UTC.
| lib/crop/cropwidget.cpp | ||
|---|---|---|
| 337–338 | Oops, you are right of course. There is no write-once type of variable built in, and we should also not roll our own. | |
Sorry Nate, I'm removing you only so that Huon can land this, because you still requested changes. Feel free to test and possibly accept again, though ;)
Huon implemented all of that, so I assume you are okay with the patch…
Can land now. If you're still around, should I merge stable into master after each patch, or just do one merge after landing all of them?
I seem to be having an issue rebasing the second patch. Getting a bunch of weird conflicts when I try to rebase onto origin/Applications/18.04. First CMakeLists.txt with the minor version, then continuing the rebase causes 14 more conflicts.
Would you be able to help if you're still here?
EDIT: I've merged stable into master for now so I don't leave them diverged.
I guess the problem is that you are simultaneously trying to rebase onto stable (for which the --onto switch is, i.e. to avoid the conflict with the different version strings for both branches) and at the same time you are using the instructions for landing dependent patches. Perhaps you just need this extra argument.
You can try this, otherwise I'll try to recreate the problem and figure something out.
EDIT: I've merged stable into master for now so I don't leave them diverged.
It's no problem if they are not in sync for a some hours. Eventually someone will merge them again…
Okay great. Everything looks good to me in the repo. Good job ;)
What was the final command that did the trick for you? Pondering about changes to the docs…
I got them landed properly by running git rebase --skip for all the commits that were already landed. For some reason for patch 2-4, rebasing onto stable was trying to replay the versio change commit 0001 GIT_SILENT Upgrade KDE Applications version to 18.07.70., and then all the commits from the parent patches.
I'm not understanding something here, but firstly, I thought git automatically skipped replaying commits if upstream had them. And secondly, I don't know where that first version change commit is coming from.
I tried using --onto but it didn't seem to make a difference. Perhaps I was still not doing it right.
@huoni I just checked out the Photos app in Windows 10, they call this aspect ratio "Original" instead of "Current Image", which I find really great. Thoughts? Now/later/never?
It's possible to make crop, then crop again. At the moment, Current Image is the ratio of the last crop. "Original" would suggest it was the original image's ratio, so we'd have to think about changing that functionality I think.