Differential D11378

Save last used crop ratio settings
ClosedPublic
Actions

Authored by huoni on Mar 16 2018, 5:49 AM.

Details

Reviewers
rkflx
Group Reviewers
Gwenview
Commits
R260:c1e227f13af1: Save last used crop ratio settings
Summary

The crop tool currently remembers if you have Advanced settings enabled.
This patch similarly remembers the Restrict to image ratio option
(Basic view) and the chosen Ratio (Advanced view).

Given how we store the ratios, and allow custom ratios using the same
combobox, we need to store both the current index of the combobox and
the ratio values.

I have removed CropTool::onWidgetSlidedIn because all it did was
ensure the crop rect was set, and we now always load a crop ratio
triggering the rect to update. If we leave it in, it actually resets
the rect after the saved ratio is loaded.

Note that if you enable the crop tool, then go to next image, any
changes will not be saved, including Advanced settings. This is
because the config is only saved when the tool is deactivated,
which is not triggered in this case.

Depends on D11202

BUG: 391757

Reimplement function now needed again, fix bug, add const

Save blank ratio combobox instead of last chosen ratio (if it's blank)

Crop ratio combobox is now saved properly

Test Plan

Change ratio constraint with checkbox in Basic or the combobox
in Advanced. Ensure changes are saved after closing the crop tool
and closing Gwenview entirely.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
huoni requested review of this revision.Mar 16 2018, 5:49 AM
huoni created this revision.
rkflx requested changes to this revision.Mar 19 2018, 12:09 AM

Works quite well, I found only two issues:

  • The custom ratio is not saved between invocations (neither the combobox "(non)-selection", nor the actual value).
  • Enter custom ratio value, open combobox: No entry should be selected, while currently Gwenview selects the last remembered one.

Note that if you enable the crop tool, then go to next image, any changes will not be saved, including Advanced settings. This is because the config is only saved when the tool is deactivated, which is not triggered in this case.

I found that too, only afterwards I realized you already had this in your summary ;) Can we do something about this? Losing the crop state is not a big deal, but in case KImageAnnotator was ever added, this would look different. Just as an idea, could we disable navigation once a tool is active? Note I do not want to disable the other tools, because e.g. rotating while cropping is kinda nice. Anyway, please open a new task or a new bug for this topic, so it won't get lost.

lib/crop/croptool.cpp
436–439

I wonder why this has to be removed? If there is a good reason, it should at least be added to the commit message.

lib/crop/croptool.h
66–67

Instead of adding an empty implementation, you could also just remove this completely (as this is not a "pure" virtual).

(But see also other comment.)

lib/crop/cropwidget.cpp
319

Why does this read > and not >=? You might want to add a comment if there is a good reason, although I don't see any currently (I guess it still works because you set 0 when initializing, but nevertheless this is a bit ugly…).

lib/crop/cropwidget.h
51

Add const.

53

Add const.

This revision now requires changes to proceed.Mar 19 2018, 12:09 AM
huoni added a comment.Mar 19 2018, 5:27 AM
In D11378#228924, @rkflx wrote:

Works quite well, I found only two issues:

  • The custom ratio is not saved between invocations (neither the combobox "(non)-selection", nor the actual value).

Saving the non-selection can be fixed, but there's a problem with saving custom ratios. The only way I can see to do that is to store the actual text string, since KConfig doesn't seem to support floats/qreals. But storing the text won't work for the preset items, because 1) they are translated strings, and 2), there are duplicates like US Letter, where we couldn't determine which one was saved.
The only solution I can think of is to store the text for custom ratios, and the index like the patch does currently for the preset ratios. Seems overly complicated though. What do you think?

  • Enter custom ratio value, open combobox: No entry should be selected, while currently Gwenview selects the last remembered one.

This is a pre-existing bug, unrelated to my patch. Will investigate a fix.

Note that if you enable the crop tool, then go to next image, any changes will not be saved, including Advanced settings. This is because the config is only saved when the tool is deactivated, which is not triggered in this case.

I found that too, only afterwards I realized you already had this in your summary ;) Can we do something about this? Losing the crop state is not a big deal, but in case KImageAnnotator was ever added, this would look different. Just as an idea, could we disable navigation once a tool is active? Note I do not want to disable the other tools, because e.g. rotating while cropping is kinda nice. Anyway, please open a new task or a new bug for this topic, so it won't get lost.

https://bugs.kde.org/show_bug.cgi?id=392036 :)

lib/crop/croptool.cpp
436–439

The reason was that since we were setting a default value in D11202, the crop was updated anyway, making this redundant.

However now that D11202 is not setting that default value, I will add this back.

lib/crop/cropwidget.cpp
319

Unfortunately, this is a mistake. Of course 0 is a valid index....

huoni added inline comments.Mar 19 2018, 5:51 AM
lib/crop/cropwidget.h
51

Should setAdvancedSettingsEnabled also be const then?

rkflx added a comment.Mar 20 2018, 12:54 AM

there's a problem with saving custom ratios. The only way I can see to do that is to store the actual text string, since KConfig doesn't seem to support floats/qreals.

You are right that saving a string is not great due to locale issues. I guess QString::toDouble in cropRatio might even be wrong, perhaps this should use QLocale::toDouble. Nevertheless, https://api.kde.org/frameworks/kconfig/html/annotated.html mentions ItemDouble, so width and height could be stored separateley, as well as as the combobox index?

This is a pre-existing bug

Right, sorry. I just jotted down some notes and did not double-check with an unpatched version.

lib/crop/cropwidget.h
51

Oversimplified, use const if you just "read" a property, and don't use it if you "write" a property. More here: https://stackoverflow.com/a/2157477

huoni updated this revision to Diff 29963.Mar 20 2018, 5:29 AM
huoni edited the summary of this revision. (Show Details)
  • Ensure custom ratio, as well as "blank" is saved
huoni added a comment.Mar 20 2018, 5:31 AM
In D11378#229625, @rkflx wrote:

Nevertheless, https://api.kde.org/frameworks/kconfig/html/annotated.html mentions ItemDouble, so width and height could be stored separateley, as well as as the combobox index?

How did I miss that :(

lib/crop/croptool.cpp
436–439

I take this back, it actually broke loading saved ratios. Explanation added to commit message.

rkflx added a comment.Mar 21 2018, 1:09 AM

(To be continued.)

lib/crop/croptool.cpp
424

I would name this ratio instead of size ;)

lib/crop/cropwidget.h
51

Sorry, maybe I wasn't clear enough here. I meant that set* should not have const.

(If you hover over an inline comment in Phab, it shows which lines I selected when adding the comment, i.e. what I was referring to.)

rkflx requested changes to this revision.Mar 21 2018, 9:48 PM

Thanks, looking even better now, and with the comments you added e.g. in setChosenRatio the code is really easy to understand.

(To be continued.)

I'm done. 26h to go ;)

lib/crop/croptool.cpp
434

Same thing, for me ratio seems more logical than size.

lib/crop/cropwidget.cpp
82

const

110

const

140

Don't forget to change this to q->restrictToImageRatio().

This revision now requires changes to proceed.Mar 21 2018, 9:48 PM
huoni updated this revision to Diff 30166.Mar 22 2018, 12:52 AM
huoni marked 3 inline comments as done.
  • Rename size>ratio, remove const from set* functions
  • Add const, use accessor function
huoni added a comment.Mar 22 2018, 12:52 AM

Thanks, looking even better now, and with the comments you added e.g. in setChosenRatio the code is really easy to understand.

I do try to be liberal with the comments! :)

lib/crop/cropwidget.cpp
140

Thanks for the reminder :)

huoni marked 5 inline comments as done.Mar 22 2018, 12:54 AM
rkflx accepted this revision.Mar 22 2018, 10:49 AM
This revision is now accepted and ready to land.Mar 22 2018, 10:49 AM
Closed by commit R260:c1e227f13af1: Save last used crop ratio settings (authored by huoni). · Explain WhyMar 22 2018, 11:32 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 30217

View Options

lib/crop/croptool.h

Loading...
View Options

lib/crop/croptool.cpp

Loading...
View Options

lib/crop/cropwidget.h

Loading...
View Options

lib/crop/cropwidget.cpp

Loading...
View Options

lib/gwenviewconfig.kcfg

Loading...