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

fix a small bug with modifier-only activation #1715

Merged
merged 2 commits into from Dec 17, 2013
Merged

fix a small bug with modifier-only activation #1715

merged 2 commits into from Dec 17, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Dec 11, 2013

See #1713 for details.

The second commit might be more controversial, so tell me if I’m overlooking something.

skurfer added 2 commits Dec 11, 2013
…when switching applications. QSInterfaceController already takes care
of this in a cleaner way.
@tiennou
Copy link
Member

tiennou commented Dec 11, 2013

While you're tweaking -appChanged: things, can you move the method (IIRC with the same name) I marked in QSProcessMonitor here (after checking that I'm not saying anything dumb and that it listens to the same notification ;-)) ?

@tiennou
Copy link
Member

tiennou commented Dec 11, 2013

Other that that, LGTM.

@skurfer
Copy link
Member Author

skurfer commented Dec 11, 2013

It does listen to the same notification (QSActiveApplicationChanged is just another name for QSProcessMonitorFrontApplicationSwitched), but it makes calls to self and depends on one of its iVars. Should I make currentApplication/previousApplication into properties so they can be changed from QSController? Or do you want to just leave it?

@skurfer
Copy link
Member Author

skurfer commented Dec 11, 2013

Or should we just move the “hide others” code to QSController and leave the juggling of application iVars where it is? Or maybe update the iVars directly from the process monitor’s appChanged() function?

@tiennou
Copy link
Member

tiennou commented Dec 12, 2013

Yeah, I was talking about the "hide others" code. If it's possible to update the ivars from appChanged() then it's best.

@skurfer
Copy link
Member Author

skurfer commented Dec 12, 2013

OK, have a look.

@pjrobertson
Copy link
Member

pjrobertson commented Dec 16, 2013

Hmmm... I have to disagree with the way things are now - I think the old way was better (kind of)

Setting the previous/current application iVars in the EventHandler itself isn't great - that's the whole point of the notification: interesting objects listen to it.
@tiennou why does the appChanged stuff in QSProcessMonitor 'not belong here'?

If you really don't want to have two objects listening for the notification, then get QSController's appChanged: to call QSProcessMonitor's. Still... they're two different classes, doing two different things - they should both be listening independently really. What if, say QSProcessMonitor was moved to an external plugin (perhaps arguably where it belongs?)... what if say we find there's a new way to get notifs without having to use the Carbon Process event handlers... just deleting the handler would break the whole previous/current app setting that Rob's just put in.

@@ -59,6 +59,10 @@ OSStatus appChanged(EventHandlerCallRef nextHandler, EventRef theEvent, void *us
if( result == noErr ) {
NSDictionary *dict = [(__bridge QSProcessMonitor*)userData infoForPSN:psn];
[[NSNotificationCenter defaultCenter] postNotificationName:QSProcessMonitorFrontApplicationSwitched object:(__bridge id)userData userInfo:dict];
// update the process monitor state
QSProcessMonitor *pm = [QSProcessMonitor sharedInstance];
Copy link
Member

@tiennou tiennou Dec 16, 2013

Choose a reason for hiding this comment

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

What about using userData instead ?

@tiennou
Copy link
Member

tiennou commented Dec 16, 2013

The part that does not belong here is the "Hide Others on App Switch" — we're monitoring processes, not handling kiosk mode. (Arguably, the current/previous application ivars look fishy to me too, I would prefer the list of applications to be ordered Dock-style (first is current, second is previous, and the position changes when you switch). This guarantees that processes[0] is the currently activated app and processes[1] the last one that was activated.

Then I'll nitpick, because you started ;-) :

they should both be listening independently really.

Yeah, that's what I was aiming for. Except one of them is actually the producer of the notification so I don't see why it would have to go through a notification dispatch for something it can do right away — while making sure that someone listening to the notification can get the correct value of "current"/"previous" application, which I don't think is guaranteed with the notification.
Example: imagine some proxy object that registers for the notification, then something cause the QSProcessMonitor to be allocated, it registers to its own notification. Now a process gets activated, firing the notification. Are "current"/"previous" up-to-date when the proxy's observer gets called ? TADA, now you're dependent on a weird ordering of notifications.

moved to an external plugin

Then you'd break the Kiosk mode if the plugin isn't installed. I think QSProcessMonitor should be

  • rewritten : processes aren't gonna be NSDictionaries anymore when the NSWorkspace deprecation warnings are dealt with
  • moved in QSFoundation.

new way to get notifs without having to use the Carbon Process event handlers... just deleting the handler

I'm not sure how to reply to that one... My preffered dev style isn't deleting stuff at point-blank range ;-). And that would introduce a regression too.

@pjrobertson
Copy link
Member

pjrobertson commented Dec 16, 2013

The part that does not belong here is the "Hide Others on App Switch”

Agreed, that’s not QSProcessMonitor’s job :)

would prefer the list of applications to be ordered Dock-style

Like a stack, or the clipboard history? I like it

Except one of them is actually the producer of the notification so I don't see why it would have to go through a notification dispatch for something it can do right away

Well, not exactly. The producer is nothing at all, since it’s just a C function. It’s just that the code is in the same file as QSProcessMonitor. I think that’s why notifications were used. There’s no way to call ‘self’ in those methods (hence why Rob had to use [QSProcessMonitor sharedInstance]
… although, looking at the appChanged() code again, it seems that userData is actually QSProcessMonitor. Using that instead of calling sharedInstance may well be fine.

...while making sure that someone listening to the notification can get the correct value of "current"/"previous" application, which I don't think is guaranteed with the notification.

Good point. So the flow should really be: set the current/previous apps, THEN call the notif. (Atm it’s the other way around)

rewritten : processes aren't gonna be NSDictionaries anymore when the NSWorkspace deprecation warnings are dealt with

Yeah, that’s been on the cards for a while. Using NSRunningApplication. My guess is the NSWorkspace methods will probably be removed in OS X 10.10 - they’ve been deprecated for a loooong time.

OK, so I guess that we’re on the same page now :P
The things that need changing (pending Rob’s input):

  • Use userData instead of [QSProcessMonitor sharedInstance]. Reinstate the QSProcessMonitor appChanged to do the actual setting of the previous/current app ivars - to avoid using properties and accessing them from the C function (Fishy - ?)
  • Set the current/previous app iVars before calling the motif
  • Maybe use a stack style apps list (not urgent)
  • Me to stop nitpicking ;-)

On 16 Rhag 2013, at 16:53, Etienne Samson notifications@github.com wrote:

The part that does not belong here is the "Hide Others on App Switch" — we're monitoring processes, not handling kiosk mode. (Arguably, the current/previous application ivars look fishy to me too, I would prefer the list of applications to be ordered Dock-style (first is current, second is previous, and the position changes when you switch). This guarantees that processes[0] is the currently activated app and processes[1] the last one that was activated.

Then I'll nitpick, because you started ;-) :

they should both be listening independently really.

Yeah, that's what I was aiming for. Except one of them is actually the producer of the notification so I don't see why it would have to go through a notification dispatch for something it can do right away — while making sure that someone listening to the notification can get the correct value of "current"/"previous" application, which I don't think is guaranteed with the notification.
Example: imagine some proxy object that registers for the notification, then something cause the QSProcessMonitor to be allocated, it registers to its own notification. Now a process gets activated, firing the notification. Are "current"/"previous" up-to-date when the proxy's observer gets called ? TADA, now you're dependent on a weird ordering of notifications.

moved to an external plugin

Then you'd break the Kiosk mode if the plugin isn't installed. I think QSProcessMonitor should be

rewritten : processes aren't gonna be NSDictionaries anymore when the NSWorkspace deprecation warnings are dealt with
moved in QSFoundation.
new way to get notifs without having to use the Carbon Process event handlers... just deleting the handler

I'm not sure how to reply to that one... My preffered dev style isn't deleting stuff at point-blank range ;-). And that would introduce a regression too.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

skurfer commented Dec 16, 2013

I think this is going off the rails and has nothing to do with single-modifier activation. :-)

Why don’t I remove the 3rd commit (maybe the second as well?) and we can deal with the appChanged: behavior on another branch?

@pjrobertson
Copy link
Member

pjrobertson commented Dec 17, 2013

Sounds good. Get this merged :P

Sorry I caused such a ruckus :(

On 17 Rhag 2013, at 07:39, Rob McBroom notifications@github.com wrote:

I think this is going off the rails and has nothing to do with single-modifier activation. :-)

Why don’t I remove the 3rd commit (maybe the second as well?) and we can deal with the appChanged: behavior on another branch?


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

skurfer commented Dec 17, 2013

Down to two commits now. Should we open an issue to remind us of the other stuff?

@pjrobertson
Copy link
Member

pjrobertson commented Dec 17, 2013

Merged.

There's already a TODO in the method in QSProcessMonitor (but maybe it should be updated to reference the nitpicking in this convo :P) Plus we'll HAVE to move away from NSDictionaries anyway before Apple deprecates it all, so I think there's enough to remember

pjrobertson added a commit that referenced this pull request Dec 17, 2013
fix a small bug with modifier-only activation
@pjrobertson pjrobertson merged commit 02d0799 into master Dec 17, 2013
@pjrobertson pjrobertson deleted the i1713 branch Dec 17, 2013
skurfer added a commit that referenced this pull request Dec 18, 2013
skurfer added a commit that referenced this pull request Dec 19, 2013
skurfer added a commit that referenced this pull request Jan 2, 2014
skurfer added a commit that referenced this pull request Jan 14, 2014
skurfer added a commit that referenced this pull request Jan 17, 2014
skurfer added a commit that referenced this pull request Jan 17, 2014
skurfer added a commit that referenced this pull request Jan 21, 2014
skurfer added a commit that referenced this pull request Jan 25, 2014
skurfer added a commit that referenced this pull request Feb 2, 2014
skurfer added a commit that referenced this pull request Feb 2, 2014
skurfer added a commit that referenced this pull request Feb 5, 2014
skurfer added a commit that referenced this pull request Feb 11, 2014
skurfer added a commit that referenced this pull request Mar 19, 2014
skurfer added a commit that referenced this pull request Apr 14, 2014
skurfer added a commit that referenced this pull request May 13, 2014
skurfer added a commit that referenced this pull request May 30, 2014
skurfer added a commit that referenced this pull request Aug 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants