Cleanup QSCatalogEntry API.

[tiennou]

Please note this depends on #1585, because I've needed the warning count to be low (so you'll actually see both in the diff until the other gets merged).

I got started on that because I tried to run the old appledoc branch I got here, which raised an exception because of the _contents/contents getter not having the right name, and when I saw _contents/contents + the property I just triggered cleanup mode.

I still have a bunch of things to fix (since I've added some warnings), but for now I'll wait on the various other branches getting merged...

[skurfer]

Grrr. I made a bunch of comments on various commits, but apparently GitHub.app links to the commit under my fork where no one will ever see it. I'll try to find a recreate them here.

[tiennou]

@skurfer: I think it's better if you comment on the PR's diff ("Files changed" at the top) directly instead of individual commits. That makes it possible for me to "hide" your comment if I change the diff in some way, which is pretty nice for reviews.

[tiennou]

Also, I saw your original comments emails — though I'm not sure how, maybe they're attached to commit ids and thus are global.

[tiennou]

Comments salvaged. But please oh please don't do it again ;-).

[skurfer]

I think it's better if you comment on the PR's diff

In retrospect, that's what I should have done. There were also a couple of things I almost commented on that were fixed in later commits.

[tiennou]

Yeah, but I just remembered that diff-comments aren't really compatible with force-pushes and rebases, so you have to be careful...

[skurfer]

After messing with 10.9 a bit last night, I rebooted and most of my catalog hadn't populated. I hit ⌘R in the main interface a couple of times, but saw no difference. Rescanning each entry individually was fine.

I see this in the console every 10 minutes, and every time I hit ⌘R: Multiple Scans Attempted

So it looks like something is getting wedged. I cleared caches and restarted and the scan finished fine, plus ⌘R is no longer generating a log message, so this could be a pain to reproduce.

[tiennou]

I actually noticed the bug you've saw while running it. I think this now works better — though it still deadlock in unexpected places, but I didn't do anything explicit to fix it, so YMMV.

[pjrobertson]

Can this be rebased? What's already been merged into master :/

[pjrobertson]

I'm LOVING how 'modern' and 'hip' QSCatalogEntry is looking :)

Awesome work Etienne.

[tiennou]

Yeah, looks so much better that before. At least now there's a clear separation between what's public and what's private. Please note that this is unfinished and not yet ready for merging, but I'll gladly bring over changes from master from time-to-time (because I'm gonna need some other stuff in other PR ;-)).

I think this is written badly, and so difficult to read (not your fault).

Yeah, I got myself wondering at what it's trying to really do. It's not the only place where I see that, but I think it's because there's a nil => default => something expectation, like catalog sources are enabled by default, unless explicitly disabled by the user.

One of the things I'm currently contemplating is the scanQueue "problem". I'm not really fond of the "one-queue-per-entry" approach, but maybe I can keep it that way until I've cleaned up enough of QSLibrarian and QSCatalogEntry so that it's clear which is responsible for what...

Right now, I have a tendency to think of QSCatalogEntries as repositories of objects generated by QSObjectSources on behalf of a request by QSLibrarian, but it just goes through loops in order to achieve that (and I gather that any us us that had to look at the threading bugs before we scan-queued the hell out of it can testify that following async code littered between two files and a handful of methods is a brain-twisting experiment ;-))... I'm more-or-less planning to only keep the index stuff and the object repository part in there, the whole scan process will get moved to QSLibrarian, with maybe a few convenience methods for easy "scan yourself, catalog entry !" fun ;-).

Also, that's doesn't really belong here — at least until I start rewriting QSLibrarian — but should I try and make preset entries user-editable ? I don't think it's that hard, in fact I'm pretty sure I've spotted some commented out code that used to handle that...

[pjrobertson]

Is there any way we can merge a minimal amount of this into master so that we can merge #1614?

One of the things I'm currently contemplating is the scanQueue "problem". I'm not really fond of the "one-queue-per-entry" approach, but maybe I can keep it that way until I've cleaned up enough of QSLibrarian and QSCatalogEntry so that it's clear which is responsible for what...

I remember when I implemented the one queue business, you wanted to use dispatch_barriers, but they were 10.7+ only. Now we've dropped 10.6 I guess we could go down that route... :)

but should I try and make preset entries user-editable ? I don't think it's that hard

The way things have been for a long time is that you have to duplicate them. Although I agree that this has been somewhat confusing for lots of users. I tried to rectify it recently by disabling all the editable views in the preset (#1341)
I don't think it's urgent.

[pjrobertson]

@tiennou any time to spare on this? I'd like to merge #1614, but it relies on this (#1601) which in turn relies on #1585. Aaargh!

[HenningJ]

Can one of the admins verify this patch?

[tiennou]

I'm working on it while keeping the IRC channel warm, in case one of you guys are around ;-).

[tiennou]

Ok to test ?

[tiennou]

Severely edited

Okay, looks good to me. I tested by chown root:wheel-ing QS's Cache directory after deleting its content, and the scanning happens correctly. I think I lost the Task Viewer though.

Anything else that looks wrong to you ? Else feel free to merge that one, then merge #1614.

I'll write some appledocs so when #1614 gets merged we'll have something pretty to see ;-).

[tiennou]

Okay, here's documentation. I actually caught some weird things (mostly calling addObjectsFromArray: with a nil argument, tough I left some of them), and stopped when my brain exploded because stuff felt wrong : see -contentsScanIfNeeded: for non-groups, which doesn't call scanForced: and thus never causes the scan task to update, likely causing the bug I was seeing, scans that don't trigger the Task Viewer (master is affected too, I didn't change it yet). I'll put back my refactor hat before continuing ;-).

Additionally, if you wanna take a look, I pushed an catalog-appledoc branch which merges #1614 with this one so you can take a look at the docs (though I imagine you could also read the .h file, but it's less poignant ;-)). Now I'm not fond of the automatic linking appledoc does on method names, so I'll have to see if that can be disabled somehow...

[skurfer]

I found one small problem. If you disable a preset, it’s re-enabled after a relaunch. Changes to custom entries appear to get saved correctly. Haven’t had time to dig further.

[skurfer]

Note that #1688 was merged, so if you want to handle it a different way, you’ll need to rebase. Actually, that’s probably why this is conflicting now.

Whatever you do, just try not to make a liar out of me in the section I just added to the plug-in docs. 😃

Update: I also wanted to mention that I have a feeling I should have done it the other way around. That is, have requests for contents return the filtered list, and create a new method that you would call explicitly when you want everything, including what the user disabled (like allContents or something). The entry’s preferences are the only place I can think of where we’d use that.

[tiennou]

Heh, let's do some brainstorm since QSLibrarian is next on my rewrite plate — after I fix the half-model/half-controller mess that is QSObjectSource. I was envisioning :

• make entries always return all objects (they're not responsible for omitting stuff).
• add a method like [QSLibrarian objectsForEntry:includeOmitted:] that takes either a QSCatalogEntry or an identifier and whether to include omitted objects or not, which returns the currently indexed objects (and doesn't trigger a source scan or whatever).

Since you might have more experience with the plugins side-of things than me, would that be sufficient to cover all the use cases ?

And I'll provide those methods you've added in the meantime. Who likes perjury anyways ;-).

[skurfer]

make entries always return all objects (they're not responsible for omitting stuff).

Well, there are three things I fixed in #1688:

• QSLibrarian recalculateTypeArraysForItem: ignores disabled objects.
• The count for an entry will ignore disabled objects.
• Right-arrowing into a catalog entry object will not show disabled objects.

If QSLib was exclusively responsible for knowing what’s disabled, we could re-implement the first item another way, but for the other two, wouldn’t it make more sense for the entry itself to be aware of what’s enabled/disabled?

add a method like [QSLibrarian objectsForEntry:includeOmitted:] that takes either a QSCatalogEntry or an identifier and whether to include omitted objects or not, which returns the currently indexed objects (and doesn't trigger a source scan or whatever).

OK, that could be used by the entry to get information about itself and handle the latter two fixes, but that wouldn’t be very obvious.

Are you wanting to do it this way because the list of disabled objects is global and not technically part of a specific entry?

Since you might have more experience with the plugins side-of things than me, would that be sufficient to cover all the use cases ?

Since we’re mostly talking about what to do after the fact with whatever the plug-in returns (from objectsForEntry:), I don’t think we’re going to affect them.

Plug-in authors do need to be aware of, and respect user-disabled objects when populating children, third pane, etc. but that’s nothing new. It’s just something we’ve been overlooking until now. That’s what I recently documented. Speaking of…

And I'll provide those methods you've added in the meantime.

No need to keep the same methods (contents and enabledContents). I didn’t mention them by name. All I said was that if you ask for objects by type (-[QSLib arrayForType:] or -[QSLib scoredArrayForType:]), the disabled items will be removed for you. So as long as that remains true, we’re fine.

Like I said, maybe it makes more sense to have contents give back an array that excludes the disabled objects automatically, and have a new one like allContents that includes them. The reason I think that is (as far as I know) there’s only one place that should show them all: The contents tab in the entry’s prefs. So it would be easier if all the other places that call contents just didn’t have to think about disabled items.

[HenningJ]

ok to test

[skurfer]

This should return the same value as canBeIndexed, right? Or maybe we don’t even need shouldIndex. It’s only used by the NIB (under Attributes), but that could be updated to point directly to canBeIndexed.

[skurfer]

On the subject of the NIB…

If the indexable boolean is read-only, do we need to show a disabled checkbox for it on all entries? Why not a label with its “Visible” property bound to canBeIndexed? I’d also rename “Include in Global Catalog” to “Enabled” or “Entry Enabled”. That one seems to cause a lot of confusion. I didn’t even know it was equivalent to enable/disable until looking yesterday. 😛

[skurfer]

One other thing to fix here: I just tried using the QSCatalogSourceInvalidated notification for the first time and it doesn’t seem to do anything. I think it’s because we call [loopItem scanForced:NO] in -[QSLibrarian reloadSource:]. Shouldn’t that be [loopItem scanForced:YES]?

[tiennou]

Rebased.

I think I handled most things we talked about, except :

• XIB changes
• the -contents/-allContents thing. I'm not fond of the performance hit we're taking right now because of that, but I see no other way to do it, other than making the object source check for omitted-ness before putting objects (sounds messy API-wise + lots of places to change), or maybe just make -contents cache its value and -allContents call -scannedObjects directly...

[skurfer]

I merged this into a copy of master and dealt with the conflicts. Seems OK except for the two things I mentioned before.

1. Presets can’t be disabled
2. Invalidating a source doesn’t cause a rescan (Well, I assume. I just saw that the boolean is still NO.)