[MU4] fix #295257: Page-Width shortcut with previous zoom-level toggle #5564

worldwideweary · 2019-12-28T01:30:21Z

Feature Request: #295257 - Implement Page-Width Shortcut.

This provides the user a shortcut entitled: "Zoom to page-width / previous magnification level" which serves mainly as Page-Width shortcut, but with the additional feature to toggle between it and any previous zoom level. A simple screen-cast is given at https://musescore.org/en/node/298839

There's also a desire shown for making Page-Width the default zoom level, and I also support it. I figured if I do that { if no one beats me to it >:o }, I'll make a separate pull request rather than have it also be part of this as a shortcut. One thing at a time. It's been a while since I've done GIT requests, so excuse the mis-match of the # or whatever else.

Harmoniker1 · 2019-12-28T03:08:05Z

mscore/musescore.cpp

+                // instead of implementing it as a member variable since there's
+                // (currently) no other use for it.
+                MagIdx current_idx = MagIdx(mag->currentIndex());
+                static MagIdx previous_idx = current_idx;


Please always use 6 spaces for indentation.

Harmoniker1 · 2019-12-28T03:10:51Z

mscore/musescore.cpp

+                    mag->setMagIdx(MagIdx::MAG_PAGE_WIDTH);
+                    // Update the actual score's zoom-level
+                    mag->magChanged(MagIdx::MAG_PAGE_WIDTH);
+                }


The right brace should also be indented.

As a base rule, place the left curly brace on the same line as the start of the statement and indent the closing one. This is know as Banner style indentation.

https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/musescore-coding-rules

Harmoniker1 · 2019-12-28T03:11:13Z

mscore/shortcut.cpp

+    {
+        MsWidget::MAIN_WINDOW,
+        STATE_NORMAL | STATE_NOTE_ENTRY | STATE_EDIT | STATE_PLAY,
+        "zoom-page-width",
+        QT_TRANSLATE_NOOP("action","Zoom to page-width / previous magnification level"),
+        QT_TRANSLATE_NOOP("action","Zoom to page-width / previous magnification level")
+    },


Wrong indentation

worldwideweary · 2019-12-28T04:04:06Z

Howard-C, thanks for editing the code and reinforcing the guidelines.
Will be sure to abide in future pull requesting.

Harmoniker1 · 2019-12-28T04:48:04Z

I didn't "edit" the code, you will have to do that yourself :)

worldwideweary · 2019-12-28T04:52:10Z

Ah, shucks.

worldwideweary · 2019-12-28T06:19:46Z

Boy, this is weird. I'm used to the style found in The C Programming Language written by K&R and the founder of C++ B.Stroustrup's style found in his C++ books. Will try to get the hang of these specific guidelines.

Harmoniker1 · 2019-12-28T06:24:31Z

It didn't get any better. You added 2 more commits but there's barely any obvious change.
You can change the indentation rule of your IDE to always use 6 spaces.
And after you're done please squash the commits into one using git rebase.

Harmoniker1 · 2019-12-28T06:28:08Z

mscore/shortcut.cpp

+      {
+         MsWidget::MAIN_WINDOW,
+       STATE_NORMAL | STATE_NOTE_ENTRY | STATE_EDIT | STATE_PLAY,
+       "zoom100",
+       QT_TRANSLATE_NOOP("action","Zoom to 100%"),
+       QT_TRANSLATE_NOOP("action","Zoom to 100%")
+       },


The indentation here is wrong too, please check more carefully.

Harmoniker1 · 2019-12-28T06:28:37Z

What happened?

worldwideweary · 2019-12-28T06:42:03Z

Not sure. I'm still not experienced with git on the commandline. On my machine right now, I have the cpp files correctly showing with appropriate indentation et cetera within Qt-Creator's ide, but a few attempts to commit weren't working. I just tried deleting my online git branch and starting over again to see if it would work, and it's still showing slightly wrong. That's probably not much help and figuring out what I'm doing wrong...Maybe tabs ought to be converted to spaces or something. Sorry if this is confusing.

Harmoniker1 · 2019-12-28T06:47:10Z

Maybe https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/git-workflow can help?

Harmoniker1 · 2019-12-28T06:48:06Z

I don't see tabs here, there're just spaces. But not enough spaces.

worldwideweary · 2019-12-28T06:52:39Z

Hrm. I just edited my Qt-creator settings to use only spaces this time @ 6 per tab (spaces only), and now on my branch it looks correct on a web-browser. Did it update automatically on this pull-request also? (I'm uneducated as to the process of correlation), or do I need to do something else? Thanks for paying attention btw.

Harmoniker1 · 2019-12-28T06:53:50Z

If the branch name is still the same, I think reopen this PR can update it.

Harmoniker1 · 2019-12-28T06:55:43Z

mscore/musescore.cpp

@@ -6018,6 +6018,33 @@ void MuseScore::cmd(QAction* a, const QString& cmd)
                  cv->setMag(MagIdx::MAG_100, 1.0);
            setMag(1.0);
            }
+      // 2019.12.25: [Page-Width] as shortcut command


The date is definitely not needed. But I assume this entire comment is not needed too.

Ironically, I think I added that comment to verify that there was a difference at first when trying to get this thing working on here, and now I have to remove it. Haha.

Harmoniker1 · 2019-12-28T06:57:36Z

Good job with the indentation! There's still a problem as said above, and after you're done you can resolve all the conversations and squash your commits!

Harmoniker1 · 2019-12-28T07:02:32Z

Well, you don't have to close the PR every time you want to update it. Just push/force-push and it will update itself.

worldwideweary · 2019-12-28T07:06:17Z

Told ya I was a noob :)
It seemed though that last time I did that, I used the push command and it didn't work as expected. Maybe it was because I didn't use the force flag?

Harmoniker1 · 2019-12-28T07:08:18Z

Doesn't seem like that, because you're simply adding commits on top of previous ones. If you have different commits between your origin and local branches, you have to use force-push to overwrite the origin branch. So it's necessary when you squashed the commits into one.

worldwideweary · 2019-12-28T07:08:25Z

How may I go about squashing the commits?

worldwideweary · 2019-12-28T07:10:10Z

Ah you already mentioned above rebase

Harmoniker1 · 2019-12-28T07:11:23Z

Yes, use git rebase -i HEAD~6, and change all pick's (except the first one) to s or squash, then close the window (or if you're using vim as text editor, press Esc and type ZZ), then update the commit message, then force-push.

worldwideweary · 2019-12-28T07:29:59Z

Please let me know if it looks okay, and if you've the time, what is the flag head~6? Like, if I had've used "git rebase -i master", what would have been the difference?

Harmoniker1 · 2019-12-28T07:33:02Z

HEAD~6 means you're only rebasing the latest 6 commits, which was the case for you.

worldwideweary · 2019-12-28T07:35:47Z

Thanks for your assistance

dmitrio95 · 2019-12-30T07:49:31Z

mscore/musescore.cpp

+                  // Store current zoom-level statically here for later retrieval
+                  // instead of implementing it as a member variable since there's
+                  // (currently) no other use for it.
+                  MagIdx current_idx = MagIdx(mag->currentIndex());


We use camel case for variables naming across the code so this variable should rather be named currentIdx instead, see the handbook.

dmitrio95 · 2019-12-30T07:52:03Z

mscore/musescore.cpp

+                  // instead of implementing it as a member variable since there's
+                  // (currently) no other use for it.
+                  MagIdx current_idx = MagIdx(mag->currentIndex());
+                  static MagIdx previous_idx = current_idx;


previous_idx (or rather previousIdx, as per comment above) should probably logically belong to the score view rather than to MuseScore::cmd() function so it should probably be made a member of ScoreView class. The current implementation would probably cause inconsistent behavior if this shortcut is invoked in several score tabs.

After testing three tabs with this shortcut, there doesn't seem to be an issue with variations of this shortcut. But I slightly understand why you say the variable should be stored elsewhere. Since nothing else is using this information, figured it would be okay, but i'll make adjustments + the camel case if this is what's decided.

Will update the pull-request later today for review.

After setting up the code to be related to handbook + moving the variable to scoreview, I have found there to be a slight loop-hole which has nothing to do with this implementation itself but the way MuseScore works generally: ScoreView doesn't really store the custom magnification level at all, so that a "free mag" doesn't get attached to a score view.

For example, let there be four scores open, each set to 150% zoom. Now change one to a custom zoom level like 155%. All of the others will show 155% in the free magnify position of the dropdown box (bottom most position). If you switch over to another score, and change the free magnification position to something like 177%, and then check the others, they now will show 177% on their custom bottom-most slot. This behavior gets exemplified with using this shortcut's toggle ability, but the shortcut itself is not to blame. I think this itself can be fixed if someone implemented a storing per ScoreView free_mag zoom level, and used it appropriately to be reloaded back into the MagBox, but that's just a guess. If the user sticks with only the standard zoom levels in the drop-down list for example, there's never any mix-up of zoom toggling that occurs are far as my testing shows. And yet I've never once heard anyone complain about this behavior understandably.

dmitrio95 · 2019-12-30T07:52:32Z

mscore/shortcut.cpp

+      {
+         MsWidget::MAIN_WINDOW,
+         STATE_NORMAL | STATE_NOTE_ENTRY | STATE_EDIT | STATE_PLAY,
+         "zoom100",
+         QT_TRANSLATE_NOOP("action","Zoom to 100%"),
+         QT_TRANSLATE_NOOP("action","Zoom to 100%")
+         },


This block should probably be removed, zoom100 shortcut is already defined a few lines above.

Total accident.

worldwideweary · 2020-01-10T01:31:55Z

mscore/shortcut.cpp

+         MsWidget::MAIN_WINDOW,
+         STATE_NORMAL | STATE_NOTE_ENTRY | STATE_EDIT | STATE_PLAY,
+         "zoom-page-width",
+         QT_TRANSLATE_NOOP("action","Zoom to page-width / previous magnification level"),


Oh, it looks like this needs to be fixed also: maybe merely "Zoom to Page-Width" would work to keep it shorter as with most others and give capitalization.

anatoly-os · 2020-06-06T16:10:23Z

I manually merged the changes from this PR to 3.x. Could you please rebase this PR to the latest master?

…oggle Manually merged #5564 to 3.x

…oggle

Jojo-Schmitz · 2021-04-24T11:23:18Z

a rebase of this one to master is still needed

Jojo-Schmitz · 2021-04-27T09:00:05Z

Or rather to 3.x, as those changes don't apply to master anymore at all, it seems?

vpereverzev · 2021-05-05T09:32:28Z

No more applicable for master

Harmoniker1 reviewed Dec 28, 2019

View reviewed changes

worldwideweary closed this Dec 28, 2019

worldwideweary deleted the 295257-pagewidthshortcut branch December 28, 2019 06:27

Harmoniker1 reviewed Dec 28, 2019

View reviewed changes

worldwideweary reopened this Dec 28, 2019

Harmoniker1 reviewed Dec 28, 2019

View reviewed changes

worldwideweary closed this Dec 28, 2019

worldwideweary reopened this Dec 28, 2019

worldwideweary force-pushed the 295257-pagewidthshortcut branch from 67ec7c1 to 597d0ea Compare December 28, 2019 07:26

dmitrio95 reviewed Dec 30, 2019

View reviewed changes

worldwideweary force-pushed the 295257-pagewidthshortcut branch 3 times, most recently from 61dfb63 to a27c9cb Compare December 30, 2019 11:46

worldwideweary commented Jan 10, 2020

View reviewed changes

worldwideweary changed the title ~~fix #295257: Implement page-width shortcut with previous zoom-level t…~~ fix #295257: Page-Width shortcut with previous zoom-level toggle Jan 19, 2020

worldwideweary force-pushed the 295257-pagewidthshortcut branch from a27c9cb to b61db4d Compare January 19, 2020 00:57

dmitrio95 added the strings Affects translatable strings label Jan 21, 2020

worldwideweary mentioned this pull request Apr 29, 2020

MuseScore 3.5 Alpha PRs to be merged #5960

Closed

anatoly-os mentioned this pull request May 3, 2020

MuseScore 3.5 Beta. PRs to be merged #6027

Closed

12 tasks

anatoly-os pushed a commit that referenced this pull request Jun 6, 2020

fix #295257: Implement page-width shortcut with previous zoom-level t…

a29de17

…oggle Manually merged #5564 to 3.x

fix #295257: Implement page-width shortcut with previous zoom-level t…

2afee2e

…oggle

worldwideweary force-pushed the 295257-pagewidthshortcut branch from b61db4d to 2afee2e Compare June 7, 2020 02:55

Spire42 mentioned this pull request Jun 23, 2020

Fix #306598: Canvas: impossible to choose a custom zoom default, difficult to override in toolbar #6213

Merged

8 tasks

igorkorsukov changed the title ~~fix #295257: Page-Width shortcut with previous zoom-level toggle~~ [MU4] fix #295257: Page-Width shortcut with previous zoom-level toggle Feb 5, 2021

vpereverzev closed this May 5, 2021

[MU4] fix #295257: Page-Width shortcut with previous zoom-level toggle #5564

[MU4] fix #295257: Page-Width shortcut with previous zoom-level toggle #5564

Conversation

worldwideweary commented Dec 28, 2019 • edited Loading

Choose a reason for hiding this comment

Harmoniker1 Dec 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worldwideweary commented Dec 28, 2019 • edited Loading

Harmoniker1 commented Dec 28, 2019

worldwideweary commented Dec 28, 2019

worldwideweary commented Dec 28, 2019 • edited Loading

Harmoniker1 commented Dec 28, 2019 • edited Loading

Choose a reason for hiding this comment

Harmoniker1 commented Dec 28, 2019

worldwideweary commented Dec 28, 2019 • edited Loading

Harmoniker1 commented Dec 28, 2019

Harmoniker1 commented Dec 28, 2019

worldwideweary commented Dec 28, 2019

Harmoniker1 commented Dec 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harmoniker1 commented Dec 28, 2019

Harmoniker1 commented Dec 28, 2019

worldwideweary commented Dec 28, 2019 • edited Loading

Harmoniker1 commented Dec 28, 2019

worldwideweary commented Dec 28, 2019

worldwideweary commented Dec 28, 2019

Harmoniker1 commented Dec 28, 2019 • edited Loading

worldwideweary commented Dec 28, 2019

Harmoniker1 commented Dec 28, 2019 • edited Loading

worldwideweary commented Dec 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worldwideweary Dec 30, 2019 • edited Loading

Choose a reason for hiding this comment

worldwideweary Dec 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worldwideweary Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

anatoly-os commented Jun 6, 2020

Jojo-Schmitz commented Apr 24, 2021

Jojo-Schmitz commented Apr 27, 2021

vpereverzev commented May 5, 2021

worldwideweary commented Dec 28, 2019 •

edited

Loading

Harmoniker1 Dec 28, 2019 •

edited

Loading

worldwideweary commented Dec 28, 2019 •

edited

Loading

worldwideweary commented Dec 28, 2019 •

edited

Loading

Harmoniker1 commented Dec 28, 2019 •

edited

Loading

worldwideweary commented Dec 28, 2019 •

edited

Loading

worldwideweary commented Dec 28, 2019 •

edited

Loading

Harmoniker1 commented Dec 28, 2019 •

edited

Loading

Harmoniker1 commented Dec 28, 2019 •

edited

Loading

worldwideweary Dec 30, 2019 •

edited

Loading

worldwideweary Dec 30, 2019 •

edited

Loading

worldwideweary Jan 10, 2020 •

edited

Loading