Bug 392036 - Discuss how to handle image navigation when tool is activated
Summary: Discuss how to handle image navigation when tool is activated
Status: RESOLVED FIXED
Alias: None
Product: gwenview
Classification: Applications
Component: general (show other bugs)
Version: 17.12.3
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Gwenview Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-19 05:20 UTC by Huon
Modified: 2018-03-21 01:30 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Huon 2018-03-19 05:20:16 UTC
Currently when a tool is activated, and the user navigates to the next or previous image, the tool is deactivated but no tool settings are saved.

There are three solutions I can think of:

  1. Save the settings when navigating to another image
  2. Keep the tool active (with same settings) when navigating to another image
  3. Disable navigation when a tool is active

Thoughts? I think the third option is simplest, and I don't see a need to accommodate navigating with a tool active. If you navigate away, I'm guessing you want to re-assess how you use the tool anyway.
Comment 1 null 2018-03-19 16:43:30 UTC
(This was split off from a discussion while working on the crop tool, see https://phabricator.kde.org/D11378#229012.)

Not yet sure what's best, but here are more thoughts: "Navigate away" is not only about pressing the previous/next buttons, but also concerns switching to Browse mode, using another tool like "Red Eye Reduction" (although using tools like rotating while cropping is quite handy) or quitting Gwenview.

I imagine it would get annoying if in any of these cases we would show a warning message. Also, some users might not notice there is a Cancel button they now have to press, so they would be stuck.

Perhaps solution #1 is the best indeed? Or combine #1 and #3? (But once #1 is solved, the #3 workaround is probably not needed anymore.)

I think you are right about #2, which would go more in the direction of batch editing. Nevertheless, #2 is a nice idea, maybe this could be added as an option in Gwenview's settings dialog, because currently for cropping lots of images you have to open the crop tool again and again. Of course for regular users it should default to "off".
Comment 2 Huon 2018-03-20 23:06:16 UTC
(In reply to Henrik Fehlauer from comment #1)
> (This was split off from a discussion while working on the crop tool, see
> https://phabricator.kde.org/D11378#229012.)
> 
> Not yet sure what's best, but here are more thoughts: "Navigate away" is not
> only about pressing the previous/next buttons, but also concerns switching
> to Browse mode, using another tool like "Red Eye Reduction" (although using
> tools like rotating while cropping is quite handy) or quitting Gwenview.
> 
> I imagine it would get annoying if in any of these cases we would show a
> warning message. Also, some users might not notice there is a Cancel button
> they now have to press, so they would be stuck.
> 
> Perhaps solution #1 is the best indeed? Or combine #1 and #3? (But once #1
> is solved, the #3 workaround is probably not needed anymore.)
> 
> I think you are right about #2, which would go more in the direction of
> batch editing. Nevertheless, #2 is a nice idea, maybe this could be added as
> an option in Gwenview's settings dialog, because currently for cropping lots
> of images you have to open the crop tool again and again. Of course for
> regular users it should default to "off".

#1 is an easy fix.
#2 would require implementing batch editing support (currently Gwenview can only operate on one URL at a time). 
#3 as you say, is less necessary if we go with #1.

I would prefer going with just #1 now, then considering #2. #3 could annoy some users?
Comment 3 null 2018-03-20 23:14:17 UTC
> currently Gwenview can only operate on one URL at a time)
Um, I did not mean it in such a complicated way ;) I was just thinking about cropping one image at a time, and instead of having to reopen the crop tool again it would simply stay open. Anyway, as we'll be able to remember the last used crop setting after your patch, adding this is perhaps not strictly necessary anymore…

> I would prefer going with just #1 now
Okay, makes sense to me.
Comment 4 Huon 2018-03-20 23:24:13 UTC
(In reply to Henrik Fehlauer from comment #3)
> > currently Gwenview can only operate on one URL at a time)
> Um, I did not mean it in such a complicated way ;) I was just thinking about
> cropping one image at a time, and instead of having to reopen the crop tool
> again it would simply stay open. Anyway, as we'll be able to remember the
> last used crop setting after your patch, adding this is perhaps not strictly
> necessary anymore…

Reading back, indeed it doesn't necessitate batch editing.

Thinking about this more, #2 will at least require #1 to be implemented, because switching images destroys the image view and any active tool along with it. That also means we'd have to save all config, not just the ratio.
Comment 5 Huon 2018-03-21 01:30:39 UTC
Git commit 312ab20ca12f393e69032bc426942759401f2616 by Huon Imberger.
Committed on 21/03/2018 at 01:30.
Pushed by huoni into branch 'Applications/18.04'.

Fix crop tool not saving config when navigating away

Summary:
Tool settings are saved in `toolDeactivated`, which is only triggered
when the tool is exited gracefully (e.g. when clicking Crop or Cancel).
This patch calls `toolDeactivated` when the parent `RasterImageView`
object is destroyed, ensuring settings are saved even when navigating
away, e.g. to another image, or back to {nav Browse}.

Test Plan:
{nav Crop} tool > activate/deactivate {nav Advanced} > navigate to next
image > open {nav Crop} tool > ensure the {nav Advanced} state is saved.
Also navigate away by clicking {nav Browse}.
Ensure navigating without {nav Crop} active is unaffected.

Reviewers: #gwenview, rkflx

Reviewed By: #gwenview, rkflx

Differential Revision: https://phabricator.kde.org/D11535

M  +3    -0    lib/documentview/rasterimageview.cpp

https://commits.kde.org/gwenview/312ab20ca12f393e69032bc426942759401f2616