[MU4] fix #68431: Extra Default Zoom Preferences

[worldwideweary]

Resolves, partially: https://musescore.org/en/node/68431 (partially in the sense that there's no saving zoom level to score involved, and the shortcut for page width is another pull request active on my account)

This provides the ability to set default zoom-level to Page Width/Whole Page/Two Pages/Percentage instead of only percentage values. The original spinner box of Zoom % still functions as usual, but it is only enabled when "Percentage" is selected in the included new QComboBox.

Here is a .gif for demonstration purposes:

Updated 2020-05-06 to include adding back the .qrc file tags automatically removed by Qt Designer, defaulted once again to [Page-Width] after realizing the issue with the unit tests, updated that test based on @AntonioBL's assistance, and made use of a simple enumerator rather than using integer constants. Thanks again to all the participants.

[Jojo-Schmitz]

seems the Travis build timed out, doesn't seem to be the fault of this PR

[worldwideweary]

seems the Travis build timed out, doesn't seem to be the fault of this PR

Did an mtest over-night and got a 100% build, so maybe it was a temporary 'hiccup' on the build system... will do another forced pull to see if it builds.

Update: still "stalling".
Any ideas as to why or what can be done since locally it passes the mtest for me? Would like to get default Page Width zoom into the next release >:D

[Jojo-Schmitz]

That last mtest, 60/60 Test #60: tst_runscripts is missing, this does look related, missed that before...

I don't think it would make it into 3.4 anyhow, it adds new strings for translation.

[worldwideweary]

I don't think it would make it into 3.4 anyhow, it adds new strings for translation.

Darn. Makes sense though. Hopefully a little after.

That last mtest, 60/60 Test #60: tst_runscripts is missing, this does look related, missed that before...

Is there anything I can be doing about this, or am I to wait for further instructions, or just expect that to work out somehow? Thanks JjS

P.S. I installed xvfb locally and ran 'run_scripts.sh' and test 60/60: tst_runscripts passed in 6 seconds. I wonder why the online system fails or timed-out while my local machine runs normally?

[Jojo-Schmitz]

rebase needed

[worldwideweary]

Okay, rebased with master code as of today. Hopefully this will pass the Travis build (last time it hung on this PR back earlier this year without really giving strong details).

On to rebasing the other PRs in hopes to get them into 3.5 Beta maybe or soon thereafter if not.

[Jojo-Schmitz]

One advantage of not having a default case is a possible compiler warning about unhandled cases, like when more get added later. Esp useful for switches on enums.

[worldwideweary]

OK, I'm going to test switching the default zoom preference to be set to Percentage @ 100% (rather than page-width) in hopes that it might fix the stalling issue with the Travis build. I doubt it, but it's possible so worth a shot. (Only a small change in the prefsdialogue.ui file).

Update: didn't work, but I forgot to make another alteration besides the value set in the .ui file, viz, within preferences.cpp. Doing a force push update and will see the results tomorrow.

[Jojo-Schmitz]

It's pretty clearly tst_runscripts that stalls. Not clear at all though where any why, @dmitrio95 any idea?

[Jojo-Schmitz]

And now it stalls again. There was one version in between that did not, see https://travis-ci.org/github/musescore/MuseScore/builds/681881318

[AntonioBL]

I compiled locally and it fails on test ux_replace_slurs_on_copy.
At a certain point of the tst, some graphical messages about "Command <> not valid in current state" (the first being "Command prev-chord not valid in current state") start to appear and I need to click "Ok" to one after the other to make them disappear, otherwise they hang the command line. That's probably why Travis is hanging.
Attached you can find a log of the failed test.
scripting_test.log

[dmitrio95]

Test hanging may be a result of missing breaks in switch statement: it makes _magIdx be assigned to MagIdx::MAG_PAGE value which, for some reason, causes "invalid state" messages to appear.

There are also other possible issues with this pull request, I added comments regarding them.

[AntonioBL]

Here are the initial commands of ux_replace_slurs_on_copy.script
It opens (mtest/testscript/scripts/)init/TrebleWithParts.mscx; then inserts 8 notes; then exits note insert mode; then it should go to the first note and select it and the next two, and add a slur

init init/TrebleWithPart.mscx
cmd note-input
cmd note-a
cmd note-b
cmd note-a
cmd note-b
cmd note-c
cmd note-d
cmd note-c
cmd note-d
cmd escape
cmd del-empty-measures
cmd prev-measure
cmd note-input
cmd escape
cmd select-next-chord
cmd select-next-chord
cmd add-slur

This is what usually happens, but after this PR, at the "cmd note-input", which is used to select a note (in principle the first of the score), the note being selected is the last one of the score.
Maybe because the default is "whole page" zoom and the algorithm to retrieve where to start note input selects a different note depending on the zoom level (maybe the note closest to the central point?)
Therefore the slur is added to the last note and the test is not successful.
If the test is modified like this (the initial part):

init init/TrebleWithPart.mscx
cmd note-input
cmd note-a
cmd note-b
cmd note-a
cmd note-b
cmd note-c
cmd note-d
cmd note-c
cmd note-d
cmd escape
cmd del-empty-measures
cmd note-input
cmd escape
cmd prev-measure
cmd prev-measure
cmd note-input
cmd escape
cmd select-next-chord
cmd select-next-chord
cmd add-slur

(note the additional cmd note-input / cmd escape / cmd prev-measure), then it succeeds.

[worldwideweary]

Interesting and nice find. Thanks for this, as it raises an issue for further reflection as to how the testing should be possibly reconfigured.

[Spire42]

I was suggesting that you update the test as part of this PR so that you can switch the default back to “Whole Page” at the same time.

I'd rather prefer "Page Width" as the default

Whoops, I did mean “Page Width”.

[AntonioBL]

@worldwideweary : if it can be of help, here is a patch which should revert all those resource="musescore.qrc" being deleted, as well as the changes <width>841</width> and <number>3</number>
(I did it by using find and replace with Notepad++)
You can apply it by using:
git apply <path to folder>/5623.txt
and then git add mscore/prefsdialog.ui
git commit --amend

I like these new zoom options and I hope they will become part of MuseScore.
I found that in the dropdown menu you see above the score there is an additional 100% after all the items (i.e. you see 25% ; 50% ... 1600% ; Page Width ; Whole Page ; Two Pages ; 100%)
Ciao,
ABL

5623.txt

[worldwideweary]

Ciao,
ABL
5623.txt

Thanks for the effort and posting. I figured that'd be the smart thing to do until proven beyond doubt that those changes omitting the musescore.qrc tag really don't hurt anything (or like @Jojo-Schmitz mentioned use a gui based Git application that makes it easy to omit changes line by line... apparently there's a 3rd part linux port to GitHub Desktop).

If I can get this applied + update the testing file to be in conformity to the above post earlier, and then change back to page width as default and verify that it works cleanly and passes the online tests... then it will be mergable :) And then there's the extra work to be done to use enumerations, but I don't believe that to be absolutely necessary for merging, but still is worth doing afterwards on spare time to achieve a more readable code.

P.S. That bottom most value is supposed to be the "Free Magnification" value after doing scroll-zooming or what-have-you. I'd imagine it shouldn't be a problem if it's initialized to be 100% in the beginning stage of a score either.

[worldwideweary]

@AntonioBL: Aside Observation about the script testing. Back in MS 3.0-3.1 time frame, Note Entry still had some kinks to be worked out, especially having to do with previously selected elements. For example, I remember that if you deleted a measure where the result was that nothing was selected afterwards, going into Note Entry mode immediately after that would default to the beginning of the system or even the beginning of the score, or if you had a slur selected then pressed "N" something funny would happen. I seem to recall around 3.2-3 was when many of those issues were fixed up so that Note Entry became more appropriate in placing entry next to the last selected element.

The irony here is I'm thinking it's quite possible that this PR might have passed the script testing as it originally was if it was checked back in initial 3.0 because the Note Entry might have started at the beginning of the system no matter what zoom level based on that. In that sense, the prev-measure commands would almost be superfluous with the way Note Entry worked. Since Note Entry was fixed up, it ends up selecting the most recent element so long as the zoom/view has it in sight rather than defaulting to beginning no matter what! This is just a guess, but I found this to be slightly amusing.

[worldwideweary]

• Got an enum (a simple one instead of making use of _magIdx for now), reverted to Page-Width as default after adding the changes provided to the above mentioned script (thanks) + also patched .ui. One must be very careful with .ui files! After patching, I attempted to merely open the file and read the preferences of the QComboBox to make sure the strings were marked "Translatable" and Qt Designer ended up performing the same changes again, haha.

[Spire42]

Glad to see the latest changes. Just one more small suggestion.

[Spire42]

What is this change for?

[worldwideweary]

Maybe you should check the Qt Manual to find documentation about the <> tags if you can find them regarding the Designer files?

[worldwideweary]

If you figure out why Qt added the line and think that it was not appropriate, let me know if you or anyone else think I should manually delete this. Thanks.

In the meantime, I'm manually eradicating the line. Although, in a sense maybe it isn't a bad idea to have a container for a RadioButton "Group box".

[Spire42]

As it happens, I created this Ms::RadioButtonGroupBox control less than two weeks ago (see #5992 if you're interested), and I hand-edited this block of code to add it here, so I was surprised to see you making an unexplained change to it in an unrelated PR.

Not sure why you suggested that I read the Qt documentation instead of answering my question.

[worldwideweary]

Nice observation

[worldwideweary]

@Spire42 P.S., Check out PR #6506, as it looks like its prefsdialog.ui changes also has a similar edit of the code in QRadioButtonGroupBox. Again, maybe some Qt documentation might be able to answer your question, as you gave the impression of wanting to understand why the <container>1</container> line is being added.

[Spire42]

Looks good to me now. Hope it makes it into 3.5 Beta. (It's already on the new list at #6027.)

[worldwideweary]

Looks good to me now. Hope it makes it into 3.5 Beta. (It's already on the new list at #6027.)

Thanks for keeping an eye out. I'm weary about Qt's behavior now related to .ui files

[worldwideweary]

Maybe you should check the Qt Manual to find documentation about the <> tags if you can find them regarding the Designer files?

[worldwideweary]

If you figure out why Qt added the line and think that it was not appropriate, let me know if you or anyone else think I should manually delete this. Thanks.

In the meantime, I'm manually eradicating the line. Although, in a sense maybe it isn't a bad idea to have a container for a RadioButton "Group box".

[anatoly-os]

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

[vpereverzev]

no more actual for MU4