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

Prevent user-disabled objects from appearing #1688

Merged
merged 3 commits into from
Dec 2, 2013
Merged

Prevent user-disabled objects from appearing #1688

merged 3 commits into from
Dec 2, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Nov 22, 2013

Unchecking individual QSObjects in the catalog would prevent them from appearing when you searched in the first pane, but they would still appear in the third pane and when right arrowing. I’ve fixed that.

Ideally, we could take care of this as part of #1601, but I gather @tiennou is too busy right now to take on additional features. I’ve at least tried to do something that won’t conflict.

We can’t globally make the change in scannedObjects or
contentsScanIfNeeded:, because there are places where the objects
should still appear. For one, they should appear in the prefs so you
can re-enable them.
@pjrobertson
Copy link
Member

This bug was kind of addressed a while back. I added an includeOmitted BOOL to QSLibibrarian.m:L619

It might be worth looking at that to see if it can be changed/tweaked or if the logic from what you've done can be combined in some way.
Whilst discussing that change, I remember we (well, I at least) wanted to keep things such that right arrowing into an object would show all objects. What if, for example you only want to index your .txt files (or maybe a better use case: only folders) in ~/Documents, but when you actually → into ~/Documents you still want to see all your files, right?

@skurfer
Copy link
Member Author

skurfer commented Nov 23, 2013

This bug was kind of addressed a while back. I added an includeOmitted BOOL to QSLibibrarian.m:L619

OK. Does that just take care of the first pane, or does it get other places, too?

Whilst discussing that change, I remember we (well, I at least) wanted to keep things such that right arrowing into an object would show all objects.

Rings a bell. I think I took the position that the browsing the filesystem should always tell the truth, ignoring catalog stuff. So I think we agree, and that’s how this works.

The changes should only affect the catalog, but browsing/right-arrowing should still be the same as before. The exception to that is right-arrowing into a catalog entry itself, since I’ve gone out of the way to prevent that from showing disabled objects. But that makes sense if you ask me. It matches what you see checked under “Contents” in the prefs that way.

@pjrobertson
Copy link
Member

OK. Does that just take care of the first pane, or does it get other places, too?
If you look at line 619 of QSLib, it basically says - only omitted unchecked items if the search set is defaultSearchSet - i.e. we’re searching the whole catalog. So this would apply to the 1st/3rd pane. It could perhaps be more general and changed to “if the search mode is ‘filter all’, omit unchecked items” but it may be difficult to get that setting from QSLib so perhaps your way is the best way.

Rings a bell. I think I took the position that the browsing the filesystem should always tell the truth, ignoring catalog stuff. So I think we agree, and that’s how this works.

Yep, that’s what I think. Right arrowing into catalog entries sounds right as well :)

On 24 Nov 2013, at 04:56, Rob McBroom notifications@github.com wrote:

This bug was kind of addressed a while back. I added an includeOmitted BOOL to QSLibibrarian.m:L619

OK. Does that just take care of the first pane, or does it get other places, too?

Whilst discussing that change, I remember we (well, I at least) wanted to keep things such that right arrowing into an object would show all objects.

Rings a bell. I think I took the position that the browsing the filesystem should always tell the truth, ignoring catalog stuff. So I think we agree, and that’s how this works.

The changes should only affect the catalog, but browsing/right-arrowing should still be the same as before. The exception to that is right-arrowing into a catalog entry itself, since I’ve gone out of the way to prevent that from showing disabled objects. But that makes sense if you ask me. It matches what you see checked under “Contents” in the prefs that way.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

skurfer commented Nov 25, 2013

If you look at line 619 of QSLib, it basically says - only omitted unchecked items if the search set is defaultSearchSet - i.e. we’re searching the whole catalog. So this would apply to the 1st/3rd pane.

I think the reason it wasn’t working before based on the includeOmitted boolean was two-fold:

  1. That only kicks in when you start typing to search, so you could still see the omitted things if you arrow down without searching.
  2. The third pane often contains an arbitrary collection of objects, so searching there isn’t necessarily going to be a particular set or a particular mode. But if we filter the pool of available objects before-hand, none of that matters.

Something else worth noting about this branch: If an object source gets children by calling [self objectsForEntry:], or by some other manual process, omitted items will still be included. (That’s why browsing files still works.) But if it gets children from the catalog, omitted items won’t be there.

Again, I think that behavior makes sense. If you explicitly disable a contact, it shouldn’t appear when right-arrowing into Contacts.app.

I think it should be up to the plug-in to decide the best behavior. But it might be worth documenting this (if this gets merged) so developers will know what to expect. I can also explain how to use some manual process to get children, while still omitting disabled objects, in case anyone has a need to do that.

@pjrobertson
Copy link
Member

OK, please make "documenting this" a post-prerequisite ;-)

pjrobertson added a commit that referenced this pull request Dec 2, 2013
Prevent user-disabled objects from appearing
@pjrobertson pjrobertson merged commit aad48b3 into master Dec 2, 2013
@pjrobertson pjrobertson deleted the omitted branch December 2, 2013 09:48
@skurfer
Copy link
Member Author

skurfer commented Dec 2, 2013

OK. I think Etienne was going to fix this some other way, but that can always be done later. I’ll update the documentation.

@pjrobertson
Copy link
Member

Oops, sorry if Ive messed things up in my excitement/mad rush now I finally have decent internet.

I’m on IRC if need be. 20 minutes or so then it’ll be bed

On 2 Dec 2013, at 22:42, Rob McBroom notifications@github.com wrote:

OK. I think Etienne was going to fix this some other way, but that can always be done later. I’ll update the documentation.


Reply to this email directly or view it on GitHub.

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.

2 participants