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

Not a bug, but a feature request

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:

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

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

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

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

Found the secret sauce to fix the previous post's problem.

I've attached test score and test plugin.

How to test this change:

If you are running on Windows you can test a build containing this change by unpacking the z-zip file found here:

https://ci.appveyor.com/project/MuseScore/musescore/builds/25808240/art…

onto your system and run the "nightly.exe" program found in the "....\MuseScoreNightly\bin" directory.

Beat it up with your previously busted plugin.

-Dale

>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

The problem is that the v2.0 api exposed 'parent' at the Element level. The v3.0 does not.

-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

A new test build is available:

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

It works for me.

-Dale

Congratulations and respect. I'm too unknowledgeable to assess your work, but .....

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

there is no PR yet

oops, there is: https://github.com/musescore/MuseScore/pull/5209

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

So civil so great. I'm a fan.

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

I pushed up the change that adds removeElement() to the PluginAPI.

I've attached a test script that checks using both Chord.remove() and remoteElement() with a toggle between calls.

-Dale

If you're running Windows there is a "nightly" that was produced as part of the PR build:

https://ci.appveyor.com/api/buildjobs/awysajsccj0p5o5t/artifacts/MuseSc…

The PR hasn't been accepted yet into master so the project's nightly isn't available yet.

Not many PR's have been accepted lately so I'm not sure when that will happen.

-Dale

> 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

This is so great. I usually go for a walk :) (Not sure my comment will help, but ...).

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.

Ok... I'm back on this.

Stay tuned.

-Dale

The changes are made and have been pushed up to Github.

-Dale

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.

I see I did indeed miss some! Sorry about that.
-Dale

I've attached a script that adds test for chord.remove() restrictions implemented in latest PR.

-Dale

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._

Fixed in branch master, commit 48fbee5a50

_Merge pull request #5209 from DLLarson/chord-add-remove

Fix #291790: Restore Chord.remove() and Chord.add() methods_

Final test script attached for posterity's sake.

Automatically closed -- issue fixed for 2 weeks with no activity.