Update colornotes.qml to use Score.elements.selection

[wizofaus]

Resolves: https://musescore.org/en/node/321398

Fixes the bug in the ColorNotes sample plugin that causes it to apply to notes not currently selected if a distinct selection (not a range) is used, and simplifies the code.

• I signed CLA
• I made sure the code in the PR follows the coding rules
• I made sure the code compiles on my machine
• I made sure there are no unnecessary changes in the code
• I made sure the title of the PR reflects the core meaning of the issue you are solving
• I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
• I made sure the commit message title starts with "fix #424242:" if there is a related issue
• I created the test (mtest, vtest, script test) to verify the changes I made

[wizofaus]

I didn't check "I made sure the code compiles on my machine" because so far I can't get MuseScore to compile. But the plugin code compiles/runs as expected.
Not sure how we can write unit tests for sample plugins?

[wizofaus]

At one point my plugin crashed here because I accidentally passed in something that wasn't a note, so figured it was worth keeping as a safety check

[Jojo-Schmitz]

It'd be advisable not to use your master branch for PRs but some 'topic-branch'

[Jojo-Schmitz]

One level less indent here
And for the next 2 closing braces too

[Jojo-Schmitz]

this is where the excess indent from above looks bad and confusing

[wizofaus]

Yes I noticed that myself, was going to fix

[wizofaus]

It'd be advisable not to use your master branch for PRs but some 'topic-branch'

Yeah I wouldn't normally, but had already made the change and forgot to checkout the new branch before committing.

[Jojo-Schmitz]

Wouldn't a similar change apply to the notenames plugin too?

You may want to bump up the version number of that plugin too.
To something that does reflect that this won't work pre 3.5.

[Jojo-Schmitz]

No merge commits, please, just git push -f
and just one commit, so please squash them

[wizofaus]

No merge commits, please, just git push -f
and just one commit, so please squash them

Is that for future reference? And are those "rules" explained anywhere (I've never seen the point in those sort of policies, been using git for 10 years, but admittedly not done a heap of open-source collaboration)

[Jojo-Schmitz]

It is, in the developers' handbook, git workflow

[wizofaus]

It is, in the developers' handbook, git workflow

Hmm ok, thanks - who's able to approve running the remaining workflows btw?

[Jojo-Schmitz]

Only staff members AFAIK. @vpereverzev?

[wizofaus]

No merge commits, please, just git push -f
and just one commit, so please squash them

The option to squash while merging doesn't seem to be available in github, might be a permissions thing?

[Jojo-Schmitz]

It is available localy, e.g. git rebase -i HEAD~5; git push -f

[wizofaus]

It is available localy, e.g. git rebase -i HEAD~5; git push -f

I thought I did that (well, not quite, I can't use git rebase -i currently due to some weird gitbash issue, but there are other ways of achieving the same thing) but it didn't work.
At any rate, I don't want to lose my local commit history, I often do big changes with a number of small independent commits each with their own comments etc., that I can then cherry-pick etc. as needed later.

[vshalkevich]

@wizofaus Did you find any issues with your PR ?
In what platforms (Linux, MacOS, Windows) did you check your PR ?

[wizofaus]

@wizofaus Did you find any issues with your PR ?
In what platforms (Linux, MacOS, Windows) did you check your PR ?

Windows only but given the nature of the change would be very surprised if it behaved differently elsewhere.

[Jojo-Schmitz]

Indeed, those changes should be entirely platform independant.

I just found issueswith this: if nothing gets selected, it does color every note, which is OK and same as before the changes, but that operation is not undoable anymore (except by running the plugin again), nor is the score marked 'dirty', regardless of having been changed.
It seems cmd("select-all") bypasses the undo stack, and curScore.startCmd() ... curScore.endCmd() doesn't help either!?

These are regressions that need fixing.

[wizofaus]

Indeed, those changes should be entirely platform independant.

I just found issueswith this: if nothing gets selected, it does color every note, which is OK and same as before the changes, but that operation is not undoable anymore (except by running the plugin again), nor is the score marked 'dirty', regardless of having been changed.
It seems cmd("select-all") bypasses the undo stack, and curScore.startCmd() ... curScore.endCmd() doesn't help either!?

These are regressions that need fixing.

Good catch, will look into that today.

[wizofaus]

Indeed, those changes should be entirely platform independant.

I just found issueswith this: if nothing gets selected, it does color every note, which is OK and same as before the changes, but that operation is not undoable anymore (except by running the plugin again), nor is the score marked 'dirty', regardless of having been changed.
It seems cmd("select-all") bypasses the undo stack, and curScore.startCmd() ... curScore.endCmd() doesn't help either!?

These are regressions that need fixing.

curScore.startCmd()/endCmd() work for me, note I put startCmd after select-all. Can you re-test? I notice it still hasn't run all the workflows, I assume all 8 are needed for it to be merged?

[Jojo-Schmitz]

Hmm, I had curScore.startCmd() before the cmd("select-all"), that didn't help (and gave numerours error messages too).
But your change indeed does work, it gives one message though: "Debug: Score::startCmd(): cmd already active". And the curScore.endCmd() seems to not be neeeded at all (but doesn't harm either), probably it gets called implicitly by the Qt.quit().
One quirk though: on 'undo' it seems to do the cmd("select-all") again.
Definitly different to what happens prior to your changes. Any idea anyone how to fix that?

Before getting the workflow to run, please squash all your changes into just one commit.

[wizofaus]

Hmm, I had curScore.startCmd() before the cmd("select-all"), that didn't help (and gave numerours error messages too).
But your change indeed does work, it gives one message though: "Debug: Score::startCmd(): cmd already active". And the curScore.endCmd() seems to not be neeeded at all.
One quirk though: on 'undo' it seems to do the cmd("select-all") again.

Before getting the workflow to run, please squash all your changes into just one commit.

I don't know how to squash the commits only on the remote branch. I don't want to squash locally, particularly on my master branch, as it's very useful having a full commit history.

[Jojo-Schmitz]

Well, I guess you'd just have to, if you want this merged. It is one fix, so should be just one commit.

[wizofaus]

Well, I guess you'd just have to, if you want this merged. It is one fix, so should be just one commit.

Looks like I figured out a way, though it was hardly straightforward!

[wizofaus]

Hmm, I had curScore.startCmd() before the cmd("select-all"), that didn't help (and gave numerours error messages too).
But your change indeed does work, it gives one message though: "Debug: Score::startCmd(): cmd already active". And the curScore.endCmd() seems to not be neeeded at all (but doesn't harm either), probably it gets called implicitly by the Qt.quit().
One quirk though: on 'undo' it seems to do the cmd("select-all") again.
Definitly different to what happens prior to your changes. Any idea anyone how to fix that?

Before getting the workflow to run, please squash all your changes into just one commit.

The behaviour of reselecting all elements after an undo that applies to all elements is exactly what occurs normally though (when not using a plugin). Understood its different to the previous colornotes behaviour but I don't see it's a problem...

[Jojo-Schmitz]

Well, it is a problem though. Whether it is a major one and/or one that can and should get fixed or whether it can be let passed is a different question.

[vshalkevich]

yes

[vshalkevich]

For some reason the MacOS build got canceled?
I don't think this PR is causing this.

Can we close it without merge ?

[Jojo-Schmitz]

Why?

[vshalkevich]

@wizofaus please rerun build check for MacOS, I would like to test also on this platform

[Jojo-Schmitz]

I don't think us 'mere mortals' can do this? Other than via something like git commit--amend; git push -f
@igorkorsukov can you trigger a rebuild please?

[Eism]

@Jojo-Schmitz triggered

[vshalkevich]

Checked. Seems fine
But can`t install any Plugins in Windows 10