Skip to content

Qt: Don't crash when pressing the Return key#6099

Merged
leoetlino merged 1 commit intodolphin-emu:masterfrom
leoetlino:activate
Oct 11, 2017
Merged

Qt: Don't crash when pressing the Return key#6099
leoetlino merged 1 commit intodolphin-emu:masterfrom
leoetlino:activate

Conversation

@leoetlino
Copy link
Copy Markdown
Member

@leoetlino leoetlino commented Oct 4, 2017

Use the "activated" signal instead of manually detecting key presses.
Handles both double clicks and Return presses, and fixes a bug in the
logic which could cause GameSelected to be emitted without any game.

@leoetlino leoetlino added this to the Qt milestone Oct 5, 2017
@ligfx
Copy link
Copy Markdown
Contributor

ligfx commented Oct 7, 2017

On macOS, this no longer handles Return presses (instead activated binds to +O, which is used in e.g. Finder).

@Helios747
Copy link
Copy Markdown
Contributor

Helios747 commented Oct 7, 2017

I love this bug.

@leoetlino
Copy link
Copy Markdown
Member Author

leoetlino commented Oct 7, 2017

Ugh, looks like Qt item views don't emit the activated signal at all on macOS...

https://code.qt.io/cgit/qt/qtbase.git/tree/src/widgets/itemviews/qabstractitemview.cpp?h=5.5#n2389

#ifdef Q_OS_MAC
    case Qt::Key_Enter:
    case Qt::Key_Return:
        // Propagate the enter if you couldn't edit the item and there are no
        // current editors (if there are editors, the event was most likely propagated from it).
        if (!edit(currentIndex(), EditKeyPressed, event) && d->editorIndexHash.isEmpty())
            event->ignore();
        break;
#else
    case Qt::Key_Enter:
    case Qt::Key_Return:
        // ### we can't open the editor on enter, becuse
        // some widgets will forward the enter event back
        // to the viewport, starting an endless loop
        if (state() != EditingState || hasFocus()) {
            if (currentIndex().isValid())
                emit activated(currentIndex());
            event->ignore();
        }
        break;
#endif

@leoetlino
Copy link
Copy Markdown
Member Author

leoetlino commented Oct 7, 2017

Changed the fix to just check whether a game is selected or not before emitting the signal.

@leoetlino leoetlino merged commit 53ccd41 into dolphin-emu:master Oct 11, 2017
@leoetlino leoetlino deleted the activate branch October 11, 2017 09:32
gshakhn added a commit to gshakhn/keepassxc that referenced this pull request Sep 26, 2018
@gshakhn
Allow Enter key to select recent DB on OS X
d6a7c1b 
* Override `keyPressEvent`on WelcomeWidget on OS X to `openDatabaseFromFile`.

openDatabaseFromFile is already invoked via the QListWidget::itemActivated signal,
  but this signal doesn't fire on OS X for Enter.
QListWidget::itemActivated activates on an OS specific activation key. [1]
On Windows/X11, this is Enter, which lets the user easily
  navigate with just the keyboard.
On OS X, this is Ctrl+O, which is already bound to Open Database. This means that itemActivated cannot fire via the keyboard.

Per StackOverflow [2], the recommended solution is to catch
the enter/return key press manually.

This seems like a common problem with Qt. [3] [4]

[1] https://doc.qt.io/archives/qt-4.8/qlistwidget.html#itemActivated
[2] https://stackoverflow.com/questions/31650780/when-does-a-qtreeview-emit-the-activated-signal-on-mac
[3] https://forum.qt.io/topic/36147/pyside-itemactivated-not-triggered-on-mac-os-x-with-return-key
[4] dolphin-emu/dolphin#6099
gshakhn added a commit to gshakhn/keepassxc that referenced this pull request Sep 26, 2018
@gshakhn
Allow Enter key to select recent DB on OS X
f0716d3 
* Override `keyPressEvent`on WelcomeWidget on OS X to `openDatabaseFromFile`.

openDatabaseFromFile is already invoked via the QListWidget::itemActivated signal,
  but this signal doesn't fire on OS X for Enter.
QListWidget::itemActivated activates on an OS specific activation key. [1]
On Windows/X11, this is Enter, which lets the user easily
  navigate with just the keyboard.
On OS X, this is Ctrl+O, which is already bound to Open Database. This means that itemActivated cannot fire via the keyboard.

Per StackOverflow [2], the recommended solution is to catch
the enter/return key press manually.

This seems like a common problem with Qt. [3] [4]

[1] https://doc.qt.io/archives/qt-4.8/qlistwidget.html#itemActivated
[2] https://stackoverflow.com/questions/31650780/when-does-a-qtreeview-emit-the-activated-signal-on-mac
[3] https://forum.qt.io/topic/36147/pyside-itemactivated-not-triggered-on-mac-os-x-with-return-key
[4] dolphin-emu/dolphin#6099
@gshakhn gshakhn mentioned this pull request Sep 27, 2018
Merged
droidmonkey pushed a commit to gshakhn/keepassxc that referenced this pull request Dec 25, 2018
@gshakhn @droidmonkey
Allow Enter key to select recent DB on OS X
129c2cd 
* Override `keyPressEvent`on WelcomeWidget on OS X to `openDatabaseFromFile`.

openDatabaseFromFile is already invoked via the QListWidget::itemActivated signal,
  but this signal doesn't fire on OS X for Enter.
QListWidget::itemActivated activates on an OS specific activation key. [1]
On Windows/X11, this is Enter, which lets the user easily
  navigate with just the keyboard.
On OS X, this is Ctrl+O, which is already bound to Open Database. This means that itemActivated cannot fire via the keyboard.

Per StackOverflow [2], the recommended solution is to catch
the enter/return key press manually.

This seems like a common problem with Qt. [3] [4]

[1] https://doc.qt.io/archives/qt-4.8/qlistwidget.html#itemActivated
[2] https://stackoverflow.com/questions/31650780/when-does-a-qtreeview-emit-the-activated-signal-on-mac
[3] https://forum.qt.io/topic/36147/pyside-itemactivated-not-triggered-on-mac-os-x-with-return-key
[4] dolphin-emu/dolphin#6099
droidmonkey pushed a commit to keepassxreboot/keepassxc that referenced this pull request Dec 25, 2018
@gshakhn @droidmonkey
Allow Enter key to select recent DB on OS X
b1ff346 
* Override `keyPressEvent`on WelcomeWidget on OS X to `openDatabaseFromFile`.

openDatabaseFromFile is already invoked via the QListWidget::itemActivated signal,
  but this signal doesn't fire on OS X for Enter.
QListWidget::itemActivated activates on an OS specific activation key. [1]
On Windows/X11, this is Enter, which lets the user easily
  navigate with just the keyboard.
On OS X, this is Ctrl+O, which is already bound to Open Database. This means that itemActivated cannot fire via the keyboard.

Per StackOverflow [2], the recommended solution is to catch
the enter/return key press manually.

This seems like a common problem with Qt. [3] [4]

[1] https://doc.qt.io/archives/qt-4.8/qlistwidget.html#itemActivated
[2] https://stackoverflow.com/questions/31650780/when-does-a-qtreeview-emit-the-activated-signal-on-mac
[3] https://forum.qt.io/topic/36147/pyside-itemactivated-not-triggered-on-mac-os-x-with-return-key
[4] dolphin-emu/dolphin#6099
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spycrab spycrab spycrab approved these changes

+1 more reviewer

@ligfx ligfx ligfx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

Qt

Development

Successfully merging this pull request may close these issues.

4 participants

@leoetlino @ligfx @Helios747 @spycrab