Notification rewrite

[tiennou]

So, this is an alternate version of #2104. Sorry @skurfer, I didn't like the "one delegate to rule them all approach", though as I said earlier, it's because of the API.

This one uses a block to notify what choice the user made. Right now it uses the same kind of block than the alerts do, but depending on some discussion, that can change. I prefer that to moving gobs of important code to unrelated places. I do agree the update code is not the most streamlined code to pick on, but, well...

Something I planned for but didn't yet handle is automatically escalating notifications to alerts via the delegate callback (that's why I started on cleaning up QSAlertManager first, then proceeded to notification handling). The issue is that I can't think of a good way of not doing so without changing the block's signature. Maybe provide a third method that has the automatic escalation behavior in ?

[skurfer]

So, this is an alternate version of #2104. Sorry @skurfer, I didn't like the "one delegate to rule them all approach", though as I said earlier, it's because of the API.

No hard feelings on that.

This one uses a block to notify what choice the user made.

Your way certainly looks better for us programmers, but not for users. 😛

I don’t know if I can get behind a notification rewrite that doesn’t use Notification Center at this point.

[tiennou]

Well, I do get your point but don't understand "it doesn't use Notification Center". 411cfb8 adds that support, but I have doubts on the UX part of it, which is why I asked in the PR.

Here are the shortcomings I've thought about :

• if the user has set QS Notification style to banners, you don't get buttons. If the Notifications are disabled, I'm not even sure what happens. I'll check, but this means some notifications could be lost without a fallback mechanism, or will behave incompletely because of missing buttons.
• right now you can either use a notification or use an alert, but not both (at least not automatically). I think there might be some times we want the automatic escalation — a manual update check should use alerts, a scheduled one should use notifications.

Maybe adding a third informWithMessage:... that does the automatic escalation thing, so we get to choose if we want it or not, depending on the context of the alert ?

[skurfer]

411cfb8 adds that support

My mistake. I missed that, and I tried switching the text ranker using this branch and still got a nasty modal alert. Am I correct that you added some methods to support Notification Center, but never actually call them? That’s what it looks like.

On the actual implementation, those are all good points. It really doesn’t seem like an API for the real world. I had the same concerns and thought “Well, I guess this is what people do now.” and plowed forward. Maybe you’re right to question the whole thing.

I really think Notification Center feels nice to use where I’ve seen it pop up (using my branch), but like you said, that all falls apart if the user changes to banners. Are you thinking we should just implement our own thing entirely?

Seems like we need three types:

• The user needs to know about something and make a choice before we proceed
• The user needs to know about something and the message stays on screen until they dismiss it
• The user needs to know about something, but it the message should dismiss itself after a few seconds

Right now, I think QS is too heavy on the first type. A lot of those could probably be changed to the second type, or just moved out of the way to await an answer. Maybe you’ve dine some of that. The best (worst?) example of this is the upgrade process and I haven’t done the work to fake an upgrade using this code. 😄

[tiennou]

Am I correct that you added some methods to support Notification Center, but never actually call them? That’s what it looks like.

Yep, for the sake of discussing first ;-).

that all falls apart if the user changes to banners.

Sadly, I don't think there's a way to get the current notification style, beforehand at least.I vaguely remember it's possible to get a notification style, so it may be possible to post one, see how it looks, and then decide to switch to alerts.

Are you thinking we should just implement our own thing entirely?

Oh my god no ! Wait, we have one already ? What's that spinning cube which tells me there were updated plugins and I should relaunch ;-).

Right now, I think QS is too heavy on the first type. A lot of those could probably be changed to the second type, or just moved out of the way to await an answer. Maybe you’ve dine some of that. The best (worst?) example of this is the upgrade process and I haven’t done the work to fake an upgrade using this code. 😄

I mostly agree with your list, I was just thinking the API should be built around some use cases (which I'm unsure of), before switching everything to it. So, based on your list above, I would do this :

• First is "the user must take action, show an action notification if it's possible (provided we can test for that) AND we're not foreground OR display an alert".
• Third is standard (with API sugar-coating), status report "this was put on the clipboard" notifications (e.g. the Dropbox plugin has those),

I'm leaving out the second kind because I don't like them (thankfully only Apple uses those, see the "App Store update available" which can't be dismissed). Can you give me an example where the user would be absolutely forced to act which wouldn't be covered by the first one ?

[skurfer]

Sadly, I don't think there's a way to get the current notification style, beforehand at least.I vaguely remember it's possible to get a notification style, so it may be possible to post one, see how it looks, and then decide to switch to alerts.

The API was truly not designed for any real-world use.

Can you give me an example where the user would be absolutely forced to act which wouldn't be covered by the first one ?

The only thing I can think of where the user would need to respond immediately is destructive file operations, but that has its own UI.

As for my “second type” above, I didn’t mean the App Store style thing that can’t be dismissed. (I hate it, too.) I’m referring to messages that can be dismissed by the user, but won’t just fade away on their own before the user has a chance to read them. Things like “This won’t work until you relaunch” or “There’s an update available”. I’m thinking this will be the most common type, and I can say from experience that using Notification Center for them feels really nice.

The third kind should continue to work as they do now, where you can choose to send them to Growl, etc.

UPDATE: We especially need to keep Growl and the built-in notifier as options for messages that can go away on their own, because for the others to work at all, we have to use “alerts” (see 7dcaffc) that stay until dismissed. Seriously. Who designed this?

[tiennou]

Some class-dumps and disassembling got involved, and I got this :

I also got some non-dismissable (a.k.a the ones we hate) notifications, and that's mostly it. There is no way to get a banner-style notification when you're set to alert — because ultimately the user gets to choose. The closest you can get looks like the one above (without the shiny star thing), which doesn't auto-dismiss after a while (even though there's also appears to be a way to do so). We can cheat the dismiss part using removeDeliveredNotification: after a short while, for the ones we want as banners, but it feels kinda wrong.

Oh my, I just noticed Mail uses some hybrid "banner with buttons that show on hover" ones !

[tiennou]

After some more disassembling, I can confirm that there are only 2 applications that can tweak their display style : Calendar & Reminders — that's hardcoded in usernoted.

Also note that it looks like the actual display style is derived from the first time we showed a notification, so if something decided to show a notification without us having the alert thing set in our Info.plist, then we're doomed because the only way to get alerts is to kindly ask the user to change the setting in the Notification preference pane — which we can't test for without doing private things, and there is no available API to reset the Notification registry.

So given we have a user-accessible, QSNotificationCenterNotifier, which arguably got used by some amount of our user base (myself included), what do we do ?

EDIT

We are not alone : Here's the defaulty thing demonstrated, here's the "reset" procedure.

[skurfer]

Also note that it looks like the actual display style is derived from the first time we showed a notification, so if something decided to show a notification without us having the alert thing set in our Info.plist, then we're doomed because the only way to get alerts is to kindly ask the user to change the setting in the Notification preference pane

Are you sure that’s still true? Those comments were from 2013. I’m sure I tested QSNotificationCenterNotifier at some point, and I still got the alert behavior after updating our Info.plist. In fact, I know I started posting notifications before I learned about that setting.

[tiennou]

I seem to remember I had the exact opposite happen : running with the Info.plist change, wondering why I'd get banners anyway, and manually changing the setting in Notification Center so it would actually work.

Note that I think the signedness of an application is also taken into account, so maybe running a signed build would allow the style to be changed (as you saw), while an unsigned one wouldn't (as I saw).

[skurfer]

Maybe signing made a difference, but I’m sure I tried it first with a Debug build which wouldn’t be signed.

Is it possible you had manually chosen banners for QS at some point in the past? If so, the OS would probably respect that setting and ignore the app’s default.

[skurfer]

Added some commits here.

The first allows the handler to actually get called. 😃

The second is because responses weren’t really making sense. If you passed in two buttons, like

@[NSLocalizedString(@"Go", nil), NSLocalizedString(@"Stop", nil)]

Clicking “Stop” would do what you think, but clicking “Go” would send QSAlertResponseSecond or QSAlertResponseCancel (according to Xcode). I just mapped the primary action button explicitly to “OK” and the second to “Cancel”, since that’s what they mean to Notification Center.

[tiennou]

10.10+ ?

[skurfer]

Oh, seriously? Fine. Leaving it out entirely doesn’t change behavior.

[tiennou]

The first allows the handler to actually get called. 😃

Such a characteristic bug 😆.

The second is because responses weren’t really making sense.

I think that's because IIRC there's no way to be informed when a notification is dismissed (slide-to-go-away), so you can't really cancel. So clicking the notification itself = first button. But I might not remember, and I might be confused because QSAlertResponseFirst = QSAlertResponseOK. IIRC the OK/Cancel are just aliases for simple yes/noes.

[skurfer]

Seeing a crash with my local copy of QS, but it’s not in 1.5.0. I think it’s a result of some changes on this branch.

To reproduce, move a file to a location where a file with the same name already exists.

Application Specific Backtrace 1:
0   CoreFoundation                      0x00007fff9be867bb __exceptionPreprocess + 171
1   libobjc.A.dylib                     0x00007fffb05f3a2a objc_exception_throw + 48
2   CoreFoundation                      0x00007fff9bf03a65 +[NSException raise:format:] + 197
3   AppKit                              0x00007fff9a2acb44 _NSRunModal + 103
4   AppKit                              0x00007fff99ce83b4 -[NSApplication runModalForWindow:] + 295
5   QSInterface                         0x0000000109326eb3 -[QSFileConflictPanel runModalAsSheetOnWindow:] + 83
6   Core Support                        0x000000010f2e819c Core Support + 33180
7   Core Support                        0x000000010f2e7f0f Core Support + 32527
8   QSCore                              0x000000010926e69c -[QSAction performOnDirectObject:indirectObject:] + 1076
9   QSCore                              0x0000000109276d69 -[QSCommand execute] + 320
10  QSInterface                         0x000000010932dc52 -[QSInterfaceController executeCommandThreaded] + 341
11  libdispatch.dylib                   0x00007fffb0ea1f5f _dispatch_call_block_and_release + 12
12  libdispatch.dylib                   0x00007fffb0e99128 _dispatch_client_callout + 8
13  libdispatch.dylib                   0x00007fffb0ea82ce _dispatch_queue_override_invoke + 743
14  libdispatch.dylib                   0x00007fffb0e9aee0 _dispatch_root_queue_drain + 476
15  libdispatch.dylib                   0x00007fffb0e9acb7 _dispatch_worker_thread3 + 99
16  libsystem_pthread.dylib             0x00007fffb10e5746 _pthread_wqthread + 1299
17  libsystem_pthread.dylib             0x00007fffb10e5221 start_wqthread + 13

[skurfer]

Regarding that crash, I think this branch actually fixes a number of similar crashes. We just need to add the file conflict thing to the main thread. And I think that exception only crashes if ou build with the latest SDK, so might not affect release builds (done from 10.10). Going to test.

[skurfer]

Confirmed. When building from 10.10, this is just a warning:

default	09:40:26.383670 -0500	Quicksilver	-[NSApplication runModalForWindow:] may only be invoked from the main thread. Behavior on other threads is undefined. (
	0   AppKit                              0x00007fff7cb60005 -[NSApplication runModalForWindow:] + 200
	1   QSInterface                         0x00000001037a61ed -[QSFileConflictPanel runModalAsSheetOnWindow:] + 83
	2   Core Support                        0x0000000105b28ff4 QSPathCanBeExecuted + 37627
	3   Core Support                        0x0000000105b28d63 QSPathCanBeExecuted + 36970
	4   QSCore                              0x000000010371c1b8 -[QSAction performOnDirectObject:indirectObject:] + 1027
	5   QSCore                              0x00000001036d2802 -[QSCommand execute] + 320
	6   QSInterface                         0x0000000103781f64 -[QSInterfaceController executeCommandThreaded] + 336
	7   libdispatch.dylib                   0x00007fff94056f5f _dispatch_call_block_and_release + 12
	8   libdispatch.dylib                   0x00007fff9404e128 _dispatch_client_callout + 8
	9   libdispatch.dylib                   0x00007fff9405d2ce _dispatch_queue_override_invo

Easy enough to fix, but the real problem (which I remember now) is that the file conflict panel is appearing in a separate window and often behind the main interface.