QT GUI: close window with cmd-w on macos #4821
Conversation
dyfer
added
os: macOS
comp: Qt GUI
There was a problem hiding this comment.
general approach looks OK but please document this by adding to the comment above event->accept().
there's an issue i noticed -- combos like Cmd+Alt+W are not being filtered correctly, they don't close the window (good) but also cause a 'bad input' alert sound (bad).
|
thanks @brianlheim for looking at this, I was hoping for your input. |
dyfer
force-pushed
the
topic/cmd-w-close-window-macos
branch
2 times, most recently
from
eb7253e to
3488bae
Compare
|
I addressed the issue with combo input. I also added comments. How does it look now? I rebased this PR on top of |
this is a workaround for a workaround introduced in supercollider#4105, which fixed supercollider#4058, but caused supercollider#4309
dyfer
force-pushed
the
topic/cmd-w-close-window-macos
branch
from
3488bae to
28682e6
Compare
|
@dyfer changed the base to 3.11, this can go into 3.11.1 |
i'm still thinking about it, would prefer to have just one |
QtCollider/QcApplication.cpp
Outdated
| // of doing this. | ||
| // In order to still allow closing GUI windows with "cmd-w", we do not accept modifier keys | ||
| // alone, as well as the "cmd-w" combination itself, so that they propagate further | ||
| // (see isKeyEvent and isCloseEvent above). |
There was a problem hiding this comment.
from this comment, i still don't understand why you filter out modifier keys alone.
|
I tried to clarify the comment, I hope it's more clear now? I also tried to put it in a single |
There was a problem hiding this comment.
thanks @dyfer . i prefer the single #ifdef block. this is not a performance critical section, it's fine in that regard.
i'm noting we're now only accepting key events. i think this is OK, maybe even more in line with what i was trying to do here before
QtCollider/QcApplication.cpp
Outdated
| if ((kevent->key() == Qt::Key_W) && (kevent->modifiers() == Qt::ControlModifier)) { | ||
| isCloseEvent = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
you can remove the local variables and place all this logic inside the if below, since it only matters if(result). you can also condense these two checks into a single if. i am guessing you split it to make this clearer, but in my opinion keeping the logic condensed and having an explanatory comment is much nicer:
if (result and key press type) {
static cast
if (is not close and is not unknown) {
accept()
}
}
QtCollider/QcApplication.cpp
Outdated
| bool isCloseEvent = false; | ||
| bool isKeyEvent = false; | ||
| if (event->type() == QEvent::KeyPress) { | ||
| QKeyEvent* kevent = static_cast<QKeyEvent*>(event); |
There was a problem hiding this comment.
it's better to use auto when it doesn't obscure the type, and especially when the type is already known from the right hand side of an initialization (for example a cast or obvious function name). this keeps code concise and makes it easier to read and maintain.
|
Thank you for the feedback and suggestions @brianlheim, I simplified the logic. |
Purpose and Motivation
This is a workaround for a workaround introduced in #4105 (which fixed #4058, but caused #4309). This fixes #4309.
It's not pretty, but it works. I hope that at some point someone looks into a proper fix that's hacked in #4105, but until then it would be great to have the ability to close windows from the keyboard again...
Types of changes
To-do list