fix #232931 for 2.2 - allow break shortcut on selections #3528
Conversation
|
Can you also include #3250? |
|
Not directly, no, since there is no cmdToggleLayoutBreak() function in 2.2. We'd have to duplicate the code in palette.cpp, and I'd rather not do that. |
|
It's only a few lines of code... |
|
Duplicated code is code that eventually diverges, leading to bugs and yet more inconsistencies. It's to be avoided unless necessary, and I don't see the need here. |
|
You're choosing to cause the code to diverge and create an inconsistency, and arguing against trying to keep it in sync because it might in the future diverge and create an inconsistency? I argued at length for the need last time. If I didn't convince you then, I don't suppose there's any point trying to now. ;-) |
|
I didn't see the big deal then and still don't now, true. But at least last
time didn't require additional duplication of code. Still, if someone else
wants to deal with it, they are welcome to.
…On Thu, Mar 8, 2018 at 6:48 PM IsaacWeiss ***@***.***> wrote:
Well, I argued at length for the need last time
<https://musescore.org/en/node/232931#comment-754496>. If I didn't
convince you then, I don't suppose there's any point trying to now. ;-)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABt3ANi6tVN2GPQdgf5cVGPsEZLhE7aMks5tcd9ZgaJpZM4Sjf0G>
.
|
|
Somehow while I slept last night you talked me into it :-). Well, the real key for me was realizing how much better the new shortcut behavior is than the palette behavior, also not liking 2.2 and master being out of sync here (another form of undesirable code duplication). So I will replace the commit here with another that basically copies the implementation from master, cmdToggleLayoutBreak() and all. This is the best answer for maintainability as well as functionality. |
MarcSabatella
force-pushed
the
232931-breaks-22
branch
from
7ad10f7
to
6e8367b
Compare
|
Re-factored so now this PR and master are in sync. Only difference is in the use of static_cast versus the new toMeasure() construct. I do notice that the code to make palette double-click call cmdToggleLayoutBreak only works on range selectins. Should really have a similar clause for list selections. I will add a second commit for that, and this commit will be good for both 2.2 and master. |
|
Done. The second commit can be cherry-picked for master as well. The static_cast should not be an issue; there is no toLayoutBreak() function. |
|
Merged in 2.2. The second commit doesn't apply cleanly on master. I will have a look to resolve the conflicts. |
|
Merged in master in d541fbc. Please have a look. |
|
Looks good, thanks. Weird, though, I didn't see the definition for toLayoutBreak(). Not defined with all the other similar functions, I guess. So the static cast a bit further down (the one added in #3250) should probably be fixed too. And use isLayoutBreak() there too. |
|
OK, I see, it's done via a macro at the bottom of scoreElement.h. I guess some elements have these defined explicitly, others by macro. Working on getting my build of master back up after all the changes last few months. Will submit PR to update use of type check & cast for master when I can. |
|
Looks like there's a couple of dozen or so other old-style casts and/or uses of ElementType comparisons in this file alone. Maybe better to make another pass through the code cleaning this all up in a separate PR? Anyhow, I won't worry about it for now. |
|
Yes, don't worry. We will change them along the way. |
|
See a regression that just came up today, where these breaks can no longer get applied to frames via double click (in 2.2, or even not at all in master): https://musescore.org/en/node/271450 |
Been meaning to do this a while now - a 2.2 port of the code I added for master last fall to allow "Enter" to toggle line breaks with selections other than just barlines. All we were doing was finding the measure for the barline and toggling the break there; it was trivial to modify the code to find try to find the measure for any single selection. For range selections, I take the last chordrest, so hitting enter on with a measure selected toggle the break on that measure. With multiple measures selected, the last measure only is toggled.