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

assign parents #2229

Merged
merged 6 commits into from Oct 21, 2016
Merged

assign parents #2229

merged 6 commits into from Oct 21, 2016

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jul 15, 2016

This should have always been there, right? Why wasn’t it?

I know retain cycles are something to worry about, but I’m sure the affected objects have a dozen references to them in various places anyway.

The first commit is the main thing, but I fixed some other minor things while investigating. My gut tells me all that left-arrow code needs to be refactored, but I’m not going any deeper for the moment.

skurfer added 3 commits Jul 15, 2016
no need to test since it can't be anything but nil here
duplicates are already handled in a more general way
@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 15, 2016

I know retain cycles are something to worry about

Wait, this stores the identifier and not a reference to the object (probably to avoid that very thing), so that’s not even a concern.

@@ -697,6 +697,9 @@ - (void)setChildren:(NSArray *)newChildren {
if (newChildren) {
if ([[self cache] objectForKey:kQSObjectChildren] != newChildren) {
[[self cache] setObject:newChildren forKey:kQSObjectChildren];
[newChildren enumerateObjectsWithOptions:NSEnumerationConcurrent usingBlock:^(QSObject *obj, NSUInteger idx, BOOL *stop) {
Copy link
Member

@tiennou tiennou Jul 18, 2016

Choose a reason for hiding this comment

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

Just a quick heads-up of sorts. I have concerns about using the block-enumerator form, after having read a little about it :

Just so I'm sure you're aware that throwing a bunch of concurrent threads on too-small-of-a-problem or on too-small-of-a-set-of-objects might not be worthwhile 😉 .

Copy link
Member Author

@skurfer skurfer Jul 18, 2016

Choose a reason for hiding this comment

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

I know it adds some overhead, but thought we could see a potentially large number of children and would want the concurrency. I had no idea where it became worth it to use that over fast enumeration, though.

In our case, “large” is something like right-arrowing into /usr/bin (about 1,000) or “Browse Tracks” (almost 10,000 for me). So only in extreme cases are we even getting close to the point where concurrency would be better. I’ll switch it to plain fast enumeration.

(Having seen that, I don’t think we need concurrent enumeration anywhere in QS unless the stuff inside the block is I/O bound.)

skurfer added 2 commits Jul 18, 2016
We're not dealing with the kind of numbers that make concurrent enumeration worth it. Also, avoid a ton of message sending to get the same identifier repeatedly.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 18, 2016

OK @tiennou, that should be better. I also added alt children since all of our children deserve love. 😃

NSString *parentID = [self identifier];
for (QSObject *child in newAltChildren) {
[child setParentID:parentID];
}
Copy link
Member

@tiennou tiennou Jul 18, 2016

Choose a reason for hiding this comment

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

There's a tab->space issue, and I think it hides a brace-mismatch thingy : the code in -setChildren: does the parenting thing on newArray != oldArray. This one does it unconditionally.

Copy link
Member Author

@skurfer skurfer Jul 18, 2016

Choose a reason for hiding this comment

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

There's a tab->space issue, and I think it hides a brace-mismatch thingy

It’s actually pretty easy to follow in Xcode (compared to this view), but the person making the changes needs to pay attention. 😁 I’ve moved it under the right if statement.

@tiennou
Copy link
Member

@tiennou tiennou commented Jul 18, 2016

I'd personally be tempted to rewrite both as

if (!children) {
  // remove key
  return;
}
// fix the children parents

But I'm also wondering, isn't the parent/child thing also part of QSObjectProvider ? Just wondering though...

@tiennou
Copy link
Member

@tiennou tiennou commented Jul 18, 2016

And I think the if (/* ptr in dict */ != /* new ptr */) only made sense in a pre-ARC world, but that ought to be general housekeeping stuff.

[obj setParentID:[self identifier]];
}];
NSString *parentID = [self identifier];
for (QSObject *child in newChildren) {
Copy link
Member

@pjrobertson pjrobertson Jul 30, 2016

Choose a reason for hiding this comment

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

I agree this is better. I always get a little bit scared when changing objects concurrently (in this case it's only setting parentID but you never know

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 30, 2016

What happens if I:

I'm assuming if I type Xcode then → and get "Quicksilver.xcodeproj" then the parent of "Quicksilver.xcodeproj" will be set as Xcode.
If I then search my catalog for "Quicksilver.xcodeproj" and then ← will I get Xcode or the folder containing that file? I should get the folder right?

@hmelman
Copy link

@hmelman hmelman commented Jul 30, 2016

Yes, I believe if you start a new search, find a file and ← you should see the containing folder, not an app based on recent files or default open with (this isn't iOS :).

If you navigate to the file from an app then ← should take you back the way you came.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 30, 2016

will I get Xcode or the folder containing that file?

You should get the folder. The handler’s parentOfObject: is consulted first, if it’s defined, which it is for files. So parentID shouldn’t matter in that case.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 30, 2016

What @hmelman described is how it works with these changes (and how it seems to be working anyway). Things that didn’t work before this:

  • → into a contact, then ←
  • → into a file tag, then ←
  • → into an artist, then ←

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 31, 2016

OK, but my concern is still valid (perhaps not for file objects, because they're a weird edge case that have a parentOfObject: defined.

Do this (I'm sure there are other ways of reproducing):

  • Install 1Password & the 1Pwd plugin
  • Enable the '1Pwd Logins' catalog entry in Prefs > Catalog > Plugins
  • Search for '1Password.app' in the first pane and press →
  • At this point, your code will be setting parentID on all these objects
  • Dismiss the interface
  • Search directly for a 1Password login (e.g. search for 'github' to find your 1Pwd GitHub login)
  • Press ←
  • you'll notice that 1Password is selected, since that's the 'parent' of the object. In reality, there is no parent for this object this time

Also, one other thing for your 3 cases:
If you → into a contact/filetag/artist then ← straight out, your search 'scope' has been lost (e.g. the search string is reset). I don't think this is an easy one to fix, and probably not a big deal, but...
Another easy way to see what I mean:

  • Search for 1Password.app (type the whole string). The results array will likely show just 1 or 2 results.
  • Right arrow into 1Pwd.app then left arrow back out
  • The results array will show your applications folder, with the 'scope' (previous search array/search string) being lost

@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 31, 2016

you'll notice that 1Password is selected, since that's the 'parent' of the object. In reality, there is no parent for this object this time

There wasn’t before. There is now. 😃 I see what you’re saying, but isn’t that better than nothing? It’s true that this is inconsistent, since if you searched for a Login item immediately after a relaunch, the parent would be missing. But the solution to that is probably to define parentOfObject: in the 1Password plug-in (and many others).

The results array will show your applications folder, with the 'scope' (previous search array/search string) being lost

I’m not really sure what should happen, but going through it a couple of times, the current behavior seems to match my expectations. Two other things I would say about that:

  1. I didn’t change the left-arrow behavior (in browse) at all, so this isn’t new.
  2. As I said in the opening comment, “My gut tells me all that left-arrow code needs to be refactored, but I’m not going any deeper for the moment.”

Are you saying I need to go deeper?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 21, 2016

This was in the 1.5.0 dev preview and no problems have been reported. I’m going to merge.

I believe any outstanding weirdness (which I haven’t seen in months of actual use) can be fixed by implementing parentOfObject: more frequently/consistently.

@skurfer skurfer merged commit 2947ff8 into master Oct 21, 2016
2 of 3 checks passed
@skurfer skurfer deleted the parents branch Oct 21, 2016
skurfer added a commit that referenced this issue Oct 21, 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.

None yet

4 participants