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 #281362: allow shift+drag to rearrange palette #5155

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

MarcSabatella
Copy link
Contributor

See https://musescore.org/en/node/281362

In 2.x it was possible to rearrange elements in custom palettes by dragging them around, but somehow this abiity was lost in MuseScore 3. After some investigation, I found the code for this restructured significantly at 292a8d9. I can see the intent was that Shift+drag would do the job now, but that doesn't work either, because currentIdx is always -1 when the following code executes:

https://github.com/MarcSabatella/MuseScore/blob/309611ca65ab3d9b1d23c4e6b7e0131f36c33db8/mscore/palette.cpp#L1825-L1834

currentIdx is originally set to the dragged element, but it gets cleared (set to -1) in Palette::leaveEvent(). That code has been there since day one and presumably serves a useful purpose. So I'm not sure how this ever worked, but apparently it did, on macOS at least - see https://musescore.org/en/node/283590.

So while I'm not totally sure what's going on here, I can say that simply disabling this clearing if Shift is pressed works: now Shift+drag allows rearranging the palette as I believe was intended.

I can't swear there are no side effects (what about Shift while moving the mouse and not dragging?) and I also can't swear this works on all platforms or all versions of Qt. But it's worth considering. I welcome further review / testing!

@dmitrio95
Copy link
Contributor

The only issue I noticed during my tests is a minor visual glitch where moving a mouse away from palette with no buttons clicked and Shift button pressed leaves a palette cell selected. Otherwise this patch indeed seems to work well. Not sure why leaveEvent is called on dragging when mouse actually doesn't leave a palette but it turns out to happen indeed.

@MarcSabatella
Copy link
Contributor Author

That's definitely the kind of glitch I kind of expected to see, but apparently I hadn't tested actually leaving the full palette completely with Shift - I was experimenting only with moving about within the palette, because that's when the problem was occurring.

leaveEvent is, as far as I can tell, is called differently for drag than for ordinary mouse movement. If you are dragging, it gets called almost immediately on start drag - potentially even before called on leaving the current cell. Maybe it's after the motion exceeds the QApplication::startDragDistance()? But when not dragging, it is only called when you leave the palette completely.

I have no idea why this difference exists, and suspect that's some sort of bug in itself, but I'd be much more afraid of trying to change anything about how the events are triggered, and reluctant to bother given that palette is in the process of being reimplemented anyhow.

But, maybe I can refine this bit of code to still clear currentIdx when you are completely leaving the palette, now that I see how to trigger that glitch.

@MarcSabatella
Copy link
Contributor Author

MarcSabatella commented Jun 27, 2019

We don't maintain a lot of state, so I don't see a direct way to differentiate the cases here. Adding a check on dragIdx (if it's -1, we're not dragging, so it's safe to clear currentIdx) helps a little but not a lot, as this never gets cleared if you've ever dragged anything. Rather than try to sort that out and risk breaking something, I'd rather just live with the glitch where an element still looks selected if you hold Shift while leaving the palette (which there is normally no reason to do anyhow). Seems a very small price to pay to regain the ability to reorder palettes.

@dmitrio95
Copy link
Contributor

OK, if it is not something trivial to fix I believe it can indeed be left as it is.

@dmitrio95 dmitrio95 merged commit edc39cf into musescore:master Jun 28, 2019
anatoly-os pushed a commit that referenced this pull request Jun 28, 2019
fix #281362: allow shift+drag to rearrange palette
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

2 participants