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

Change QDialog Close button from RejectRole to AcceptRole #7784

Open
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@chargeflux
Copy link

chargeflux commented Feb 4, 2019

This PR fixes https://bugs.dolphin-emu.org/issues/11184. While it says it affects macOS, Windows is also affected.

Because the Settings , Controllers and Mapping windows are QDialog classes, the first QPushButton added seems to be automatically designated as "default." This behavior is unique to QDialog classes. A "default" QPushButton means if the user press enter, it will automatically activate. This akin to a "Open File" Dialog and pressing enter to "press" the blue OK button.

This issue can be seen for example, in the first tab, General, when opening Config window where the "Generate a New Statistics Identity" button appears as blue and will activate if the user press enter. Or the first Configure Button when opening the Controllers window as described in the above issue at the issue tracker.

This PR changes the "Close" button (contained in QDialogButtonBox) for each of the QDialog windows to having the AcceptRole instead of the RejectRole. This prioritizes the "Close" Button as the default button and will activate if the user uses "Enter" as per the documentation:

However, if there is no default button set and to preserve which button is the default button across platforms when using the QPushButton::autoDefault property, the first push button with the accept role is made the default button when the QDialogButtonBox is shown.

This will prevent all QPushButtons in a dialog window from being "default" and from activating upon the user pressing Enter; instead the "Close" button will activate. Any QPushButton that wants to be "default" instead of the "Close" button will simply need to be set as default after their creation: setDefault(true).

This PR so far only fixes the Config, Controller and Mapping windows. Likely any QDialog window that has a "Close" Button should be changed to having an AcceptRole so that the QPushButtons in those windows or future QPushButtons aren't considered "default" just because they come before the Close Button. I can add a second commit to cover those cases (or amend and force-push).

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Feb 4, 2019

Nice job. This looks like the proper way to fix the issue.

@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Feb 5, 2019

Does this mean "Enter" will close the dialog? I'm not sure if thats a good idea. While random buttons certainly shouldn't be a default accept button, I don't think a button labelled "Close" should be an accepting default either (and stick to reject, which makes more sense in my head; especially since it maps to "Escape").

Can we opt to not set a default button at all? Or is that not possible with a QDialog?

@chargeflux

This comment has been minimized.

Copy link
Author

chargeflux commented Feb 5, 2019

Yes. Hitting Enter would activate the "Close" button.

All QPushButtons created in a QDialog have the property autoDefault set to true. This means that the first QPushButton in the layout will automatically be "default". If you don't have a button in a QDialogButtonBox (or no QDialogButtonBox for that matter) explicitly with QDialogButtonBox::AcceptRole, the first QPushButton in the layout will be the default.

If setting the "Close" button as default is not desired, then the solution is to set the property "autoDefault" for every QPushButton created in QDialog as false. This means QPushButtons created in individual panes that is called from a QDialog class (like SettingsWindow). Using findChildren and changing the property through a loop is an option but more restrictive since it is essentially all-or-nothing. Next, the buttons in QDialogButtonBox also must be set to false for autoDefault, i.e., "Close".

When you say reject, do you mean RejectRole or &QDialogButtonBox::rejected? The latter function can only be used with RejectRole, not AcceptRole. The button doesn't work if you combine AcceptRole and &QDialogButtonBox::rejected.

@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Feb 5, 2019

I honestly don't know. I'm mostly coming from the UX angle about making sense, I know next to nothing about how Qt handles those things (apart from few things that I see every now and then in the docs).

It probably is better than the current solution (where hitting "Enter" would do...something); and I don't necessarily want to block this from being merged if we'd rather settle on a consistent solution.

@chargeflux

This comment has been minimized.

Copy link
Author

chargeflux commented Feb 5, 2019

From a macOS perspective, most settings window don't usually have a OK or Close button. On Windows, it is different and I think the "OK" button is usually receptive to Enter for Dialog windows. Here is the style guide on Dialog Boxes. As a side note, Microsoft recommends "OK" for setting windows, not "Close" in that style guide.

It is partially up to code preference I suppose. If you don't intentionally want to have the close button (assuming it is necessary and present on every QDialog window) to be default, then one just has to modify the "autoDefault" property for every relevant QPushButton instance created, modifying multiple files and also remember for new QPushButtons.

These are the remaining classes that are affected:
Properties
Cheats Manager
GC Memcard
NetPlay

MD5Dialog.cpp from Netplay and the NewBreakpointDialog.cpp might be affected but I don't know how they look and what buttons have focus.

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Feb 5, 2019

I think Enter=Close is sensible for a dialog where our "Close" button is essentially an "OK" or "Apply" button. (e.g. Settings dialog, Input dialog, etc).

Some dialogs (like Netplay, TAS or Debugger) might not make sense to close with Enter.

I suspect we can work around the auto-default issue by creating an invisible button with no action and making it the default? Hide this behind a utility function of course. (This is only if the dialog has no sensible Enter/Default action)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment