ISSUE-50: Add zoom in/out shortcuts by danailbd · Pull Request #315 · powertab/powertabeditor

danailbd · 2020-11-28T18:02:02Z

Description of Change(s)

Introduces keyboard shortcuts for zooming in/out the score. It mainly reuses the existing zoom change mechanics. The default shortcuts are as follows:

MacOS

Zoom in - Cmd+=
Zoom out - Cmd+-

Widndows

Zoom in - Ctrl+=
Zoom out - Ctrl+-

Fixes Issue(s)

partly fixes issue 50 - TODO add link when PR is ready

TODO

Add tests?

cameronwhite · 2020-11-28T20:06:35Z

Great to see this being worked on! Feel free to let me know when I should take a look over the changes, or if you have any questions about how to approach things

danailbd

Hey @cameronwhite thanks for the heads up! : )

I think I have the basic idea in place. I've done probably the most basic approach for achieving these zoom in/out shortcuts (which do seems relatively small as a change). I haven't added wheel zoom though.

Overall, I don't really enjoy enlarging the already significant powertabeditor class. I guess the zoom logic deserves a dedicated class, especially if the wheel zoom is added. On the other hand I didn't what to overengeneer this simple functionality.

I'd be happy to hear your thoughts on the current changes. I've added a couple of questions

danailbd · 2020-11-29T16:32:14Z

source/app/powertabeditor.cpp

+
+void PowerTabEditor::zoomOutScore()
+{
+    const int ZOOM_CHANGE_COEFITIENT = 25;


I don't have a good idea where this constant should live :/ I don't want to pollute the substantial powertabeditor class.
This constant is related to the "Zoom in shortcuts", something that we don't have a dedicated place for...

I think I'd probably put that inside the ViewOptions class, maybe with just an increaseZoom / decreaseZoom method? If the predefined zoom levels (currently stored in playbackwidget.ui for populating the combobox widget) were instead stored in ViewOptions, the increase / decrease methods could also step through the predefined zoom levels rather than a fixed percentage

danailbd · 2020-11-29T16:32:34Z

source/app/viewoptions.cpp

 #include "viewoptions.h"

+static constexpr double MIN_ZOOM = 25;
+static constexpr double MAX_ZOOM = 300;


I feel these should live in a more general place so that the playback widget could also appreciate them. Any suggestions?

I think these could be fine as static constants on the ViewOptions class, and then the UI widgets can refer to those constants as needed.

cameronwhite · 2020-11-30T03:27:29Z

I agree on not adding further bloat to the main PowerTabEditor class 👍 . I think it's fine to put most of the additional logic or constants in the ViewOptions class

danailbd · 2021-02-13T14:08:07Z

source/app/viewoptions.h

+    /// Increases to the next possible zoom level.
+    bool increaseZoom();
+    /// Increases to the previous possible zoom level.
+    bool decreaseZoom();


I decided to make them Booleans in case we decide to add an "error" sound upon hitting a bound

danailbd · 2021-02-13T14:12:10Z

source/app/viewoptions.h

+public:
+    static constexpr double MIN_ZOOM = 25;
+    static constexpr double MAX_ZOOM = 300;
+    static constexpr std::array<unsigned short, 14> ZOOM_LEVELS = { 25, 50, 70, 80, 90, 100, 110, 125, 150, 175, 200, 225, 250, 300 };


Wanted these to be scoped. Wasn't sure whether to use namespace or the class though

danailbd · 2021-02-13T14:33:49Z

@cameronwhite I moved as much logic to ViewOptions as I could, but the PowerTabEditor class still has to serve as the mediator that applies the updated zoom - at least I assumed it should as the the PowerTabEditor holds the Score.

One side note - how about adding setting for the default "zoom level"? That would save me a few actions when opening a document. I suppose there are others having a similar feeling for this behaviour

cameronwhite · 2021-02-13T15:38:33Z

That looks good! Having the zoom constants in class scope is fine

Adding a preference to remember the last zoom level sounds reasonable!

danailbd

Hey @cameronwhite, here's a suggestion on the saving the zoom level as a setting. Overall, I'm keeping the last "set" zoom level (LastZoomLevel) and apply it to all newly opened documents (either preexisting or fresh new).
Another way would be to have it configurable in the preferences.

danailbd · 2021-02-14T10:03:28Z

source/app/documentmanager.cpp

    myCurrentIndex = static_cast<int>(myDocumentList.size()) - 1;
+
+    auto settings = settings_manager.getReadHandle();
+    myDocumentList.back()->getViewOptions().setZoom(settings->get(Settings::LastZoomLevel));


I'm not so sure about applying the setting here - for a "default" document we should be ok. On the other hand, at some point in the future we might find it useful to serialise the zoom level with a file...

At this point, at least, I'm thinking it probably won't be likely that we want to save the zoom level with a file, since that's specific to a user's machine / monitor?
More likely might be saving per-document zoom settings locally, but I think the current behaviour is good for now.

Yep, I misused the idea of serialising. I was thinking precisely for a per-document zoom setting locally

danailbd · 2021-02-14T10:07:43Z

source/app/scorearea.cpp

    }

    myScene.addItem(myCaretPainter);
+    refreshZoom();


We want the document's zoom applied. Most likely there's a better way to integrate it in the rendering itself (in the Caret). I'd prefer to avoid breaking something with this oftenly used functionality.

Is this needed because the document might now have a zoom level other than 100% after it's first created / rendered?

I think that's fine to call here - it just sets the overall transform for the scene, so it could probably even be done at the start of this method.

Yep, the document didn't got rescaled with the initial value. I'll give it a try moving it to the 🔝

danailbd · 2021-02-14T10:09:31Z

source/app/viewoptions.h


-    double getZoom() const { return myZoom; }
-    void setZoom(double percent) { myZoom = percent; }
+    int getZoom() const { return myZoom; }


@cameronwhite was there a particular reason for the zoom to be a double? I believe a short would suit it better. What do you think? (I know I've changed it to int here. A simple way to avoid a settings converter)

I don't think I had any particular reason for double - changing to int would be fine!
I usually avoid lower capacity types like short unless there's a real need for it.

danailbd · 2021-02-14T10:25:10Z

@cameronwhite what do you think about the predefined behaviour of QKeySequence::ZoomIn on Windows? - https://stackoverflow.com/a/40688325
On macOS it works with or without the shift key, which I'd prefer to be the behaviour on Windows as well. On the positive side, the key combination is configurable through the shortcuts, but for a new user the proper zoom in key combination might not be that obvious, nor would be the possibility of changing it. We're so used to using "Ctrl+=" for zooming in...

cameronwhite

Saving the zoom level by default makes the most sense IMO, versus adding a new configurable preference.

For QKeySequence I'd directly specify your own shortcuts, then, if it isn't the expected behaviour (we have ifdef's in a few places for macOS-specific variants of other shortcuts)

cameronwhite · 2021-02-17T22:25:26Z

source/app/documentmanager.cpp

    myCurrentIndex = static_cast<int>(myDocumentList.size()) - 1;
+
+    auto settings = settings_manager.getReadHandle();
+    myDocumentList.back()->getViewOptions().setZoom(settings->get(Settings::LastZoomLevel));


At this point, at least, I'm thinking it probably won't be likely that we want to save the zoom level with a file, since that's specific to a user's machine / monitor?
More likely might be saving per-document zoom settings locally, but I think the current behaviour is good for now.

cameronwhite · 2021-02-17T22:29:01Z

source/app/scorearea.cpp

    }

    myScene.addItem(myCaretPainter);
+    refreshZoom();


Is this needed because the document might now have a zoom level other than 100% after it's first created / rendered?

I think that's fine to call here - it just sets the overall transform for the scene, so it could probably even be done at the start of this method.

cameronwhite · 2021-02-17T22:31:25Z

source/app/viewoptions.h


-    double getZoom() const { return myZoom; }
-    void setZoom(double percent) { myZoom = percent; }
+    int getZoom() const { return myZoom; }


I don't think I had any particular reason for double - changing to int would be fine!
I usually avoid lower capacity types like short unless there's a real need for it.

cameronwhite · 2022-01-02T05:57:48Z

I've merged this in and fixed up the last couple suggestions from the code review
Thanks for your work on this!!

ISSUE-50: Add zoom in/out shortcuts

97fd986

danailbd added 2 commits November 29, 2020 17:44

ISSUE-50: Simple way to avoid zoom update cycle

1c89362

ISSUE-50: Use Qt build in zoom sequences

9bcb7e0

danailbd force-pushed the feature/zoom_shortcuts branch from 312027b to 9bcb7e0 Compare November 29, 2020 15:59

ISSUE-50: Clean up comments

56cd0a3

danailbd commented Nov 29, 2020

View reviewed changes

danailbd and others added 2 commits February 13, 2021 15:56

ISSUE-50: Use prefixed zoom steps

3f35fa2

Merge branch 'master' into feature/zoom_shortcuts

bb6db09

danailbd marked this pull request as ready for review February 13, 2021 14:01

danailbd commented Feb 13, 2021

View reviewed changes

ISSUE-50: Use last set zoom level

958e2a5

danailbd commented Feb 14, 2021

View reviewed changes

cameronwhite reviewed Feb 17, 2021

View reviewed changes

cameronwhite merged commit 03b0ca5 into powertab:master Jan 2, 2022

This was referenced Jan 2, 2022

Adjustable layout #50

Open

More persistent view settings #340

Open

Conversation

danailbd commented Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change(s)

MacOS

Widndows

Fixes Issue(s)

TODO

Uh oh!

cameronwhite commented Nov 28, 2020

Uh oh!

danailbd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameronwhite commented Nov 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danailbd commented Feb 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cameronwhite commented Feb 13, 2021

Uh oh!

danailbd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danailbd commented Feb 14, 2021

Uh oh!

cameronwhite left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameronwhite commented Jan 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danailbd commented Nov 28, 2020 •

edited

Loading

danailbd left a comment •

edited

Loading

danailbd commented Feb 13, 2021 •

edited

Loading