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

Rework QSTask. #1611

Merged
merged 42 commits into from Jul 28, 2016
Merged

Rework QSTask. #1611

merged 42 commits into from Jul 28, 2016

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Sep 18, 2013

This supersedes #1603 and #1265.

Major surgery on QSTask, this makes QSTask a real model-only object, kills the deadlock issues between QSTaskController and QSTaskViewer and fixes the auto-show/hide behavior once and for all (at least it seems to behave right, now ;-)) :

  • It will show if there's a new task and the prefs says it (it was).
  • It won't show if during the time the task was created and the time the viewer reacts the task is done. This should fix #1040, and stop me wanting to bash it each time I open a new tab.
  • It won't try to auto-close the window if you open it manually and a task finishes (this fixes a never reported issue ;-)).

Known issues/discussion:

  • The first time it opens (set it to automatically open, quit and restart so you get the catalog startup scan), the view is messed up because it extends all the way to the right, which looks weird.
  • I've setup minimal code to correctly to handle subtasks, but there's no viewer suppport, and I'm not sure it's worth it. It was mostly present, but the only thing I can think that would use that is the catalog scanning, and maybe the update system.
  • I'm tempted to push up the HIDE_DELAY by a few millisec, so you can see the task completed. Right now it just closes before you can even see the task has ended (which looks the same to the user, but could be nicer).
  • I've killed the target/action mechanism it had for task cancellation, instead relying on a cancelBlock.
  • Documentation. I really want to do this one, but I need some more time.
  • This could break plugins that used tasks (there was a -(QSObject *)result method I've killed, plus the effects of the major surgery). But I don't think that many plugins did that, and I think the API as it is is sufficient for "general user information". But if you find I broke some plugin, I'll happily revise and provide at least a private runtime equivalent.
  • Maybe the preferences settings should be also specified in the Extras pref pane ? I think it's pretty hidden atm, IIRC by default it won't autoshow, and you can't change those settings unless you open it first and fiddle with the gear menu. #119 talks about that a little.

All in all, eyes are welcome so I'm breaking anything, and if you stop a thing I didn't point out in the list above, the

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 19, 2013

Maybe the preferences settings should be also specified in the Extras pref pane…

You mean something like “Preferences → Extras → Show the Task Viewer automatically when tasks are created”? 😃

I've always thought that things created with “Run After Delay…” and “Run at Time…” should appear in the Task Viewer. That will let you see that they exist, when they will run, and there should be a way to remove/cancel them from there. I'm not saying you need to implement any of this now, but if we're doing major surgery, we should at least make sure the design will allow for it to be added easily later. Agreed?

I’ll take a look at this tomorrow.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 19, 2013

Does this also obviate #1598, since it includes the renames?

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 19, 2013

Yep, forgot to point that out ;-).

Le 19 sept. 2013 à 06:09, Rob McBroom notifications@github.com a écrit :

Does this also obviate #1598, since it includes the renames?


Reply to this email directly or view it on GitHub.

}
- (void)cancel {
@synchronized (self) {
NSAssert(self.isRunning == NO, @"Asked to cancel stopped task %@", self);
Copy link
Member

@skurfer skurfer Sep 19, 2013

Choose a reason for hiding this comment

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

The assertion happens if the condition is false, so this should be == YES, right?

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 19, 2013

I found a couple of things by trying to implement the stuff I was talking about with “Run after Delay…” (which isn’t going to be that hard at all).

  • The Hidden property in QSTaskView.xib needs to be hooked up to the inverse of task.showProgress.
  • I know I’ve used -[QSTask stopTask:] in at least one (never released) plug-in. Could there be others using it?

Other than the cancel method, and the progress bar issues, all seems to be working well with this.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 2, 2013

If you're busy, I can add the finishing touches mentioned above and do another pull.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 3, 2013

I'm looking at it atm.

The Hidden property in QSTaskView.xib needs to be hooked up to the inverse of task.showProgress.

Which hidden property ? There are many I can choose from ;-).

I know I’ve used -[QSTask stopTask:] in at least one (never released) plug-in. Could there be others using it?

If you think there's a need to, I can put in (deprecated) wrappers for the old names.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 3, 2013

Which hidden property ?

I mean the one literally named “Hidden”. The one that controls whether or not the progress bar is visible. 😃

I can put in (deprecated) wrappers for the old names.

Looks like the plug-ins mainly use updateTask: and removeTask: from QSTaskController, so as long as those still work, it should be fine. I can update the Cloud plug-in before release.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Oct 3, 2013

I mean the one literally named “Hidden”. The one that controls whether or not the progress bar is visible. 😃

I was wondering which one — from the 10-or-so controls/views that are hideable in that XIB — you were talking about (or the NSView itself). Now I now, though still pretty indirectly ;-). Damn, text-based communication isn't getting any easier.

On a branch-related note, it seems like it doesn't work as well as when I first wrote it. I'm really wondering what's happening now because I clearly remember noticing the progress bar bug at startup, trying to debug the auto-show/hide behavior, then running it for a whole evening while in a 3-hour-long browsing and drooling in contempt that I didn't get bothered by empty task viewers popping in and out all the time ;-). Now, it won't show itself at startup (I actually chown root ~/Library/Quicksilver/Cachesed) no matter what I do, so I can't check the startup bar bug. The only difference I have is that I merged in master before starting to work on that, and I'm now on Xcode 5.

If you get the time, can you confirm whether I'm going crazy or something ? ;-)

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 3, 2013

I see it during the initial scan when I launch from the Debugger (Xcode 5 here too).

Are you asking about a bug with the window or the progress bar?

It's too fast to see if the progress bar looks correct, but the initial position of the panel is strange. For me, it appears at the top of the screen (behind the menu bar) and right of the screen's center.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Nov 30, 2013

Okay, updated. This fixes the infinite progress bar issue, tries to make it dockable (since we're using a dockable window), and "various bug fixes".

I said "tries" because — as I pointed out in the dev group a while ago — QSInterface is a mess, like in that case, the docked Task Viewer won't undock if you tickle it with you mouse because I don't know, it doesn't. I imagine it has to do with the fact that -[NSWindow isVisible] and -[QSWindow hidden] aren't agreeing, but you can't just override it without breaking animations. So let's hope that nobody has a use case for tickling the Task Viewer out — personnally I can't see one, because if it's docked, then it's empty.

Okay, just noticed it doesn't animate when popping in...

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 1, 2013

I'm always keen for a tickle. If you want to try and figure out why it's not working, I know a bit about it whilst I fixed the forever bug with the clipboard/shelf windows sometimes disappearing etc. I think the main thing was to do with canBecomeKeyWindow and resignsKey. Basically it was important to only let the window become key when absolutely necessary.

Since you've rebased nicely, I'll have a play. This doesn't depend on any other PR right? Do we still have any 'PR dependencies'? If so, I'll concentrate on those first

@tiennou
Copy link
Member Author

@tiennou tiennou commented Dec 1, 2013

Please do ;-), it should stands alone now. You can see a side-effect of all that mess in the new toggleViewer: I had to add (does it make you go "WTF" when you read it ? good, because it did when I wrote it ;-)). I can tell that QSWindow is forced to reimplement "hide" itself because the standard "orderOut:" method closes the window, which ought to prevent events like mouse-in/out to be delivered to it, breaking the docking thing. I think there's a better way, but I don't feel like going down QSWindow hierarchy one method at a time and rewrite/document it.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 2, 2013

A few minor usability things:

  • Do we really need the debug Will show for task NSLog?
  • I think the SHOW_DELAY should be shortened. I like 0.05s
  • I tried increasing the HIDE_DELAY to 0.4s, since I think the tasks are disappearing too quickly. That didn't solve my problem, since it just causes the window to stay open, even after the task has disappeared. Perhaps the removeTask: code should be dispatch_delayed as well (see note below^)
  • The window doesn't seem to have an autosave poision - move it, quit QS, and relaunch - it appears in the original (annoying/behind the QS interface) place

^ The removeTask: methods (in both QSTaskController and QSTaskViewer are confusing me. It seems the QSTaskController one never gets called, the QSTaskViewer one does, since it's called when a QSTaskRemovedNotification notif is sent.

EDIT: I like this:

  • Change HIDE_DELAY to 1.0
  • in removeTask: wrap lines 171-173 in a dispatch_after block. Means you can see the last task that was run - that's the whole point of the task viewer right?! :P

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 2, 2013

One more thing:

  • If you resize the task viewer to be too small for the task, it stays that way. Should it resize to show the full task? I think automatic resizing should be standard. Why would you ever want the Task Viewer to be bigger/smaller than it needs to be?

@skurfer
Copy link
Member

@skurfer skurfer commented Dec 2, 2013

What about my comment on line 103. That should be self.isRunning == YES, shouldn’t it?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 2, 2013

Yeah I think you're right @skurfer

@tiennou
Copy link
Member Author

@tiennou tiennou commented Feb 16, 2014

Minor update to that one. I've fixed most things you guys pointed out.

Interesting points :

  • I've tweaked the GCD helpers a little (added a delayed variant and noticed this wasn't DRY).
  • I've left (some of my) debugging statements in there, in case you want to take a peek at it (mostly the add/remove task ones).
  • I got an exception once and only once at soon as I started working, and fixed it by serializing accesses (see b6ae632).

Sadly, that last point creates a visual glitch visible at startup because the QSDelayedScan task finishes before the QSCatalogScan task does, and it still displays through because it's not redrawn correctly when it should, and whatever -setNeedsDisplay: I've thrown at it, I haven't been able to fis that. Now, I'm utterly bored with how this whole window is built/managed, so if one of you guys wants to switch to a View-Based TableView or something, feel free to do so. I'm happy with the low-level changes at that point even though I marked stuff as deprecated and didn't update every place yet. I still have a TODO on subtask-management but hey, it's not like it's used anywhere.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Feb 16, 2014

so if one of you guys wants to switch to a View-Based TableView or something, feel free to do so.

I like those as well, so I’ll try and take a look… although I’m bogged with stuff right now, so maybe not soon.

On 17 Chwef 2014, at 00:35, Etienne Samson notifications@github.com wrote:

Minor update to that one. I've fixed most things you guys pointed out.

Interesting points :

I've tweaked the GCD helpers a little (added a delayed variant and noticed this wasn't DRY).
I've left (some of my) debugging statements in there, in case you want to take a peek at it (mostly the add/remove task ones).
I got an exception once and only once at soon as I started working, and fixed it by serializing accesses (see b6ae632).
Sadly, that last point creates a visual glitch visible at startup because the QSDelayedScan task finishes before the QSCatalogScan task does, and it still displays through because it's not redrawn correctly when it should, and whatever -setNeedsDisplay: I've thrown at it, I haven't been able to fis that. Now, I'm utterly bored with how this whole window is built/managed, so if one of you guys wants to switch to a View-Based TableView or something, feel free to do so. I'm happy with the low-level changes at that point even though I marked stuff as deprecated and didn't update every place yet. I still have a TODO on subtask-management but hey, it's not like it's used anywhere.


Reply to this email directly or view it on GitHub.

subtasks = [value copy];
}
- (BOOL)animateProgress {
return self.progress < 0;
Copy link
Member

@skurfer skurfer Feb 17, 2014

Choose a reason for hiding this comment

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

Should this be reversed to >? Or to put it another way, why do we need animateProgress and indeterminateProgress if they’re identical?

@skurfer
Copy link
Member

@skurfer skurfer commented Feb 17, 2014

Is it just the name of the delayed scan that shows through, or would that explain the progress bar problems I see as well?

screen shot 2014-02-16 at 10 36 57 pm

The position of the progress varies, but it never appears to finish and the list never goes blank. There’s always the delayed scan overlapped with an unfinished catalog scanning task.

Also, the progress bar should be hidden when showProgress is NO. Right now, it’s shown no matter what.

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 26, 2016

OK, I’m running with this once again. I know I’ve looked over the code like 3 times, so I’ll just test for real-world functionality.

If it looks good, I’ll also update the NIB deployment targets (unless you want to add that here) and I’ll finally be able to work on adding “run after delay” and “run at time” commands to the task viewer. 😃

@skurfer skurfer merged commit af2a8ec into quicksilver:master Jul 28, 2016
1 of 2 checks passed
@skurfer
Copy link
Member

@skurfer skurfer commented Jul 28, 2016

Three years later… Never give up hope!

The change log for this might take the rest of the day. :hurtrealbad:

skurfer added a commit that referenced this issue Jul 28, 2016
@skurfer
Copy link
Member

@skurfer skurfer commented Jul 28, 2016

If I missed anything in 8636c0e, feel free to push change log updates straight to master.

@tiennou tiennou deleted the task-cleanup branch Jul 28, 2016
@tiennou
Copy link
Member Author

@tiennou tiennou commented Jul 28, 2016

I think that covers it all.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 29, 2016

wow, awesome work guys!

pjrobertson pushed a commit that referenced this issue Aug 3, 2016
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.

3 participants