Port the add/remove functions that exist on the Chord cpp end into the 3.x Plugin API wrapper

• Jul 5, 2019 - 19:41
Reported version
3.2
Type
Plugins
Frequency
Once
Severity
S5 - Suggestion
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project

Steps to reproduce:

1) https://github.com/birdwellmusic/PruneStack/blob/master/prunestack-3x.q…
2) line 176, e.g., chord.remove(...) should work if/when "remove" (for "chord" objects) is exposed to the API via QML and/or plugin API.


Comments

I have the Chord.remove() code working with undo support.

However, when removing the final note of a chord I get an internal ASSERT complaining of a chord with zero notes in it.

Given that a scripting interface (or any code for that matter) should NEVER crash the program what should this behavior be? Should an error be raised when deleting the final note? Or... what other approach? Certainly the script writer shouldn't be trusted with the responsibility.

I'm currently using a simple score and QML to test (see attached.)

Ultimately I'd like a test score that can effectively test the prunestack-3x plugin.

Also, note that I won't create a pull request against the upstream code until we're happy with the work.

FWIW...Here's my working topic branch on github:

https://github.com/DLLarson/MuseScore/tree/chord-add-remove

-Dale

I have the Chord.add() method nearly working but it runs into an ASSERT having to do with Tpc values. I have no idea what all this Tpc stuff is.

Here is the script I'm using to test the add:

console.log("Call cursor.element.add(newNote)...")
var newNote = newElement(Element.NOTE);
newNote.pitch = 69
cursor.element.add(newNote)

When newElement creates a new note object it produces a note with invalid tpc values (-2). From C++ this looks like this:

QMLnewElementCallAndResult-20190706.png

Then QML sets the pitch and adds the new note to the chord. This looks like this in C++:

QMLChordAddCallAndResult-20190706.png

Note that the Chord.add() operation returns with no issue.

Finally an ASSERT occurs in some Tpc code. In C++:

QMLassertAfterChordAddCall-20190706.png

This appears to be due to MuseScore rendering but I'm not nearly sure. It's definitely an async event though.

I'm at a loss how to get these Tpc stuff corrected.

-Dale

In reply to by DLLarson

Hi Dale - thanks for the update and for your work on this - awesome!

Got the nightly per above and tested it with the attached QML. While the script runs, it doesn't seem like the chord.remove method is accessible (see line 176 and/or the debug/trace).
...
176:-1: TypeError: Cannot call method 'remove' of undefined
Warning: :176: TypeError: Cannot call method 'remove' of undefined
...

It didn't crash though! :) Is it possibly I don't have the correct nightly or your revised resource?

>Is it possibly I don't have the correct nightly or your revised resource?

I'm trying the same downloaded build again fresh using your QML and score. I get the same result.

However when I run my test script it works so I know the method is exposed. This suggests that the object isn't a chord.

I'm checking on it.

-Dale

The problem is that the parent field is returning something QML doesn't recognize.

     var chord = notesToDelete[n].parent;
     console.log("chord=" + chord)

Debug: chord=undefined

If this code worked in v2.0 it suggests a behavior difference between the two.

-Dale

I think I've corrected the problems.

As stated above, the v3.0 plugin api didn't expose the parent property. I've added that in. Plus, plugin's with a dialog UI (or any UI?) need to manage their own undo stack boundaries.

Also the function Score.doLayout() function is not exposed in v3.0. But that may be OK as the view is repainted automatically.

I've attached an updated version of your QML with the above changes added. It runs my testing version.

Stay tuned for a new test build here:

https://ci.appveyor.com/project/MuseScore/musescore/builds/25826475

-Dale

Attachment Size
prunestack-3x.qml 11.61 KB

Bravo! This worked great in my QML test. Thank you so much Dale/DLLLarson and everyone involved. In the next day or so I will update the Prune Stack QML for 3.x with Dale's recommendations and I might clean up some other things in the script too. Bottom line is that chord.remove works now in 3.x!

In reply to by rob@birdwellmu…

Well the PR still needs to get past the github crew but it should pass muster!

It was very helpful having a PR build I could point to for testing. This prevented my original commit from being absorbed until a real user--you--could try it.

Otherwise we would've waited for a patch release, then it would have failed for you. Thereby starting another fix iteration.

-Dale

Sorry for being a bit late to this discussion, but I would like to make a bit different proposal. The implementation of this feature in the pull request looks good, but maybe the API itself should be organized in a slightly another way. Consider the following:

1) Chord is actually not used in remove() function, or at least should not be used. Nothing stops a plugin code from calling remove() for a note in a wrong chord, and the implementation would still work correctly (currently except the case when the only note in a chord is deleted).

My suggestion is to add a removeElement() function to the "global" part of API instead as a counterpart to newElement() function. Then one doesn't need to manually determine the correct element to remove some element from it. It might be easier to implement such function using deleteItem() function to ensure that all elements types are removed properly. But that needs further testing.

2) Chord::add() may be not needed as Cursor::add() function is supposed to do the same thing. Maybe the implementation of Cursor::add() could be extended to handle notes, artuculations and some other types instead. Then we would continue to have a single API to add elements to a score.

What do you think of having that API designed this way instead of directly exposing analogs of Chord class functions to plugins? Would this be reasonable or is there something else my suggestions do not cover?

For (2)... very good point. Looking at the code this should work.

For (1)...
I like what you're saying here.

You're right that chord isn't used to do the remove but it was there to qualify if the removal is valid. I was worried about creating an orphan chord having no notes:

  if (chord()->notes().size() <= 1 && s->type() == ElementType::NOTE)
        qWarning("PluginAPI::Chord::remove: Removal of final note is not allowed.");

If the generic version is used will it prevent this problem?

-Dale

I just recalled that this is a restoration of the QML 2.0 functionality that didn't make the cut on the original 3.0. I have another PR that is a new feature and I sometimes confuse the two.

That should carry some weight in changing approaches.

Or...perhaps add the improvement and subtly deprecate the 2.0 versions of the Chord.add/remove.

-Dale

More comments on the proposal...

(1) "My suggestion is to add a removeElement() function to the "global" part of API instead as a counterpart to newElement() function."

This make sense. However, even though internally Chord.remove() calls the exact same function it should be retained as it was already published in 2.0.

There does need to be an object disposal service in the plugin api. It is conspicuously absent when you look for it.

"It might be easier to implement such function using deleteItem() function to ensure that all elements types are removed properly."

The Score::deleteItem() method seems lower level than what we want here. I'm using "Score::undoRemoveElement(Element*)" which provides awesome removal/recovery support. It should do the deleteItem() call (and probably does) when the undo frames are tossed forever.

(2) "Chord::add() may be not needed as Cursor::add() function is supposed to do the same thing. "

True but the plugin doesn't assume the chord is "under" the cursor. It builds a list of chords and modifies them wholesale. The cursor might not even exist when the add/removes are done.

-Dale

Cool - will this be in the nightly build tonight? I can test with my PruneStack QML.

I like the idea of a removeElement() method for plugins. Seems like that would potentially be useful for other plugins to surface additional ways to selectively remove elements from the current selection.

> even though internally Chord.remove() calls the exact same function it should be retained as it was already published in 2.0.

We don't have to maintain a compatibility with 2.X API, but we will have to maintain a compatibility with the API added to 3.X versions at least while 3.X series is actively developed. I believe at this point constructing a cleaner API is more important than trying to achieve a maximal compatibility with 2.X plugins as this compatibility is not preserved anyway.

> The Score::deleteItem() method seems lower level than what we want here

Actually deleteItem() is a somewhat higher level than undoRemoveElement: it calls undoRemoveElement() in its implementation and, among other things, ensures that the whole chord gets removed from a score if the last note from it gets removed. That is why I suggested using this function, but, as I have said, that may need some testing.

> True but the plugin doesn't assume the chord is "under" the cursor. It builds a list of chords and modifies them wholesale. The cursor might not even exist when the add/removes are done.

Plugin can always create a cursor, but then a cursor would need some way to rewind to a certain element (or cursor should be able to be created just on a certain element's position). Overall the Cursor API makes it easier to ensure that elements are added to a score properly, and it would seem less consistent if some elements were added via Cursor and others via Chord.add(). In any case I would extend Cursor.add() to support notes, chord articulations and other types supported by Chord.add(), but I am not sure whether we should make Cursor the only way to add them to a score or we should retain the Chord.add() function from 2.X API.

>Plugin can always create a cursor, but then a cursor would need some way to rewind to a certain element (or cursor should be able to be created just on a certain element's position). Overall the Cursor API makes it easier to ensure that elements are added to a score properly, and it would seem less consistent if some elements were added via Cursor and others via Chord.add(). In any case I would extend Cursor.add() to support notes, chord articulations and other types supported by Chord.add(), but I am not sure whether we should make Cursor the only way to add them to a score or we should retain the Chord.add() function from <

This final paragraph only adds confusion to what I'm doing as it just heaps on more uncertainty. I need solid approach to proceed. I can't keep throwing code against the wall waiting for a "Try again!" suggestion.

I feel as though I'm at an impasse here.

-Dale

I can sympathize with Dale's comments. It seems this task is getting some feedback and ideas, however, if the recommendations/approach keep shifting with each development iteration then the "specifications" for the task, perhaps, are not as clearly defined or consistently agreed upon as we'd all hope.

In other words, there are multiple ways to pull this potato from the ground.

In the following responses, I would suggest folks (especially those with deep insights into MuseScore's API and Plugins) weigh-in on how add/remove elements for QML / Plugins should be implemented in 3.x.

Once a definitive approach is enumerated and generally agreed upon, then let the coding begin (again) and the task can be QA'd against those criteria.

Well, sorry for the confusion, perhaps I should have ended my comment explicitly asking for a feedback.
But it turns out that I don't see any clear advantage of having to add elements to a chord via a Cursor only, so I believe we can stick with an old Chord.add() function approach for now. It would be great if Cursor.add() could get a support for adding notes and articulations to a score either but this can safely be done separately of the discussed pull request.

If we agree on that then the pull request already contains all the necessary changes, and, except for the possible improvement from using deleteItem() rather than undoRemoveElement() and some other minor issues, should be ready to be merged. I'll put my comments to the pull request then.

Some of my comments are unresolved there, some of them (regarding using a proper score and chord for some operations) may be important for the new functions to work properly.

Status PR created fixed

Fixed in branch master, commit 19677e3876

_Fix #291790, fix #267604: Restore Chord.remove() and Chord.add() methods.

Restore the add and remove QML methods for the
Chord object. Adds exposed Element.parent property.
These existed in v2.x. Finally it adds a removeElement
method to PluginAPI for general element disposal._