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

Fix #293008: Plugin: Provide Note.tieBack & Note.tieForward properties in QML. #5258

Merged
merged 1 commit into from Aug 28, 2019

Conversation

@DLLarson
Copy link
Contributor

commented Aug 5, 2019

Provide Note.tieBack & Note.tieForward properties and their
associated Tie objects in QML. Also provides Note.firstTiedNote
and lastTiedNote methods to locate a tie range.

@BernardSGreenberg

This comment has been minimized.

Copy link

commented Aug 5, 2019

The identity strategy for MS QML objects is a set of tradeoffs that requires careful documentation for plugin writers. The non-identity (non "EQ"-ness) of javascript objects representing the same C++ object is a keystone of this memory-management strategy, not a glitch.

@DLLarson DLLarson force-pushed the DLLarson:plugin-expose-note-ties branch 2 times, most recently from 7fea4f0 to efcc2fe Aug 5, 2019

@marlovitsh

This comment has been minimized.

Copy link

commented Aug 9, 2019

would be nice to see this in the next release.
I can't used MS 3 because I can't compare any more MS objects directly as I did in MS2 in qml/plugins.
This would be a nice solution.

@DLLarson DLLarson force-pushed the DLLarson:plugin-expose-note-ties branch from efcc2fe to fd37d68 Aug 21, 2019

@dmitrio95 dmitrio95 added the Plugins label Aug 27, 2019

mscore/plugin/api/tie.h Outdated Show resolved Hide resolved
@@ -32,6 +32,8 @@ namespace Ms {
namespace PluginAPI {

class Element;
class Tie;
extern Tie* tieWrap(Ms::Tie* tie);

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Aug 27, 2019

Contributor

wrap<Tie>() can be used instead (if Tie becomes inherited from Element)

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

extern Tie* tieWrap(Ms::Tie* tie);

I left this extern so I could isolate the wrap<Tie>(foobar) template call into the tie.cpp plug in wrapper implementation. If I just used the above in the element.h file I spent hours unsuccessfully chasing type errors in the template instantiation. Isolating it fixed the 'chase'.

Having wrappers with the same class names but different namespaces can be a real PITA when it comes to templates.

If you can solve this I would be happy to learn something in the process. Otherwise, I'm going with what works.

-Dale

@@ -383,7 +383,7 @@ class Note final : public Element {
void setTieFor(Tie* t) { _tieFor = t; }
void setTieBack(Tie* t) { _tieBack = t; }
Note* firstTiedNote() const;
const Note* lastTiedNote() const;
Note* lastTiedNote() const;

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Aug 27, 2019

Contributor

This const should probably not be removed, it is better to add a non-const flavor of this member function:
Note* lastTiedNote();
It could then be used in the plugins API implementation.

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

Because I couldn't get the compiler to be happy. In fact it looks like someone did the same thing as I did for firstTiedNote(). For instances this code with both const and non-const members:

  void setTieBack(Tie* t)         { _tieBack = t;    }
  Note* firstTiedNote() const;
  const Note* lastTiedNote() const { return const_cast<const Note*>(lastTiedNote()); }
  Note* lastTiedNote() const;

yields this compiler error:

d:\dev\musescore\musescore\libmscore\note.h(387): error C2556: 'Ms::Note *Ms::Note::lastTiedNote(void) const': overloaded function differs only by return type from 'const Ms::Note *Ms::Note::lastTiedNote(void) const'
d:\dev\musescore\musescore\libmscore\note.h(386): note: see declaration of 'Ms::Note::lastTiedNote'

Suggestions for a solution would be appreciated! :)

-Dale

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

Note that this commit (found via git blame) changed the non-const version into const causing my issue:

b46b55f

The committer did not const the other version.

-Dale

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Aug 27, 2019

Contributor

The compiler is correct, what is needed here is something like this:

  const Note* lastTiedNote() const { return const_cast<const Note*>(lastTiedNote()); }
  Note* lastTiedNote();

Note that I removed the second const in the second line of this example, that const refers not to the returned type but to the object for which this function is called, and compiler chooses which function to call depending on that const attribute for the type:

Note* n1 = new Note(score);
n1->lastTiedNote(); // Note* lastTiedNote() gets called
const Note* n2 = new Note(score);
n2->lastTiedNote(); // const Note* lastTiedNote() const gets called

So in order to fix the issue you should probably declare these functions like I mentioned above and remove const from declarations of these functions in wrappers as it doesn't seem to be needed there:
https://github.com/musescore/MuseScore/pull/5258/files#diff-3b5eed1d493c70737297ac61ccfec286R467-R468

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

that const refers not to the returned type but to the object for which
this function is called, and compiler chooses which function to call
depending on that const attribute for the type:

This is what tripped me up. To me the second const asserts that the method won't be modifying the object state upon call. So it seems like it belongs from that perspective. Clearly the compiler sees it differently.

-Dale

*
* \since MuseScore 3.3
*/
Q_PROPERTY(qint64 elementId READ elementId)

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Aug 27, 2019

Contributor

Could you please move this change to a separate pull request? Other changes in this PR are certainly good and can be merged once mostly minor implementation-related issues are resolved, but I would rather avoid using elementId if it is possible at all.

The reason I don't believe this to be a good solution is that elementId value can easily be stored (and seems natural to be stored) without a wrapper object itself. As it is some memory address it can lead to mistakes while trying to indentify objects. For example, if we take elementId of some element which later gets deleted for some reason, its memory address could be (and is likely to be) reused by another element created later. Comparing elementId of this element with the stored one will identify this as the same element as the old one while it is essentially different and can potentially even have a different type.

So I would consider exactly this change debatable and would try to consider other solutions first (like ensuring that the same wrapper is always returned for the same element or exploiting valueOf() for Element type to make it possible to compare different wrappers using == (but not === in this case) operator).

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

I knew this would rub some sensibilities since it rubbed mine. The problem looks to be a side-effect of factoring out the PluginAPI interfaces and using wrappers for them.

Please join the discussion here so we can hammer this out.

https://musescore.org/en/node/292900#comment-937888

I'll be removing the elementId change from this PR shortly.

-Dale

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

valueOf()

I don't see this implemented in Element in either namespace. It looks like it belongs to Javascript but I don't see how it drills down into the Element held by a wrapper.

Dale

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

I created a new PR and associated MuseScore issue for the elementId stuff.

https://musescore.org/en/node/293837

-Dale

@DLLarson DLLarson force-pushed the DLLarson:plugin-expose-note-ties branch 2 times, most recently from 85ec5c6 to bf8ccb8 Aug 27, 2019

@DLLarson
Copy link
Contributor Author

left a comment

Why is this here?
-Dale

@@ -383,7 +383,7 @@ class Note final : public Element {
void setTieFor(Tie* t) { _tieFor = t; }
void setTieBack(Tie* t) { _tieBack = t; }
Note* firstTiedNote() const;
const Note* lastTiedNote() const;
Note* lastTiedNote() const;

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

that const refers not to the returned type but to the object for which
this function is called, and compiler chooses which function to call
depending on that const attribute for the type:

This is what tripped me up. To me the second const asserts that the method won't be modifying the object state upon call. So it seems like it belongs from that perspective. Clearly the compiler sees it differently.

-Dale

@DLLarson DLLarson force-pushed the DLLarson:plugin-expose-note-ties branch 2 times, most recently from c220a73 to 1cb6a99 Aug 27, 2019

@@ -3053,7 +3053,7 @@ Element* Note::prevSegmentElement()
// lastTiedNote
//---------------------------------------------------------

const Note* Note::lastTiedNote() const
Note* Note::lastTiedNote()
{
std::vector<const Note*> notes;
const Note* note = this;

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Aug 27, 2019

Contributor

If you remove const from this declaration there should be also no need in doing a const_cast a few lines later (at line 3069)

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

I'm getting recursion warnings now... This is the header defn...

  Note* lastTiedNote();
  const Note* lastTiedNote() const { return const_cast<const Note*>(lastTiedNote()); }

Yields this error...

d:\dev\musescore\musescore\libmscore\note.h(387): warning C4717: 'Ms::Note::lastTiedNote': recursive on all control paths, function will cause runtime stack overflow

Which it would.

It probably doesn't like using the non-const function version so defaults to self.

-Dale

This comment has been minimized.

Copy link
@dmitrio95

dmitrio95 Aug 27, 2019

Contributor

Ah, yes, the implementation of const flavor of this function should also be corrected:
`const Note* lastTiedNote() const { return const_cast<Note*>(this)->lastTiedNote(); }

This comment has been minimized.

Copy link
@DLLarson

DLLarson Aug 27, 2019

Author Contributor

That didn't fix it either. I just found a solution. It's in a book by Scott Meyers called "Effective C++: 55 Specific Ways to Improve Your Programs and Designs" on page 23...25ish. It's in a section called "Avoiding Duplication in const and Non-const Member Functions".

We need to fully define the const version of the function and then do the casting in the non-const version. I'll be pushing it up shortly. There is a full explanation in the book as to why this works and a good approach.

See this post:

https://stackoverflow.com/questions/123758/how-do-i-remove-code-duplication-between-similar-const-and-non-const-member-func

Always something to learn...and they keep heaping it on!

-Dale

Fix #293008: Plugin: Provide Note.tieBack & Note.tieForward propertie…
…s in QML.

Provide Note.tieBack & Note.tieForward properties and their
associated Tie objects in QML. Also provides Note.firstTiedNote
and lastTiedNote methods to locate a tie range.

@DLLarson DLLarson force-pushed the DLLarson:plugin-expose-note-ties branch from 1cb6a99 to b60fea5 Aug 27, 2019

@anatoly-os anatoly-os referenced this pull request Aug 28, 2019
30 of 30 tasks complete

@dmitrio95 dmitrio95 merged commit 2c82d0d into musescore:master Aug 28, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@DLLarson DLLarson deleted the DLLarson:plugin-expose-note-ties branch Aug 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.