korgac: Add an option to not grab keyboard focus when a reminder is displayed
ClosedPublic

Authored by dfries on Oct 16 2018, 3:32 AM.

Details

Summary

While it can make it easier to dismiss (if it grabs focus through the
activateWindow call), it also means if you are typing and a reminder
comes in it will both get those keystrokes and a space or return will
dismiss the reminder before you have a chance to ever see what the
reminder was about, which assumes you are even looking at your screen
to see the reminder box flash up, if you are typing in a paper you
might not even know.

Test Plan

Setup two notifications in korganizer one minute apart.
I the dock window menu test once with "Reminder grabs focus" enabled and once with it disabled.
Give focus to some other program before the first alarm and second alarm, don't dismiss the reminder box between alarms.
With "Reminder grabs focus" checked, the Reminders window should get focus on the second reminder, (this is also the behavior before this change).
With "Reminder grabs focus" unchecked, the Reminders window should not get focus on the second reminder.

Note I'm writing this up for two reminders to avoid the case of window managers which automatically give focus on every new window that pops up. That's a policy choice on the user/window manager and if this did something to not get focus in that case, this would be going against that policy.

Diff Detail

Repository
R210 KOrganizer
Lint
Lint Skipped
Unit
Unit Tests Skipped
dfries created this revision.Oct 16 2018, 3:32 AM
Restricted Application added a project: KDE PIM. · View Herald TranscriptOct 16 2018, 3:32 AM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
dfries requested review of this revision.Oct 16 2018, 3:32 AM
dvratil requested changes to this revision.Oct 19 2018, 11:51 AM
dvratil added a subscriber: dvratil.
dvratil added inline comments.
korgac/alarmdockwindow.cpp
47

const, grabFocus

96

Maybe add a tooltip with explanation what this really means - "Reminder grabs focus" is quite technical

98

Use the new connect method (via method pointers)

156

Remove this.

Or add some more context - in release builds the method name is not known, so this just prints "<uknown>" to the log

159

No need to explicitly sync

This revision now requires changes to proceed.Oct 19 2018, 11:51 AM
dfries updated this revision to Diff 43957.Oct 20 2018, 2:17 AM
dfries marked 5 inline comments as done.
dfries updated this revision to Diff 43967.Oct 20 2018, 1:01 PM

const bool grabfocus
different menu label, added tooltip, enabled tooltips
used newer connect style
removed qCDebug, config.sync();

dfries added inline comments.Oct 20 2018, 1:03 PM
korgac/alarmdockwindow.cpp
96

How does this sound?
menu item: "Reminder Requests Focus"

tooltip: "Enable when you want the reminder dialog to request keyboard focus when a notification appears."
or
"Enable for the reminder dialog to request keyboard focus when a notification appears."
or
"Enable to automatically get keyboard focus when a notification appears."

menus disable tooltips by default, adding:
contextMenu()->setToolTipsVisible(true);

I'd be glad to redo this patch to not include the menu option and not grab focus. That would simplify things. I just figured as something went out of their way to add activateWindow, that wouldn't to over, so I made it optional.

https://blog.codinghorror.com/please-dont-steal-my-focus/

dvratil requested changes to this revision.Oct 21 2018, 11:23 PM

Let's just improve the wording a little, rest of the code looks OK now.

Oh and rename the grabfocus variable to grabFocus, please.

korgac/alarmdockwindow.cpp
96

Usually, the wording of tooltips is what happens when you use/enable it, rather than "use/enable it to achieve something", so "When this option is enabled the reminder dialog will automatically receive keyboard focus when it opens." is better IMHO.

I haven't realized this is a menu action, not a checkbox somewhere in a settings dialog, but I think the tooltip does make sense in this case and is OK.

Having this as an option is perfectly OK. We like having options :-)

This revision now requires changes to proceed.Oct 21 2018, 11:23 PM
dfries updated this revision to Diff 44053.Oct 22 2018, 1:40 AM

grabFocus, tooltip message you posted

dvratil accepted this revision.Oct 22 2018, 7:59 PM
This revision is now accepted and ready to land.Oct 22 2018, 7:59 PM
This revision was automatically updated to reflect the committed changes.