Center zoom to cursor position for zoom In/Out, Fill and 100% actions
ClosedPublic

Authored by muhlenpfordt on Jul 17 2018, 9:27 AM.

Details

Summary

Zooming by mouse click/wheel and shortcut toggling from Fit/
Fill to 100% already centers the zoom to the current
cursor position.
This patch also centers the zoom on the cursor position for:

  • triggering Fill zoom ( middle click/+F)
  • shortcut zooming in/out (Ctrl++/Ctrl+-)
  • shortcut zooming to 100% (if Actual Size shortcut set)
Test Plan
  • Try different zoom actions by mouse/shortcut
  • Check if zoom centers to current cursor position (inside image view)
  • Check compare mode

Diff Detail

Repository
R260 Gwenview
Branch
zoom-center (branched from Applications/18.08)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 942
Build 955: arc lint + arc unit
muhlenpfordt requested review of this revision.Jul 17 2018, 9:27 AM
muhlenpfordt created this revision.

Zooming in/out in synchronized compare mode looks a bit strange sometimes. But this did not change by this patch.

lib/documentview/documentview.cpp
667–669

If zoom in/out is triggered by a mouse press event in AbstractImageView we can use the given position and don't need to recalculate it.

rkflx accepted this revision.Jul 17 2018, 11:52 PM
rkflx added a subscriber: rkflx.

Awesome, really great to see how focussing on a particular area often improves the user experience beyond the fix for the original bug report.

The only thing I could come up with is a minor comment regarding style, otherwise LGTM.

lib/documentview/documentview.cpp
667–669

Good call. Should I also add this to toggleZoomToFill then, which can also be triggered by a shortcut or by a click?

684–686

The ternary operator is great for choosing between two options based on a condition, e.g. shiftPressed ? fill() : fit(). However here you are trying to update a value (which is also part of the condition) if it is not valid yet, and which in theory can also be used in other parts of the function.

IMO in that case it makes sense to write an explicit if right at the top of the function. Or is this just my personal preference? Commit as you wish ;)

This revision is now accepted and ready to land.Jul 17 2018, 11:52 PM
muhlenpfordt added inline comments.Jul 18 2018, 7:32 AM
lib/documentview/documentview.cpp
667–669

Sure, I think using the mouse event position is more reliable and definitely faster.

684–686

What do you mean with "update a value"? It's just a switch for the second argument of setZoom().
Is this your suggestion?

void DocumentView::zoomOut(QPointF center)
{
    if (center == QPointF(-1, -1)) {
        center = d->cursorPosition();
    }
rkflx added inline comments.Jul 18 2018, 10:49 PM
lib/documentview/documentview.cpp
684–686

It was just nitpicking in search for the safest, most readable and easiest to understand code. I commented because to me it seemed a bit strange to write inline what is essentially code for replacing the value of center with something more valid. Don't overthink this, just commit what you prefer ;)

Is this your suggestion?

Yes, that's what I was going for.

Increase readability of zoomIn/zoomOut functions

This revision was automatically updated to reflect the committed changes.

Was that ok how I pushed this to master?
There are two different GIT_SILENT ... commits in Applications/18.08 and master, which I got both after using git merge -Xours origin/Applications/18.08.
Now I used git cherry-pick <commit> to get only this commit.
Do we have to use this from now on while 18.08 lives?

Was that ok how I pushed this to master?

In general (or at least over here ;) merging stable into master is preferred over cherry-picking (so there is only a single commit sha per change, instead of a unique sha for each branch the change is in). Not the end of the world, but worth figuring out why it did not work for you.

There are two different GIT_SILENT ... commits in Applications/18.08 and master, which I got both after using git merge -Xours origin/Applications/18.08.

Could you specify which commits you mean exactly? Those adding translations, or those changing the version strings? The former are added by Scripty to both branches, and should merge just fine since they should not contain conflicts. The latter are obviously touching the same line in CMakeLists.txt and are in conflict, but that should be taken care of by specifying -Xours.

I now recreated your setup and tried a merge (did not push, of course):

git checkout Applications/18.08
git reset --hard 635da70e8f640179c4e79bd5552f41c0d6c0f3ac
git checkout master
git reset --hard 467d65859badc39f50a52771f28dc9e99ccbf6d7
git merge -Xours Applications/18.08

…which looks fine to me.

Could you detail what you mean with "I got both"? I'm not really sure what you did!?

Do we have to use this from now on while 18.08 lives?

Hopefully not ;)

First I landed to stable as usual:

$ git checkout my-branch
$ arc land --hold --onto Applications/18.08
$ git push -- origin ...:Applications/18.08

Then tried to merge into master and got something like this:

$ git checkout master
$ git merge -Xours origin/Applications/18.08
$ git log
commit 2a375e0165ae1d666c2e62f4c22b71b7c53574f7 (HEAD -> master)
Merge: 8cbc6377 e6026082
Author: Peter Mühlenpfordt <devel@ukn8.de>
Date:   Thu Jul 19 13:02:23 2018 +0200
    Merge remote-tracking branch 'origin/Applications/18.08'

commit e60260826f77de8bdd3a1fd0465f5f32b230f2dd (origin/Applications/18.08, Applications/18.08)
Author: Peter Mühlenpfordt <devel@ukn8.de>
Date:   Tue Jul 17 11:15:48 2018 +0200
    Center zoom to cursor position for zoom In/Out, Fill and 100% actions

commit 467d65859badc39f50a52771f28dc9e99ccbf6d7
Author: l10n daemon script <scripty@kde.org>
Date:   Wed Jul 18 06:13:23 2018 +0200
    GIT_SILENT made messages (after extraction)

commit 635da70e8f640179c4e79bd5552f41c0d6c0f3ac
Author: l10n daemon script <scripty@kde.org>
Date:   Wed Jul 18 03:35:19 2018 +0200
    GIT_SILENT made messages (after extraction)

Commit 467d65859bad is from stable and 635da70e8f64 from master but they are identical.
There were no conflicts, just this duplicated commit. But I thought 467d65859bad should not appear in master.
Maybe I should have just ignored that?

But I thought 467d65859bad should not appear in master.

Why not? That's how merging works: Every commit from stable will (and should) be present in master, and Git will figure out (depending on the options given) how to handle merging the actual code changes.

For example look at https://phabricator.kde.org/source/spectacle/graph/master/ and search for 9c692efb542f. You'll notice it was committed on the stable branch and later merged to master. Now look at https://phabricator.kde.org/source/spectacle/history/master/, where you'll find it appearing in the history of master right along the other commits. Also look at https://phabricator.kde.org/R166:55dfeb60768c28bffdc5e2ed23703a63d06d39ba, which lists all commits included in the merge.

(Have we talked about this topic before? At least for me, visualizing with gitk --all often clears up confusion.)

Maybe I should have just ignored that?

Probably. In general you should be fine to git push if your local setup is up-to-date with origin, i.e. you recently fetched or pulled all relevant branches.

That's how merging works: Every commit from stable will (and should) be present in master

Yes, that's why I was confused that 467d65859bad was not merged but 'copied' instead.
Thanks for the explanations.