-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix #301561: Plugin API: restore pagenumber and pagePos properties from MuseScore 2.X #5986
fix #301561: Plugin API: restore pagenumber and pagePos properties from MuseScore 2.X #5986
Conversation
@@ -114,6 +115,11 @@ class Element : public Ms::PluginAPI::ScoreElement { | |||
* \since MuseScore 3.3 | |||
*/ | |||
Q_PROPERTY(qreal posY READ posY) | |||
/** | |||
* Position of this element in page coordinates, in spatium units. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(read only)
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually some "read-only" marker will also be generated by Doxygen so there is no strict necessity in writing that explicitly. Other similar properties in Element
class do not have read-only
in their description. Maybe it would be better to remove that from pagenumber
description then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, either way works for me ;-)
/** | ||
* Page number (read only). | ||
* Returns number of this page in the score counting from 0, i.e. | ||
* for the first page its \p pagenumber value will be equal to 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make sense? Why not starting with 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I actually just made the implementation of this property equivalent to the one from MuseScore 2.X. It would be good to have a property for user-visible page numbers, but then is should start not with 1 but with (page numbering offset setting) + 1
. Since that setting can have any value it would make it more problematic to obtain an "internal" page number, e.g. to detect the first page in a score. It seems to me it could potentially be useful to have both these kinds of page number property (or to expose a property for page numbering offset for Score
objects) but in this pull request I just added the one which had been exposed in 2.X versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make sense to rename it to something like pageIndex
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is 'just' reimplementing something from MuseScore 2.x, better keep name and semantic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and maybe add pageOffset and let the plugin programmer sort the logic.
Was it really 'pagenumber' rather than `pageNumber'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was named exactly pagenumber
, see https://github.com/musescore/MuseScore/blob/2.3.2/libmscore/page.h#L127
I added a commit with pageNumberOffset
property here.
ad782ca
to
15a3051
Compare
Restores
pagenumber
andpagePos
properties which were exposed to plugins in MuseScore 2.X, as requested in https://musescore.org/en/node/301561.Update: also added
Score.pageNumberOffset
property to allow obtaining (and changing) user-visible page numbers.