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

[GUI] Accept dialogs with ENTER, reject with ESC #1392

Merged
merged 4 commits into from
Mar 20, 2020

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Mar 9, 2020

Ref: #1163 (review)

Completes the ability to accept/dismiss modal dialogs with the keyboard, including:

  • confirmation dialogs (DefaultDialog)
  • tx detail / tx confirmation dialogs (TxDetailDialog)

@random-zebra random-zebra added this to the 4.1.0 milestone Mar 9, 2020
@random-zebra random-zebra self-assigned this Mar 9, 2020
Fuzzbawls
Fuzzbawls previously approved these changes Mar 9, 2020
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good QOL improvement.

utACK d88eaef

src/qt/pivx/sendconfirmdialog.cpp Outdated Show resolved Hide resolved
src/qt/pivx/sendconfirmdialog.cpp Outdated Show resolved Hide resolved
@furszy
Copy link

furszy commented Mar 20, 2020

Alternative implementation to the grabKeyboard and releaseKeyboard. Just override QDialog::keyPressEvent virtual method and set focus over the dialog when it's being shown.

Tested changes patch (did it only over one dialog, can easily be applied to -squashed into- each of the previous commits):

Index: src/qt/pivx/defaultdialog.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/qt/pivx/defaultdialog.h	(date 1584632051000)
+++ src/qt/pivx/defaultdialog.h	(date 1584717124293)
@@ -19,6 +19,8 @@
     explicit DefaultDialog(QWidget *parent = nullptr);
     ~DefaultDialog();
 
+    void showEvent(QShowEvent *event) override;
+
     void setText(QString title = "", QString message = "", QString okBtnText = "", QString cancelBtnText = "");
 
     bool isOk = false;
@@ -29,7 +31,7 @@
 private:
     Ui::DefaultDialog *ui;
 protected:
-    bool event(QEvent* event) override;
+    void keyPressEvent(QKeyEvent *e) override;
 };
 
 #endif // DEFAULTDIALOG_H
Index: src/qt/pivx/defaultdialog.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/qt/pivx/defaultdialog.cpp	(date 1584632051000)
+++ src/qt/pivx/defaultdialog.cpp	(date 1584717244585)
@@ -38,7 +38,11 @@
     connect(ui->btnEsc, SIGNAL(clicked()), this, SLOT(close()));
     connect(ui->btnCancel, SIGNAL(clicked()), this, SLOT(close()));
     connect(ui->btnSave, &QPushButton::clicked, this, &DefaultDialog::accept);
-    grabKeyboard();
+}
+
+void DefaultDialog::showEvent(QShowEvent *event)
+{
+    setFocus();
 }
 
 void DefaultDialog::setText(QString title, QString message, QString okBtnText, QString cancelBtnText)
@@ -60,7 +64,7 @@
     QDialog::accept();
 }
 
-bool DefaultDialog::event(QEvent* event)
+void DefaultDialog::keyPressEvent(QKeyEvent *event)
 {
     if (event->type() == QEvent::KeyPress) {
         QKeyEvent* ke = static_cast<QKeyEvent*>(event);
@@ -69,11 +73,9 @@
         // Detect Esc key press
         if (ke->key() == Qt::Key_Escape) close();
     }
-    return QDialog::event(event);
 }
 
 DefaultDialog::~DefaultDialog()
 {
-    releaseKeyboard();
     delete ui;
 }

@random-zebra
Copy link
Author

random-zebra commented Mar 20, 2020

Applied @furszy's patch to TxDetail and Default dialogs. Squashed / rebased on master.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 37e73fe

@furszy furszy merged commit da2f6e5 into PIVX-Project:master Mar 20, 2020
akshaynexus added a commit to ZENZO-Ecosystem/ZENZO-Core that referenced this pull request Mar 22, 2020
… ESC

37e73fe [Doc] Add GUI dailogs keyboard navigation to the release notes (random-zebra)
52157ce [GUI] Guard access to TxDetailDialog::acceptTx (random-zebra)
7ec6c13 [GUI] Set focus in DefaultDialog and detect Enter/Esc (random-zebra)
436fea2 [GUI] Set focus in TxDetailDialog and detect Enter/Esc (random-zebra)

Pull request description:

  Ref: PIVX-Project#1163 (review)

  Completes the ability to accept/dismiss modal dialogs with the keyboard, including:
  - confirmation dialogs (`DefaultDialog`)
  - tx detail / tx confirmation dialogs (`TxDetailDialog`)

ACKs for top commit:
  furszy:
    ACK 37e73fe

Tree-SHA512: 0d0771e8b9891a5d3555c835564feadee2dd4f9d23b0733361e9279448f559eee1865369693d428e210c40f48ad80c544f757c4ec01cdf6f7f57a999e3e2a056
furszy added a commit that referenced this pull request May 13, 2020
26b5dc2 [GUI][Refactor] Dismiss dialogs with QDialog::reject vs QWidget::close (random-zebra)
7cb8107 [GUI] Make more dialogs inherit from FocusedDialog (random-zebra)
9acda36 [GUI] Make DefaultDialog and TxDetailDialog child of FocusedDialog (random-zebra)
bc579f7 [GUI] Introduce FocusedDialog generic class (random-zebra)

Pull request description:

  This creates a generic extension of QDialog, named `FocusedDialog`, with the property of having the focus on show and filtering key press events to call `accept()` with ENTER/RETURN and `close()` with ESC (ref: #1392).
  Removes code duplication in `DefaultDialog` and `TxDetailDialog` by making them child classes of FocusedDialog.
  Also add this ability to other dialogs as well:
  - `AddNewContactDialog` (closes #1606)
  - `MasterNodeWizardDialog`
  - `MnInfoDialog`
  - `ReceiveDialog`
  - `RequestDialog`
  - `SendChangeAddressDialog`
  - `SendCustomFeeDialog`

ACKs for top commit:
  Fuzzbawls:
    ACK 26b5dc2
  furszy:
    Tested ACK 26b5dc2 and merging.

Tree-SHA512: 43a8f74969a55e5c9f9119721f94f54b33ba44291c6e06e7603f63fb4d1a959fce8cf17e6e63811e2b550ce0c62e06d5831acdc279de20fe914c97a71c0638d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants