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 loading URL children #1422

Merged
merged 1 commit into from Mar 14, 2013
Merged

fix loading URL children #1422

merged 1 commit into from Mar 14, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Mar 12, 2013

This is why Current Web Page ⇥ Search Contents wouldn't work. If you just ask for a URL's children without ever calling objectHasChildren: first, the handler would always be nil.

Generally, I think we should discourage making changes to an object when simply asking if it has children, but I can see why it was done in this case. Since we can keep the work-around local to the class that creates the "problem", I think it's fine.

Some code just asks for children without calling `objectHasChildren:` first.
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 13, 2013

This kinda feels like it's the wrong way round for me. I see how it makes sense but it leads me to think things might break somewhere else / later down the line.

Surely the 'search contents' action should just validate on objectHasChildren:....

Looking at QSObjectActions.m:L38, it looks like [dObject hasChildren] is being called, so why doesn't it work?
...ok, so Proxy objects unconditionally return YES for hasChildren, I guess because the child is the resolved object. This is kind of where things go a bit funny, since "Current Web Page → Search Contents" gets the children of the resolved object, not just the resolved object (which is actually the child of the proxy object).

I'm not sure what we can do about that. If we change hasChildren for proxy objects to return [resolvedObject hasChildren] it'll break things like right arrowing into the proxy object.
Maybe the proxy object should have a new method resolvedHasChildren which is called in the required places, but then this would require a bit of finicky coding in places.

Thoughts?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Mar 14, 2013

This kinda feels like it's the wrong way round for me. I see how it makes sense but it leads me to think things might break somewhere else / later down the line.

I thought of many complicated work-arounds, too. What led me to this was one thought: You should be able to ask for an object's children without first testing to see if there are any. Some reasoning below…

Surely the 'search contents' action should just validate on objectHasChildren:....

We could do that, but these actions are mostly useful for triggers, so we couldn't rely on validation alone. The actions themselves would need to call it as well. And if they did, what would they do differently? Return nil? That's already going to happen if there are no children. Shouldn't you be able to just return the children without worrying about what they are?

This would apply to the "Show Contents" action too, and we'd really need to search the code for any other place that loads children and make sure it checks objectHasChildren: first (even if it doesn't care). Then we'd have to remember to always do this in the future as well. And all because there's just one object source that mutates the object when checking for children? 😕

The only way this could cause problems down the line is if another object source came along that makes the same assumption (that "has children?" will always precede "give me the children"). The way I see it, if a class is doing something weird like that, it's the responsibility of the class to make sure it doesn't break anything. (Some background I didn't mention: I checked all the other implementations of objectHasChildren: in the Core plug-in before I even made this change. None of them mutate the object they're checking. I didn't check all the plug-ins, but I know I've never done it.)

Looking at QSObjectActions.m:L38, it looks like [dObject hasChildren] is being called, so why doesn't it work?
...ok, so Proxy objects unconditionally return YES for hasChildren, I guess because the child is the resolved object. This is kind of where things go a bit funny, since "Current Web Page → Search Contents" gets the children of the resolved object, not just the resolved object (which is actually the child of the proxy object).

I'm not sure what we can do about that. If we change hasChildren for proxy objects to return [resolvedObject hasChildren] it'll break things like right arrowing into the proxy object.
Maybe the proxy object should have a new method resolvedHasChildren which is called in the required places, but then this would require a bit of finicky coding in places.

Or we could do what I said in #1420 (but we'd still need to make the change in this request). 😃

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 14, 2013

OK so I tried to think of another way of doing this but this seems like the best (going through everything you've said/tried.)

merged

pjrobertson added a commit that referenced this issue Mar 14, 2013
@pjrobertson pjrobertson merged commit 4eb18e3 into quicksilver:master Mar 14, 2013
@skurfer skurfer deleted the urlChildren branch Mar 14, 2013
@skurfer skurfer mentioned this pull request May 28, 2013
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

2 participants