Hide fullscreen thumbnailbar when on-canvas image tools are in use
ClosedPublic

Authored by rkflx on Feb 26 2018, 10:42 PM.

Details

Summary

In fullscreen mode the thumbnailbar will slide in once the cursor moves
near the top. As this covers parts of the image, it makes the upper
handles of the Crop tool inaccessible and prevents the user from
applying the Red Eye Reduction tool on every part of the image.

setDistractionFreeMode already provides the necessary wiring which was
supposed to prevent the hot edge from triggering, however it only
disabled the auto-hiding. This is fixed by adding a function and a
variable which disables auto-showing.

Test Plan

Open image in fullscreen View mode, press +C to trigger
Crop and move cursor to the top. The thumbnailbar does not
reappear again. Stop the tool, now triggering should resume.

Switching to fullscreen should show the thumbnailbar initially without
the cursor triggering, as before.

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.
rkflx requested review of this revision.Feb 26 2018, 10:42 PM
rkflx created this revision.

Feels good to actually submit a patch once in a while…

@huoni Thanks for the report, now you are the one who's tasked with reviewing :D

Feels good to actually submit a patch once in a while…

@huoni Thanks for the report, now you are the one who's tasked with reviewing :D

Well this is exciting!

It's behaving perfectly for me (I really tried to break it!). Just one minor quibble (see inline comment)

lib/fullscreenbar.cpp
274

I think this should be setEdgeTriggerEnabled for two reasons:

  1. It accepts a bool, therefore doesn't always enable it. enableEdgeTrigger(variable) could be confusing.
  2. Using setThing for setters and just thing for getters seems to be the convention (would also match setAutoHidingEnabled).
rkflx updated this revision to Diff 28156.Feb 27 2018, 12:07 AM
  • Rename to setEdgeTriggerEnabled
rkflx marked an inline comment as done.Feb 27 2018, 12:09 AM

Thanks for the comments. Of course I did not upload all the revisions which behaved oddly ;)

lib/fullscreenbar.cpp
274

Indeed, I wondered for quite a while how to name that thing. Let's go with your suggestion.

huoni accepted this revision.Feb 27 2018, 12:15 AM

Of course I did not upload all the revisions which behaved oddly ;)

That's a nifty trick! I should try it one day :P

This looks good now!


Vaguely related question - what's your review workflow like? I used arc patch to test the patch, but didn't see an easy way to get updated changes (running it again creates a new local branch). Is there a better way?

This revision is now accepted and ready to land.Feb 27 2018, 12:15 AM
rkflx marked an inline comment as done.EditedFeb 27 2018, 12:28 AM

Cool. I'll leave it open until tomorrow, just in case someone still finds a problem.

Vaguely related question - what's your review workflow like? I used arc patch to test the patch, but didn't see an easy way to get updated changes (running it again creates a new local branch). Is there a better way?

Just ask away!

I use exactly the same workflow. Unfortunately after three branches of the same Diff it will complain, so you have to clean up a bit.

An option would be to use arc patch for the first iteration (as it will fetch and set the base revision as chosen by the author), and arc patch --nobranch for subsequent updates. See also https://secure.phabricator.com/T3277.

Thinking about this, https://community.kde.org/Infrastructure/Phabricator does not have anything on arc patch. Why not add a section? It's a wiki…

Thinking about his, https://community.kde.org/Infrastructure/Phabricator does not have anything on arc patch. Why not add a section? It's a wiki…

Writing such a section is on my to-do list, but I certainly wouldn't mind if you beat me to it... ;)

huoni added a comment.Feb 27 2018, 3:25 AM

Thinking about his, https://community.kde.org/Infrastructure/Phabricator does not have anything on arc patch. Why not add a section? It's a wiki…

Writing such a section is on my to-do list, but I certainly wouldn't mind if you beat me to it... ;)

I'm better with code than words, and I'm not sure I'm qualified to write a guide when I don't know how to use it properly myself :)

Perhaps down the road once I've used Arcanist a bit more I'll consider adding to the wiki.

This revision was automatically updated to reflect the committed changes.