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

allow unicode flat and sharp as input for chord symbols #3461

Merged
merged 1 commit into from
Aug 28, 2019

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Feb 11, 2018

also double flat and double sharp, Δ, °, ø and Ø. Comes up at times. last time in https://musescore.org/en/node/269352

@MarcSabatella
Copy link
Contributor

I haven't built this to test, but it kind of surprises me that it works to just change this here. Seems that will only catch accidentals used in the chord root (or bass note) but not in the chord extension.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 12, 2018

I've toyed with this some 2 years ago, so indeed would need to test this (again?).

Doesn't work at all currently, the whole chord symbol rendering seems broken or disabled in master.

Changed it to replace them all on input already, which seems the better choice and at least should work in current master too

@Jojo-Schmitz Jojo-Schmitz force-pushed the chord-symbols branch 3 times, most recently from a6b0711 to 0a18c72 Compare February 12, 2018 09:15
@MarcSabatella
Copy link
Contributor

I don't see how that would work either - as far as I can tell the replacement happens before the text exists. Have you verified this works?

@Jojo-Schmitz
Copy link
Contributor Author

No, not yet. It is carnival here ;-)

@lasconic lasconic added the work in progress not finished work or not addressed review label Feb 12, 2018
@MarcSabatella
Copy link
Contributor

OK, I finally tried it, and no, it does not work. To do the replacement, you'd need to treat the keystrokes as they come in (in edit) or when finished (in endEdit) or when parsed (in parseHarmony). A quick test suggests doing it at the very beginning of Harmony::endEdit() seems to work. More testing would still be required to make sure it really works correctly with respect to parts, transposition, etc.

@Jojo-Schmitz
Copy link
Contributor Author

Thanks, I'll move them there, tomorrow ;-)

@MarcSabatella
Copy link
Contributor

BTW, another approach is to allow the accidentals but parse them normally. That is, add them to the parser in chordlist.cpp (ParsedChord::configure) as well as convertNote(). This approach would mean they stay Unicode even when the user goes back to edit the chord later. The replace approach means if he edits later, he'll see "b" et al. Maybe that's better, then he'll learn he doesn't need the Unicode characters.

@Jojo-Schmitz
Copy link
Contributor Author

How exactly did you test it in Harmony::endEdit()? Wouldn't it need to be done in Harmony::edit(), just before the call to Harmony::parseHarmony(), for that not to fail?

@MarcSabatella
Copy link
Contributor

The parsing in edit() is just for the sake of the "spell check" so it is harmless - and maybe useful - if it fails there. I am not comfortable with replacing on each keystroke - which is how it would with edit() - because I think it would be disorienting and possibly glitchy to have your text changing as you type. Better to do it at the end, just as wedo with "b" and "#", I think. Do the replacement right the very top of Harmony::endEdit(), before even calling Text::endEdit(), since the latter has a bit of a ripple effect the links. At least, that's what makes sense to me at first glance. Still would need more extensive testing with respect to parts and transposition.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 13, 2018

How to get at the string there? Got to be from EditData& ed, but how?
Hmm, nonsense, I guess we could use _username.
Darn, can't get that to work

@lasconic
Copy link
Contributor

What's the status?

@Jojo-Schmitz
Copy link
Contributor Author

"Work in Progress" or "Help needed" ;-) I can't get it to work

@MarcSabatella
Copy link
Contributor

_username should indeed work. It also works to user the accessor function hUserName() as you were originally. Seems to work fine in 2.2, except as I mentioned, more testing woud be required. As you observed, master has more severe issues with chord symbols, so I didn't even bother trying that.

@Jojo-Schmitz
Copy link
Contributor Author

Ok, so I'll better switch to 2.2 for this

_userName.replace("\u0394", "t"); // Δ
_userName.replace("\u00d0", "o"); // °
_userName.replace("\u00f8", "0"); // ø
_userName.replace("\u00d8", "0"); // Ø
TextBase::endEdit(ed);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in 2.2 this is Text::endEdit();, guess that might be why the above replacements are not working in master?

Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Feb 16, 2018

Choose a reason for hiding this comment

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

No, it isn't that, stepping thru with a debugger (I really should have done that much earlier) shows that the replacement happens, but gets reverted by the endEdit

Copy link
Contributor

Choose a reason for hiding this comment

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

Odd, it worked for me when I tried it. But there is stuff going on with transposition and links that could be complicating this. I'm not worried, this strikes me as extremely low priority. The supported way of entering these symbols is easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. The discussion here shows that it will be hard to add this in 2.2 since we don't know what we will break :)

Copy link
Contributor Author

@Jojo-Schmitz Jojo-Schmitz Feb 17, 2018

Choose a reason for hiding this comment

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

Well, the problem is that it doesn't work at all. If it were, it'd be a simple text replacement before any interpretation and only the previously known characters would ever make it into the score file.
@MarcSabatella: I'd really like to see what you had working...

@Jojo-Schmitz
Copy link
Contributor Author

Not different in 2.2, it still doesn't work there, the replacement does take place as seen in debugger, but doesn't take any effect or gets reverted by the endEdit

@MarcSabatella
Copy link
Contributor

Exactly what you have - those same replace statements right at top of Harmony::endEdit(), exceptr for 2.2 instead of master. Worked as expected for me in my test, but again, I recognize results may differ depending on transposition, parts, etc.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 17, 2018

very strange, I did the very same stuff in 2.2 and it didn't work there either, see #3475
Edit: turned out my testing was bogus... as was your's!

@@ -735,6 +735,14 @@ bool Harmony::edit(EditData& ed)

void Harmony::endEdit(EditData& ed)
{
_userName.replace("\u1d12b", "bb"); // double-flat
_userName.replace("\u226d", "b"); // flat
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stupid mistake, it should be \u266d

and also double flat and double sharp, Δ, °, ø and
Ø.
@Jojo-Schmitz Jojo-Schmitz force-pushed the chord-symbols branch 2 times, most recently from 654fd20 to 3f094b4 Compare April 1, 2019 07:48
@dmitrio95
Copy link
Contributor

@Jojo-Schmitz as far as I understand, this PR is ready to be merged, isn't it?

@Jojo-Schmitz
Copy link
Contributor Author

Yes, I think so

@anatoly-os anatoly-os merged commit 6c2df0c into musescore:master Aug 28, 2019
@Jojo-Schmitz Jojo-Schmitz deleted the chord-symbols branch August 28, 2019 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress not finished work or not addressed review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants