Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

resolve direct object before using it to determine indirects #1248

Merged
merged 11 commits into from Dec 10, 2012

Conversation

Projects
None yet
2 participants
Owner

skurfer commented Nov 27, 2012

This is against the release branch.

I guess all the reworking of the move/copy actions revealed a problem that arises when calling validIndirectObjectsForAction:directObject: with certain proxy objects. See #1243.

In #652, we started resolving proxy objects before handing them to actions so every single action provider wouldn't have to test for proxies. This essentially extends the same idea to validIndirectObjectsForAction:directObject:.

skurfer added some commits Nov 27, 2012

resolve direct object before using it to determine indirects
So individual action providers don't need to test for and resolve proxy objects.

fixes #1243
add a new `isProxyObject` method to QSObject
This ensures that we use a consistent test across the codebase, and saves a fair amount of typing.
Owner

pjrobertson commented Nov 27, 2012

Seems like the current application proxy isn't working now :(

I've been through most other proxies I can think of, which all work. Could probably do with some more checking though

Owner

pjrobertson commented Nov 28, 2012

Were you also going to make splitObjects resolve proxy objs? :)

Owner

skurfer commented Nov 28, 2012

Wasn't planning on it, since the places that call splitObjects mostly never see a proxy. But I can if you think we should (or to be consistent with other methods).

Owner

pjrobertson commented Nov 29, 2012

I think it would be good to include it. It means that developers just don't have to think about calling splitObjects on anything, anywhere, anytime :)

Owner

skurfer commented Nov 30, 2012

I think we're better off just casting to a QSObject in splitObjects. Every single message sent to object in that method is a QSObject method.

Owner

pjrobertson commented Dec 3, 2012

I've just had another quick look at this, and what's the problem with just making the resolvedObject method more sensible? :P

It's defined as returning a QSBasicObject, which is the problem… whereas it will always return a QSObject, since that's what [self proxyObject] returns:

Should be:

- (QSObject *)resolvedObject {
    // proxyObject in this file (QSProxyObject.m) returns a QSObject, so why does resolvedObject not?
    return [self proxyObject];
}
Owner

skurfer commented Dec 4, 2012

I would love to make it just return a QSObject. Makes more sense. I know I've looked into why it's QSBasicObject, and there was a reason, but I don't remember if it was a show-stopper. I'll look at it again and let you know.

skurfer added some commits Dec 4, 2012

make proxy objects resolve to QSObject
It was previously returning a `QSBasicObject`, which required casting
the result all over the place.
Owner

skurfer commented Dec 4, 2012

Now I remember. It's because resolvedObject was implemented in QSBasicObject and QSProxyObject was overriding that. Since QSProxyObject is a subclass of QSObject, I see no reason why it can't be moved there, so that's what I've done. No problems so far.

On a side note, I suppose the whole point of this is so we can call resolvedObject blindly without first testing whether or not it's a proxy. Perhaps we should start doing that to eliminate some tests. At the very least, I'll do it in splitObjects.

remove unnecessary proxy tests
Things were designed so you could call `resolvedObject` blindly on any
QSObject.
Owner

pjrobertson commented Dec 4, 2012

cool, let's hope it works!

I suppose the whole point of this is so we can call resolvedObject

Seems nice :) Almost means we might not even need isProxyObject now. Although it's probably nice to have around for some situations

Owner

skurfer commented Dec 4, 2012

Proxy object tests removed across the board.

There's one other thing I want to look at after lunch: I'm not comfortable with the way _safeObjectForType: and arrayForType: call each other. I managed to lock up QS because of it.

I also don't like the idea of testing [self isProxyObject] in QSObject at all. I feel like if the code truly needs to be different for proxy objects, then the method in question should have its own implementation in the QSProxyObject class. Hopefully you can weigh in on that before I get back and start writing code. :-)

Owner

pjrobertson commented Dec 4, 2012

I think you've mentioned it before, but from looking sat things just now, proxies should probably declare their allowed types explicitly.

Just now

[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(objectIconModified:) name:QSObjectIconModified object:proxy];

was called for the 'random track' proxy, because QSExecutor called [object validPath] which went all the way down the chain to resolve the object to figure out if it had a path. Yuck!
Although that's another story for another day :)

I think overriding the method is definitely a good idea. Much tidier IMO. QSObject doesn't need to know about QSProxyObject at all -that's the way subclasses are meant to work, yeah?!

I don't see how you could have got a lock, unless aKey (in arrayForType) was QSProxyObject or if [self resolvedObject] somehow resolved to the proxy itself :S
Anyhow, perhaps by moving _safeObjectForType to QSProxyObject you might be able to sort things out a bit better :)

Oh, and just so I can get my GCD fix for the day, do you think you could replace the NSTimer…

    dispatch_time_t popTime = dispatch_time(DISPATCH_TIME_NOW, (int64_t)interval * NSEC_PER_SEC);
    dispatch_after(popTime, dispatch_get_main_queue(), ^(void){
        [self releaseProxy];
    });

I've spent the last half an hour looking into whether we'd need to use a 'weak' reference to self in that block (so that if self gets released at some point between the block being dispatched and it being called, the block doesn't retain the value), but I've decided that since the proxy object is unlikely to ever get released, we don't need to.

Owner

skurfer commented Dec 4, 2012

QSExecutor called [object validPath] which went all the way down the chain to resolve the object to figure out if it had a path. Yuck!

Yeah, calling validPaths when validating objects for the third pane was also the reason we couldn't include proxy objects in the list.

Although that's another story for another day :)

Yes, another day please. :-)

I think overriding the method is definitely a good idea. Much tidier IMO. QSObject doesn't need to know about QSProxyObject at all -that's the way subclasses are meant to work, yeah?!

Agreed. Working on it. I've got everything figured out except that test for "ProxyIcon" in loadIcon.

I don't see how you could have got a lock, unless aKey (in arrayForType) was QSProxyObject or if [self resolvedObject] somehow resolved to the proxy itself :S

It happened when I changed the test from (!object && [self isProxyObject]) to (!object). Anyway…

Anyhow, perhaps by moving _safeObjectForType to QSProxyObject you might be able to sort things out a bit better :)

Yep. Moved.

Oh, and just so I can get my GCD fix for the day, do you think you could replace the NSTimer…

So timers are OK if GCD handles them? ;-) Sounds good. I'll try it out. I'll make the same changes on the tempMeta branch. Maybe you'll like it more then. :-)

I've spent the last half an hour looking into whether we'd need to use a 'weak' reference to self in that block (so that if self gets released at some point between the block being dispatched and it being called, the block doesn't retain the value), but I've decided that since the proxy object is unlikely to ever get released, we don't need to.

True. I guess we'd just need to make sure it gets set to nil if we ever get rid of one.

move all proxy object code into QSProxyObject
The QSObject class shouldn't be implimenting things for a subclass.
Owner

skurfer commented Dec 4, 2012

It's getting interesting now. :-)

I'm thinking we don't need to have _safeObjectForType in QSProxyObject at all. All it does differently is fall back to the resolved object to get data when it has none of its own. But some checking in the debugger shows that this only happens for a few objects, and in every case, the resolved object returns nil too. Seems pointless. I'll check the history, but I expect to find nothing but "Code cleanup".

Side benefit: I figured out why some proxy objects reflect the icon for the underlying object. If the icon is set to "ProxyIcon", this signals Quicksilver to get the icon for the resolved object instead. So it was just a plist change. I've also set it up so the same thing will happen if a proxy object defines no icon of its own (as that seems like a more intuitive way to do it).

Owner

pjrobertson commented Dec 4, 2012

Side benefit: I figured out why some proxy objects reflect the icon for
the underlying object. If the icon is set to "ProxyIcon", this signals
Quicksilver to get the icon for the resolved object instead. So it was just
a plist change. I've also set it up so the same thing will happen if a
proxy object defines no icon of its own (as that seems like a more
intuitive way to do it)

Nice! I was still looking into this, but not really getting anywhere :)

So timers are OK if GCD handles them? ;-) Sounds good. I'll try it out.
I'll make the same changes on the tempMeta branch. Maybe you'll like it
more then. :-)

GCD should be more efficient, (no need to create an object, except it has
to store the block in memory) and I've also found really weird behaviours
with NSTimers
(the one I deleted here:
cc248d1#L3R28was
giving random crashes all over the place)

True. I guess we'd just need to make sure it gets set to nil if we ever
get rid of one.

Sort of. Blocks are more complicated, since they retain the objects inside
the block. If the objects are released whilst the block is in the queue,
then the block doesn't necessarily release them (leaving dangling objects).
That's why you normally need a __week reference, so that if the objects are
released outside the block, the block doesn't retain them.
At least, that's what I think :)
So if you want to go to ARC… you still have to deal with memory management
in blocks :D

On 4 December 2012 19:55, Rob McBroom notifications@github.com wrote:

Side benefit: I figured out why some proxy objects reflect the icon for
the underlying object. If the icon is set to "ProxyIcon", this signals
Quicksilver to get the icon for the resolved object instead. So it was just
a plist change. I've also set it up so the same thing will happen if a
proxy object defines no icon of its own (as that seems like a more
intuitive way to do it).

@skurfer skurfer referenced this pull request Dec 5, 2012

Merged

localized files #1240

Owner

pjrobertson commented Dec 7, 2012

I'm not sure where the discussion is, but didn't @tiennou mention something about not using GCD here, and @skurfer said he'd change it back?
I've been out of the loop a little the past few days.

I'm thinking we don't need to have _safeObjectForType in QSProxyObject at all. All it does differently is fall back to the resolved object to get data when it has none of its own. But some checking in the debugger shows that this only happens for a few objects, and in every case, the resolved object returns nil too. Seems pointless. I'll check the history, but I expect to find nothing but "Code cleanup".

Still, it's certainly neater doing what you've done, so I think it's fine.

All looks good to me :)

Owner

skurfer commented Dec 10, 2012

We talked about not using GCD to replace the timers on the tempMeta branch (and I made the change).

In this case, it never supported cancellation to begin with, so we don't really lose anything. And I don't think we really need to support canceling/extending the timer here, since it's only ever set internally and only if the previous cache has expired.

Owner

pjrobertson commented Dec 10, 2012

Aaah OK, just me getting confused :)

merged

pjrobertson added a commit that referenced this pull request Dec 10, 2012

Merge pull request #1248 from skurfer/validIndirects
resolve direct object before using it to determine indirects

@pjrobertson pjrobertson merged commit 0fb18dc into quicksilver:release Dec 10, 2012

skurfer added a commit that referenced this pull request Dec 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment