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

Process monitor updates #575

Merged
merged 9 commits into from Mar 26, 2012
Merged

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Nov 13, 2011

This is major surgery on QSProcessMonitor ;-).

I made it always keep the list of process up-to-date by building the full list in -reloadProcesses (aka the first scan on startup), and then the appLaunched/Terminated notifications ensure that the list is always up to date.

The responsibility of indexing background apps has been moved to QSProcessSource, and this last one had been made to observe QSProcessMonitor -allProcesses array (instant reindex when something launches/quits !). Also, the necessity to restart QS when you fiddle with the "Show Background Application" checkbox is gone (its observed by QSProcessSource).

A few things to note :

  • I #define'd QSActiveApplication to be the same as the new set of notifications introduced.
  • I dropped a @"processesChanged" notification. It only seemed to be used internally (no others reference in the main source code), and KVO is much nicer than notifications for that. Plugins should be checked, and updated to 1) either use the new notifications or 2) observe either -allProcesses, -visibleProcesses, or -backgroundProcesses.
  • I'm also planning to kill the beginning of the -appChanged notification (where switching applications hides all others, so called kiosk mode), since it has absolutely nothing to do with process monitoring. Maybe move it in QSController ?

This is related to #453, and another one which I can't find anymore (related to background apps only getting scanned once at startup).

tiennou added 9 commits Nov 13, 2011
- Use Carbon for monitoring. NSWorkspace only handles non-background applications.
- Processes are now cached in a dictionary keyed by PSN, loaded the first time you ask for the process list. This means the process list is always up to date now w.r.t background processes, since notifications are now seen for background processes.
- Check the process dictionary for process info on termination (that's the only place we can get it, since the process itself is no longer running).
- Remove most of the stuff from the header (most of those are private stuff that shouldn't have been exported in the first place).
- Reimplement -allProcesses, -visibleProcesses and their -get* variants using the new process list.
@skurfer
Copy link
Member

@skurfer skurfer commented Nov 14, 2011

Plugins should be checked, and updated to 1) either use the new notifications or 2) observe either -allProcesses, -visibleProcesses, or -backgroundProcesses.

I keep a copy of the old repo that had all plug-ins for checking things like this. I only found one that looks for processesChanged: The Nimbus App Switcher plug-in. Never heard of it. It’s not available for in-app download. I’d be surprised if anyone is using it.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 15, 2011

why does the user need to know what a 'background process' is? If something's running, then it's a process :)
I can' think of any situation when I'd only want 'front ground' processes running.

I don't know, I would say maybe some users what to keep the cruft out of their process list ? I'm not really found of seeing 'loginwindow' show up in that list.

I agree on the checkbox, I just kept it for discussion with users first ;-). Another way would be to split the "Applications & Process" Catalog into "Applications" & "Background Processes", so that users have more control over what gets indexed. This would add more visibility to that setting...

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 15, 2011

I'm all about simplifying preferences. Eliminate any preference checkbox that you possible can. So I say: Remove the checkobx, always include background processes and the users will get used to it. It will still be just as quick to select the app you want.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 19, 2011

Maybe the Catalog item could be splat in "Applications" & "Background Processes" ? I'm all in favor of dropping the checkbox, but I don't like changing user habits (and its a little checkbox in an oft-used drawer after all ;-)).

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 19, 2011

I guess I'd be ok with that. More obvious than the little checkbox in an often-used drawer. Still a preference, but better than before. :-)

Btw. is "splat" really a word? ;-)

@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 20, 2011

Le 19 nov. 2011 à 21:51, Henning Jungkurth a écrit :

I guess I'd be ok with that. More obvious than the little checkbox in an often-used drawer. Still a preference, but better than before. :-)

Haha, that's what you get trying to use Old English word without knowing what they mean ! I though oft meant its exact opposite ;-).

Btw. is "splat" really a word? ;-)

Split, splat, splat ? ;-P

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 24, 2011

So...are you gonna splatter it up? ;-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jan 14, 2012

I've been in favour of dropping the checkbox all together for a while now. Just has a user report a bug about the 'problem' (#635)

Maybe 2 catalog entries with the checkbox dropped would work fine as well :)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 26, 2012

I see no problems with this 'as is', except for the matter of the background processes checkbox, but it's not a show stopper.

Any objections from others to this being merged?

I'm a bit wary of just dropping the @"processesChanged" notification, but @skurfer has done a check and it seems to be unused.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 26, 2012

I’ve just been waiting for the UI changes before merging this, but I’m guessing @tiennou is busy. I’ve been using this since it was submitted without issue. No objections to merging.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 26, 2012

One of us can probably make the UI changes. I'll merge this now.

pjrobertson added a commit that referenced this issue Mar 26, 2012
@pjrobertson pjrobertson merged commit b8c2cda into quicksilver:master Mar 26, 2012
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

4 participants