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 #316504: Update MusicXML Import of Fretboard Diagrams #7455

Conversation

worldwideweary
Copy link
Contributor

@worldwideweary worldwideweary commented Feb 9, 2021

Resolves: https://musescore.org/en/node/316504

After updating Fret Diagrams to allow for [Open markers + additional fret dots] from within MuseScore's fretboard diagram editor, MusicXML import needed updating to properly form a diagram with potentially both open markers and fret marks on the same string. There was also an issue with extraneous mute markings even with the presence of a fret-dot while being imported (they are implicitly formed during initialization rather than being explicit data like with fret markers). This takes care of issue linked above.

Aside: even though FretDiagram::init() isn't involved in importing from xml, it was erroneously setting _maxFrets rather than _frets from the corresponding "getter" frets() function, so changed that also.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@RobFog
Copy link

RobFog commented Mar 24, 2021

What are the chances this could introduces regressions? Perhaps this feature should be added to MuseScore 4 instead of 3.x?

@worldwideweary
Copy link
Contributor Author

@RobFog, any ideas?

@RobFog
Copy link

RobFog commented Apr 20, 2021

I'm not a programmer but from the description it sounds like this is a larger change. The maintainers don't seem to want to introduce new features and major changes in 3.x.

@RobFog
Copy link

RobFog commented Apr 20, 2021

@jthistle What do you think?

@jthistle
Copy link
Contributor

This is a 25 line change that doesn't really introduce any new features, but rather makes existing features compatible with MusicXML. That is, it's a fix rather than a feature. I'm all for this PR.

@jthistle
Copy link
Contributor

That said, would it be possible to add an automated test (or modify an existing one) to check that this works and stays working?

@lvinken
Copy link
Contributor

lvinken commented Apr 24, 2021

Code reviewed and tested, all OK. The current MusicXML regression tests (read MusicXML, write MusicXML, compare) do not catch this issue, as it affects fretboard state that cannot be exported to MusicXML. Reading MusicXML and writing in MuseScore format does catch the issue.

@Jojo-Schmitz
Copy link
Contributor

Interestingly large parts of this, most parts of the changes to libmscore/fret.cpp, are in master already.
For the rest see #8014

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request May 11, 2021
@vpereverzev
Copy link
Member

3.x merges are closed

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jun 28, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 27, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jul 28, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 13, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 19, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 2, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Sep 23, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Mar 5, 2023
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

6 participants