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

Collections of files should never get an identifier. #1112

Merged
merged 3 commits into from Oct 3, 2012

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Sep 14, 2012

Fixes #555.

This makes it possible (again) to create triggers with multiple direct objects (like Adium + Colloquy > Run). There are still wonky things going up, like if you reedit the trigger, the collected applications will get added again to the list.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 14, 2012

Here's a big rant about that :
I'm all in favor of cleaning up the mess w.r.t to collections of stuff (a.k.a the more I look at it, the more I'm baffled by how it works). There's a special QSCollectingSearchObjectView that prevents interfaces from benefiting from those unless you update it, there's load of special case code everywhere to account for that (mainly QSObject_FileHandling, but the bug lied somewhere in there, and I'm pretty sure there can be more of them in other places).

My question is : who is in favor of beefing up QSCollection, adding a real QSCollectionType, and moving the bulk of QSCollectingSearchObjectView to QSCollection, tweaking things like +objectByMergingObjectsFromArray: to return a QSCollection ? This would allow a nicer icon to be generated in the interface (I foresee something like the blue object plus a small overlay with the various collected objects icons), because right now it defaults to a generic icon if all objects have the same type (like the generic application icon), or a question mark with a completely useless details in case you try something like Home Folder + /Applications + Adium > Open, which gives me Open 3 items in / and a stack of generic document icons when encapsulated.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 14, 2012

There are still some issues still w.r.t encapsulating commands. Try with that : Home Folder + /Applications + Adium > Open, then Ctrl-Return to encapsulate, no Run action for you, though this is a command (I get Show contents menu. Now close the interface, reopen and do a down arrow in the first pane (you'll notice the first pane drops its collection) and Run appears and works.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 16, 2012

@skurfer will be shocked by this sneaky pull request Etienne.

He's been working on it for the past few days. It may be worth popping onto IRC when we're all about to discuss the best ways to go about this, and finding out where Rob's got to on his work.

I think having a QSCollection class sounds like a good idea, first off though.

I'm intrigued as to how this fixes the problem, interesting! So if you don't set an identifier for a collection, then what gets set as the identifier in the trigger's .plist?
I could try it out… :P

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 17, 2012

Heh, I'm somewhat baffled too that it looks this simple ;-). It's just that the method I'm removing would set an identifier on collections and the Trigger -dictionaryRepresentation would pack that, instead of a real archive.
There are other ways to fix that :

  • this one.
  • check for an object's -count before archiving in QSTrigger.
  • make a real QSCollection and have a -dictionaryRepresentation that does it.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 17, 2012

This change makes sense to me (though I haven't had a chance to test its practical effects). We talked about it on IRC first @pjrobertson, so it wasn't a complete shock. :-) Though, the reason I haven't gotten very far on it myself is I've been looking for the "big fix" that takes care of 6 or so problems. Not just the one. I've been thinking about it over the weekend and I created #1115 to discuss the direction we should take (rather than clutter up this small pull request with a long, involved discussion).

I'll try this out tomorrow.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 17, 2012

I'll be about on IRC tomorrow, if there's any chatting to do.
I guess I didn't think you'd be talking when I was away… :P

On 17 September 2012 04:54, Rob McBroom notifications@github.com wrote:

This change makes sense to me (though I haven't had a chance to test its
practical effects). We talked about it on IRC first @pjrobertsonhttps://github.com/pjrobertson,
so it wasn't a complete shock. :-) Though, the reason I haven't gotten very
far on it myself is I've been looking for the "big fix" that takes care of
6 or so problems. Not just the one. I've been thinking about it over the
weekend and I created #1115https://github.com/quicksilver/Quicksilver/issues/1115to discuss the direction we should take (rather than clutter up this small
pull request with a long, involved discussion).

I'll try this out tomorrow.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1112#issuecomment-8603858.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 18, 2012

OK, I found I couldn't actually test this because when you encapsulate multiple applications, only the most recent one is replaced in the first pane. The others remain selected along with the new command object, preventing the right actions from appearing. Adding this to encapsulateCommand in QSInterfaceController fixes it:

[self clearObjectView:dSelector];

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 18, 2012

Yeah, that's what I was trying to explain in my second comment above ;-). The new commit should fix that in QSCollectingSearchObjectView directly.

@skurfer
Copy link
Member

@skurfer skurfer commented Sep 18, 2012

OK, that seems to fix it. I wonder if that would allow us to remove some of the other calls to clearObjectView:dSelector, but it can wait for the mythical QSCollection branch. :-)

A question on that: All of the existing actions expect a QSObject. How will we pass a collection to an action? Will it be a subclass of QSObject, or will there be some method on it that returns a QSObject representation?

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 18, 2012

I replied on #1115 about the QSCollection API.

skurfer added a commit that referenced this issue Oct 3, 2012
Collections of files should never get an identifier.
@skurfer skurfer merged commit 487d156 into quicksilver:master Oct 3, 2012
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.

3 participants