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 #275479: Master palette doesn't close on ESC #3885

Conversation

jthistle
Copy link
Contributor

@lasconic
Copy link
Contributor

lasconic commented Aug 21, 2018

The other dialogs do not implement keyPress so it's probably not necessary to do it here. How is the master palette different from other?

@jthistle
Copy link
Contributor Author

Master Palette is not a dialog. It's a widget with the Qt::Dialog flag, which means it's styled like a dialog. It also means it doesn't have the default escape = close keypress event that all dialogs have, since it inherits the keypress events of a QWidget.

@lasconic
Copy link
Contributor

Indeed, same for synthcontrol, playpanel and mixer. The implementation of KeypressEvent should be exactly the same than in these classes then.

@jthistle jthistle force-pushed the 275479-master-palette-doesnt-close-on-esc-linux branch from 6e4bab6 to 926f215 Compare August 21, 2018 09:01
@jthistle
Copy link
Contributor Author

From mixer.cpp, in keyPressEvent:

if (ev->key() == Qt::Key_Escape && ev->modifiers() == Qt::NoModifier) 
      close();
      return;
}
QWidget::keyPressEvent(ev);

I'll change the keyPressEvent code in masterpalette.cpp to match this for consistency.

@jthistle jthistle force-pushed the 275479-master-palette-doesnt-close-on-esc-linux branch from 926f215 to 3e79402 Compare August 21, 2018 09:06
@IsaacWeiss
Copy link
Contributor

While we're looking at this: I've gotten used to having to press Esc, but I still wish that Ctrl+W would close these widgets when they are open instead of the score beneath.

@anatoly-os
Copy link
Contributor

@IsaacWeiss let's discuss your suggestion in the related issue. Do we have one?

@anatoly-os anatoly-os merged commit ad78d4e into musescore:master Aug 24, 2018
@jthistle jthistle deleted the 275479-master-palette-doesnt-close-on-esc-linux branch August 24, 2018 10:53
@anatoly-os
Copy link
Contributor

@jthistle looks like this PR leads to https://musescore.org/en/node/275694. Could you please have a look?

@jthistle jthistle restored the 275479-master-palette-doesnt-close-on-esc-linux branch August 28, 2018 15:19
@jthistle
Copy link
Contributor Author

I'd be surprised if it did lead to that issue, but I'll take a look anyway.

@jthistle jthistle deleted the 275479-master-palette-doesnt-close-on-esc-linux branch August 28, 2018 15:20
@jthistle jthistle restored the 275479-master-palette-doesnt-close-on-esc-linux branch August 28, 2018 15:21
@jthistle jthistle deleted the 275479-master-palette-doesnt-close-on-esc-linux branch August 28, 2018 15:24
@anatoly-os
Copy link
Contributor

To be honest, I didn't check it yet, but this PR looks the most suspicious :)

@jthistle
Copy link
Contributor Author

I have no idea how changing masterpalette.cpp would break the start center, but I'll see if I can get ESC working. I'd blame @handrok (if anyone, sorry ;)), because their changes were added August 4th, and you were testing with the August 2nd nightly.

@anatoly-os
Copy link
Contributor

anatoly-os commented Aug 28, 2018

Maybe :)
I will test thoroughly, could be a coincidence, indeed. Sorry for distraction.

@jthistle
Copy link
Contributor Author

No problem, I'm working on a fix for it :)

@jthistle
Copy link
Contributor Author

Fixed it, made a pr: #3904

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

4 participants