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][UX] Add accept/close keyboard controls to all dialogs #1610

Merged
merged 4 commits into from May 13, 2020

Conversation

random-zebra
Copy link

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:

ambassador000
ambassador000 previously approved these changes May 11, 2020
Copy link

@ambassador000 ambassador000 left a comment

Choose a reason for hiding this comment

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

Functionality tested, working as intended.

to be extended by all QDialog child classes that are meant to be focused
on show and need keyboard control (enter/esc) to accept/close.
- AddNewContactDialog (closes PIVX-Project#1606)
- MasterNodeWizardDialog
- MnInfoDialog
- ReceiveDialog
- RequestDialog
- SendChangeAddressDialog
- SendCustomFeeDialog
@random-zebra
Copy link
Author

Rebased for conflicts after merge of #1604

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.

Concept ACK, code looking good too 👍 .

Left two comments, only the nextClicked one is little bit confusing at the code readability level.

@@ -37,10 +36,10 @@ class TxDetailDialog : public QDialog
void setDisplayUnit(int unit){this->nDisplayUnit = unit;};

public Q_SLOTS:
void acceptTx();
void accept() override;
void close();
Copy link

Choose a reason for hiding this comment

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

isn't close() an override too?

Copy link
Author

Choose a reason for hiding this comment

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

I thought so too, but the compiler complained.
That's probably because the QWidget::close() function (inherited by QDialog) returns a boolean value, not void.
Actually, have a point of discussion about this: I think it would be better to not use close() at all here, but rather resort to QDialog::reject() and override that one.

Copy link

@furszy furszy May 11, 2020

Choose a reason for hiding this comment

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

yeah but we should double check something. The reject method is the one that sets the return value to rejected but it hides the dialog, it doesn't close it.
QT documentation is not clear over the difference between hide and close.

Copy link
Author

Choose a reason for hiding this comment

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

both accept and reject call QDialog::done() and Qt docs state:

If this dialog is shown with exec(), done() also causes the local event loop to finish, and exec() to return r.

As with QWidget::close(), done() deletes the dialog if the Qt::WA_DeleteOnClose flag is set. If the dialog is the application's main widget, the application terminates. If the dialog is the last window closed, the QApplication::lastWindowClosed() signal is emitted.

Copy link

Choose a reason for hiding this comment

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

k then, all good 🕺

Copy link
Author

Choose a reason for hiding this comment

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

Added 26b5dc2 with the change

src/qt/pivx/masternodewizarddialog.cpp Show resolved Hide resolved
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.

Looks good

ACK 26b5dc2

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.

Tested ACK 26b5dc2 and merging.

@furszy furszy merged commit 9ac68cf into PIVX-Project:master May 13, 2020
@random-zebra random-zebra deleted the 202005_GUI_focuseddialog branch September 24, 2020 00:28
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.

[Bug] Edit Address Label dialog does not accept ENTER keyboard button press
4 participants