Asynchro issues #388

pjrobertson · 2011-06-22T06:47:07Z

There were a few problems that people had reported with the asynchro icon loading changes.
This pull request hopefully fixes them.

Outside of adding a few comments and altering some header files to tidy them up, here's what I've done:

Added a new objectIconModified notification. The original objectModified notif was being called when the icon was changed. The corresponding method meant all actions were being reloaded every time the icon loaded: a bit wasteful. Plus, asking the actions to reload when the icon's reloaded meant any key presses in pane 2 would be lost. See issue Pane 2 loses typed letters after resolving an image in pane 1 #338
Made QS only refresh the results window if it's open
Made icons reliably load when you have things being returned to QS (cases where you rename a file or use ⌘⎋ to grab a file) See issue Renaming files causes QS to lose their preview thumbnail #339

skurfer · 2011-06-22T14:01:52Z

Make your life easier. Just put “closes #339” or something like that in the commit message. The issue will then refer to this pull request automatically (no need to go post a link back to here manually) and when this gets merged, those issues will close on their own. :)

https://github.com/blog/831-issues-2-0-the-next-generation

pjrobertson · 2011-06-22T14:06:04Z

Oh yeah, I think you mentioned that before. Thanks for the link, definitely
use it in the future :)

On 23 June 2011 00:01, skurfer <
reply@reply.github.com>wrote:

Make your life easier. Just put closes #339 or something like that in the
commit message. The issue will then refer to this pull request automatically
(no need to go post a link back to here manually) and when this gets merged,
those issues will close on their own. :)

https://github.com/blog/831-issues-2-0-the-next-generation

Reply to this email directly or view it on GitHub:
#388 (comment)

HenningJ · 2011-06-22T14:18:59Z

Quicksilver/Code-QuickStepCore/QSObject_FileHandling.m

@@ -375,7 +371,7 @@ NSArray *recentDocumentsForBundle(NSString *bundleIdentifier) {
 	theImage = [self prepareImageforIcon:theImage];

 	[object setIcon:theImage];
-	[[NSNotificationCenter defaultCenter] postNotificationName:@"ObjectModified" object:object];
+	[[NSNotificationCenter defaultCenter] postNotificationName:@"ObjectIconModified" object:object];


Could you define a constant in Code-QuickStepCore/QSNotifications.h for the notification name and document it with a few words?

Good idea. I'll do that (tomorrow-ish) for the ObjectModified notif as well if it hasn't already been done (tired)

OK I've doe that now. The last two commits do this.

HenningJ · 2011-06-22T14:29:21Z

Cool. I knew there were a few problems with it, but never got around to looking at it.

HenningJ · 2011-06-24T18:51:48Z

Quicksilver/Code-QuickStepCore/QSObject_FileHandling.h

@@ -13,6 +13,7 @@
 @interface QSFileSystemObjectHandler : NSObject {
    NSMutableDictionary *applicationIcons;
 }
+- (NSArray *)actionsForDirectObject:(QSObject *)dObject indirectObject:(QSObject *)iObject;


hmmm? why wasn't this here...of why is it needed?

I'm not 100% sure now since I made this change a while ago. I have a feeling that line 287 of QSExecutor.m may have given me a warning at some point or something like that. I'm not entirely sure :/

Just tried it without this line and I don't get any different behaviour...

pjrobertson · 2011-06-24T22:57:15Z

I've made a few changes. See what you think :)

There's still the debate of the actionsForDirectObject method.

HenningJ · 2011-06-30T13:13:15Z

I've had another look at QSObject_FileHandling.h and I must say it's quite a mess.
For example there is the category definition @interface QSObject (QSObjectFileHandling), QSObject_FileHandling.m implements these methods in @implementation QSObject (FileHandling). I have no idea what's the implication of this, or if it even matters, but it seems wrong.
There is also the definition of the QSFileObjectCreationProtocol category on QSObject and I'm pretty sure that isn't implemented anywhere.
The same goes for QSFileCreatingHandlingProtocolon NSObject (even disregarding that "creatinghandling" just sounds very wrong ;-)).
Then there is QSFileSystemObjectHandlerwhich seems like it's supposed to be a QSObject, but instead inherits from NSObject.

As I said: it's a mess. But all of this is not really related to this pull request, so maybe it should be merged anyway, and it should be clean up in another pull request. But on the other hand, since you were already working on this file, maybe you could just clean it up now. I don't know, your choice. :-)

But I also want to add some encouraging words: I've been running this for a while now and it works great. :-)

pjrobertson · 2011-06-30T13:34:26Z

I've had another look at QSObject_FileHandling.h and I must say it's quite
a mess.

Tell me something that isn't new! :P

maybe you could just clean it up now. I don't know, your choice. :-)

There are a couple of other things I want to be working on right now, and to
be honest, I don't think I know the formalities of Cocoa well enough to be
the best person for this. I am back in Wales now so I do have access to my
Cocoa books...

I've been running this for a while now and it works great. :-)

Awesome, thanks :)
I think the only underlying issue is whether (NSArray *)actionsForDirectObject:(QSObject *)dObject indirectObject:(QSObject *)iObject
should be in the .m file or not, but you've already touched upon how much of
a mess everything is.
Perhaps the solution could be to leave it in, and just add a comment saying
I added it, and that if/when someone sorts out the files they can look into
it again?

On 30 June 2011 14:13, HenningJ <
reply@reply.github.com>wrote:

I've had another look at QSObject_FileHandling.h and I must say it's quite
a mess.
For example there is the category definition @interface QSObject (QSObjectFileHandling), QSObject_FileHandling.m implements these methods in
@implementation QSObject (FileHandling). I have no idea what's the
implication of this, or if it even matters, but it seems wrong.
There is also the definition of the QSFileObjectCreationProtocol category
on QSObject and I'm pretty sure that isn't implemented anywhere.
The same goes for QSFileCreatingHandlingProtocolon NSObject (even
disregarding that "creatinghandling" just sounds very wrong ;-)).
Then there is QSFileSystemObjectHandlerwhich seems like it's supposed to
be a QSObject, but instead inherits from NSObject.

As I said: it's a mess. But all of this is not really related to this pull
request, so maybe it should be merged anyway, and it should be clean up in
another pull request. But on the other hand, since you were already working
on this file, maybe you could just clean it up now. I don't know, your
choice. :-)

But I also want to add some encouraging words: I've been running this for a
while now and it works great. :-)

Reply to this email directly or view it on GitHub:
#388 (comment)

HenningJ · 2011-06-30T13:43:14Z

Perhaps the solution could be to leave it in, and just add a comment saying I
added it, and that if/when someone sorts out the files they can look into it again?

Ok, fine with me. Add that and I will merge it. :-)

…hat fixes quicksilver#338, fixes quicksilver#339

pjrobertson · 2011-06-30T13:48:40Z

Yaya! I've done that :)

@skurfer — also stuck in a sneaky couple of fixes #... thanks again for the heads up :)

Asynchro issues

HenningJ · 2011-06-30T13:53:13Z

yay

skurfer · 2011-06-30T14:09:15Z

Cool. For what it’s worth at this point, I’ve also been running this for some time without issue.

pjrobertson added 6 commits May 29, 2011 22:13

fixed icon not displaying when re-invoking QS

2c94e2b

added a method to the header file to remove a warning

0713fee

coments as I understood the code

2f5a63c

added a new ObjectIconModified notif for less ambiguity

4b64d71

made loading of icons for single items in the 1st pane more reliable

48e13c4

a few comments

11ea3de

HenningJ reviewed Jun 22, 2011
View reviewed changes

HenningJ reviewed Jun 24, 2011
View reviewed changes

pjrobertson added 5 commits June 25, 2011 08:39

defined constants for notifications and given a description

b616be9

more meaningful comments

74722d1

one last string to change to a const

f591a8f

wrong string used, got it right this time

b02adaf

tidied up objectIconModified method

5208aeb

added comments to an unclear definition. Last in a group of commits t…

cec87a6

…hat fixes quicksilver#338, fixes quicksilver#339

HenningJ added a commit that referenced this pull request Jun 30, 2011

Merge pull request #388 from pjrobertson/asynchroIssues

3f8d78b

Asynchro issues

HenningJ merged commit 3f8d78b into quicksilver:master Jun 30, 2011

skurfer mentioned this pull request Apr 17, 2012

Load web search icons asynchronously (fixes #637) #805

Merged

Asynchro issues #388

Asynchro issues #388

pjrobertson commented Jun 22, 2011

skurfer commented Jun 22, 2011

pjrobertson commented Jun 22, 2011

HenningJ Jun 22, 2011

pjrobertson Jun 22, 2011

pjrobertson Jun 24, 2011

HenningJ commented Jun 22, 2011

HenningJ Jun 24, 2011

pjrobertson Jun 24, 2011

pjrobertson commented Jun 24, 2011

HenningJ commented Jun 30, 2011

pjrobertson commented Jun 30, 2011

HenningJ commented Jun 30, 2011

pjrobertson commented Jun 30, 2011

HenningJ commented Jun 30, 2011

skurfer commented Jun 30, 2011

Asynchro issues #388

Asynchro issues #388

Conversation

pjrobertson commented Jun 22, 2011

skurfer commented Jun 22, 2011

pjrobertson commented Jun 22, 2011

HenningJ Jun 22, 2011

Choose a reason for hiding this comment

pjrobertson Jun 22, 2011

Choose a reason for hiding this comment

pjrobertson Jun 24, 2011

Choose a reason for hiding this comment

HenningJ commented Jun 22, 2011

HenningJ Jun 24, 2011

Choose a reason for hiding this comment

pjrobertson Jun 24, 2011

Choose a reason for hiding this comment

pjrobertson commented Jun 24, 2011

HenningJ commented Jun 30, 2011

pjrobertson commented Jun 30, 2011

HenningJ commented Jun 30, 2011

pjrobertson commented Jun 30, 2011

HenningJ commented Jun 30, 2011

skurfer commented Jun 30, 2011