Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin API: partially restore readScore, closeScore and writeScore #20822

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmitrio95
Copy link
Contributor

Resolves (partially): #13593

The changes in this pull request partially restores the functionality of the plugins' readScore(), closeScore() and writeScore() functions to the extent that doesn't require changing the current multi-instance logic. With these changes it will be possible to:

  • Use readScore() to open a score in the current window, if it is empty, or in a new window if the current window already has an opened project;
  • Use writeScore() to write the currently opened score;
  • Use closeScore() to close the notation project opened in the current window.

In addition to the limitations of the multi-instance model, there are two more limitations in the proposed implementation of these functions which may be addressed in future (this will probably require changes in the code not related to plugins):

  • The noninteractive flag of readScore() is not currently implemented, addressing this will require changes in the code related to notation projects loading and import;
  • writeScore() can only write the currently selected notation and not other (part and master) scores within the current project, as the export logic seems to rely on Notation and NotationProject objects, while plugins have access only to Score objects.

The proposed implementation, of course, is far from a full functionality of this API but this will still allow the most basic import/export plugins to function.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 4, 2024

Batch Convert does seem to work fine with this!

Comment on lines 84 to 85
notation
project
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these are really necessary. When only using "interfaces", like INotationWritersRegister, INotationWriterPtr, INotation, linking is usually not necessary because all methods of these interfaces are pure virtual methods, so #including already gives all available information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, these are indeed not necessary. I have added this linking because the plugins module now depends on those other modules, but I didn't notice that this dependency does not really require linking. I have removed it now.

return false;
}

project::INotationWriter::UnitType unitType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, for better readability, we can move the resolving of unitType into a separate method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I moved this code to a separate method.

@DmitryArefiev
Copy link
Contributor

DmitryArefiev commented Feb 12, 2024

Tested on Win11.

@dmitrio95 @Jojo-Schmitz To what extent Batch Convert and ABC Import plugins should work better than in master?

  1. Using Batch Convert plugin, now it works in Import mode (batch processing of folder with scores) and doesn't work in Export current score and Export opened scores. In master build the plugin doesn't work at all

Screenshot 2024-02-12 190916

  1. Using ABC Import plugin, it works the same as in master (as described in https://musescore.org/en/project/abc-import)

@dmitrio95
Copy link
Contributor Author

Concerning Batch Convert:

  • Import mode should work as expected at least if no scores are currently opened in the current window (this can be checked by a plugin).
  • The "export current score" mode doesn't work because MuseScore 4 does not support the score.path property that is available for plugins in 3.6.2. This can be corrected either on the plugin's side (by avoiding using this property and exporting the score directly) or on the MuseScore's side by implementing this property.
  • The "export opened scores" may not work for the same reason, although this mode will probably face an additional difficultly because of inability to access scores opened in different windows, and it will likely work effectively the same as the "export current score" mode.

Concerning the ABC Import plugin, it will need to be corrected to re-enable this line which has been disabled exactly because the readScore() API function was not working in MuseScore 4.

@Jojo-Schmitz
Copy link
Contributor

rebase needed

@dmitrio95
Copy link
Contributor Author

Rebased

@DmitryArefiev
Copy link
Contributor

@dmitrio95 Sorry for the delay..

Concerning the ABC Import plugin, it will need to be corrected to re-enable this line which has been disabled exactly because the readScore() API function was not working in MuseScore 4.

Can you correct that line so I can check it on my side?

@Jojo-Schmitz
Copy link
Contributor

It is in a different repository, @jeetee's

@dmitrio95
Copy link
Contributor Author

Here is the modified plugin: abc_import-4.0.0.zip. I enabled back the readScore() call and removed the workaround with copying the path to the imported file. Seems to work fine on my build.

@DmitryArefiev
Copy link
Contributor

@dmitrio95 Thanks! But when I wanted to check you PR, I found a bug in master branch #21659 which blocking me now..

@Jojo-Schmitz
Copy link
Contributor

I guess for now you'd better rebase it to 4.3.0?

@cbjeukendrup cbjeukendrup changed the base branch from master to 4.3.0 March 31, 2024 22:17
@cbjeukendrup cbjeukendrup changed the base branch from 4.3.0 to master March 31, 2024 22:17
@RomanPudashkin
Copy link
Contributor

@dmitrio95 please rebase

@yedaming12
Copy link

Tested on Win11.

@dmitrio95 @Jojo-Schmitz To what extent Batch Convert and ABC Import plugins should work better than in master?

  1. Using Batch Convert plugin, now it works in Import mode (batch processing of folder with scores) and doesn't work in Export current score and Export opened scores. In master build the plugin doesn't work at all

Screenshot 2024-02-12 190916

  1. Using ABC Import plugin, it works the same as in master (as described in https://musescore.org/en/project/abc-import)

Hello, can your Batch Convert work normally in import mode? Why do I display error open score in versions 4.2.1 and 4.3.0

@Jojo-Schmitz
Copy link
Contributor

Because this PR hasn't been merged yet (and needs another rebase)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants