search

tibirna / qgit

Official git repository for QGit.
Other
175 stars 68 forks source link

Ignore mac folder prefs, fix tab focus on find/'end of document' messagebox #92

Open NucleaPeon opened 4 years ago

NucleaPeon commented 4 years ago

When using option+f to find content in a diff, when it prompts to search from the beginning of the file, I am unable to:

  1. press the space key to act on the hightlighted button
  2. use the tab key to move between buttons
  3. use the arrow keys to move between buttons

I also noticed that you use the bold heading text for the entire message, which, I'm ambivalent about.

I've created a branch that fixes the focus for me. It's likely a mac os x issue, an older one at that. I'll attach a screenshot momentarily of what it looks like.

Signed-off-by: Daniel Kettle initial.dann@gmail.com

NucleaPeon commented 4 years ago

endofdocreached

I set a fixed base size because it was wrapping text and I wanted to mirror your look and feel of having it in one line. Feel free to push back if you don't want it, I'll do my best to make it perfect.

tibirna commented 4 years ago

Thanks a lot for this refinement.

Why is the QMessageBox created without the 'this' parent?

mvf commented 4 years ago

FWIW, IMHO this PR might be trying to solve the wrong problem. Native macOS message boxes don't support focus switching either, so it doesn't seem sensible to add for this one message box only.

Focus doesn't even seem to be the issue here, but rather: Yes/No should be conveniently selectable with the keyboard. On macOS, the only way I found to achieve this is changing the default from No to Yes. The user can then press Enter for Yes and Esc for No. Yes is arguably the better default here anyway: At worst the user gets an additional error message. At best they unexpectedly catch that BEGIN OPENSSH PRIVATE KEY they almost pushed.

Other issues with this change in its current form:

All the above combined, my take on this would be:

@@ -2110,8 +2110,8 @@ void MainImpl::ActFindNext_activated() {
            return;
        }
        if (QMessageBox::question(this, "Find text - QGit", "End of document "
-           "reached\n\nDo you want to continue from beginning?", QMessageBox::Yes,
-           QMessageBox::No | QMessageBox::Escape) == QMessageBox::No)
+           "reached.\n\nDo you want to continue from the beginning?", QMessageBox::Yes | QMessageBox::No,
+           QMessageBox::Yes) == QMessageBox::No)
            return;

        endOfDocument = true;

Here's what it looks like on Catalina: msgbox_mac_raw

NucleaPeon commented 4 years ago

@tibirna

Thanks a lot for this refinement.

Why is the QMessageBox created without the 'this' parent?

Hmmm, I tried using msgBox.setParent(this); initially and it didn't work, the popup wouldn't ever show. However, QMessageBox msgBox(this); works, so I will update my pull request with that.

@mvf

missing parent

I am on Mac OS X 10.6.8. I tried defaulting to Yes as per your code, but I am still unable to tab to a different option, which is the point of this request. The parent problem has been solved as per above.

informative text

In the QT 5.3.2 docs, the entry for setInformativeText says:

This property holds the informative text that provides a fuller description for the message. Inf[or]mative text can be used to expand upon the text() to give more information to the user. On the Mac, this text appears in small system font below the text(). On other platforms, it is simply appended to the existing text.

I feel that the informative text matches the criteria set out by the documentation and how Mac handles that is how the designers meant it to be. I think we're both in agreement that the bold text doesn't look right; it's meant to emphasize and when everything is emphasized, nothing is and it fails as a UI cue. I understand that setting the size is not desirable, but I'll upload images of with it sized and with it not sized and let you be the judge. (If not giving it a size is what makes or breaks this merge, I'm fine with either.)

With Size:

window-with-size

Default, no size

window-with-no-size

Lastly, I only changed this instance of the dialog box because it was my immediate issue, and my changes to the QMessageBox went from a few lines to 8 lines and if it wasn't a desirable change, I didn't want to spend more time on it. One word from you and I'll fix up any other instances, I'll push my parent change, and then wait for the go-ahead or feedback.

I appreciate you guys taking the time to view my changes seriously and help me to better the code. Honestly.

mvf commented 4 years ago

I am still unable to tab to a different option, which is the point of this request.

IMHO apps shouldn't casually override platform conventions. Tab focusing is not a thing in macOS message boxes, so why add it to this one in particular? I'm also not sure why one would prefer Tab + a second keystroke when there's just two options. With the Yes-default one can just hit Enter or Esc.

I think we're both in agreement that the bold text doesn't look right

Sure, but before putting form before semantics, maybe we should consider redesigning the whole message: It could be that it looks bad here mainly because of the new paragraph. Native Apple alerts don't seem to have these in their "Message" element. But they do have the question in there: https://developer.apple.com/design/human-interface-guidelines/macos/windows-and-views/alerts/

NucleaPeon commented 4 years ago

IMHO apps shouldn't casually override platform conventions

This got me thinking actually. I did a bit of searching and found that tabbing between buttons in a prompt is something that OS X disables by default. https://osxdaily.com/2010/02/26/use-the-tab-key-to-switch-between-dialog-buttons-in-mac-os-x/

@mvf You are correct that it is a platform convention and by turning on the setting, I am able to tab between buttons. That means that setting Strong Focus isn't needed and I can revert that. However, it would be nice to consolidate and style all the message prompts through a internal factory method. I could work on that.