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

improve appearance for multiple dropped files #2263

Merged
merged 1 commit into from Dec 24, 2016
Merged

improve appearance for multiple dropped files #2263

merged 1 commit into from Dec 24, 2016

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Aug 19, 2016

I thought I had found all the places. Here’s one more.

That redisplay method keeps creeping outward, when we really only need it on the one class. If anyone has an idea on a cleaner way to handle this, let me know.

@tiennou
Copy link
Member

tiennou commented Aug 19, 2016

Make QSCollection actually useful ? I think the main issue is that collections don't exist at the "model" layer, and thus we keep piling on stuff in view classes because it doesn't exist.

For example, having a real QSCollection class (modulo implementation), would allow us to "kill" QSCollectingSearchObjectView which doesn't help wrapping your head around the QSObjectView API), allowing every interface to work with collections — instead of the current "whatever the interface developer" used in its NIB file.

My 2 cents, obviously 😉.

@tiennou
Copy link
Member

tiennou commented Aug 19, 2016

Or (now that I've looked at the code), override -QSCollectingSOV setObjectValue: instead of defining a new selector ? I'm completely out of my mind here, but this is a fix for what is called a "violation of the Liskov substitution principle", so unless there were/are valid reasons to have introduced that selector in the first place, this ought to be more correct — modulo what happens if you select something that can be split — since, given we don't have QSCollection, any object can be split into anything, which IMHO is the crux of the issue.

@skurfer
Copy link
Member Author

skurfer commented Aug 19, 2016

override -QSCollectingSOV setObjectValue: instead of defining a new selector ?

I don’t know about that, but it looks like overriding selectObjectValue could work. Problem with that is, QSObejctView doesn’t have select….

@tiennou
Copy link
Member

tiennou commented Aug 19, 2016

AFAIK -selectObjectValue: should be merged with -setObjectValue:, because there should be only one way to say "this view displays this object", and the NSView API for that is -setObjectValue:.

Anyway, as I'm pretty sure I've already said, my opinion on the QSInterface part of the code is that it's hopelessly "broken". I mean, the characterization of this is here : you have -setObjectValue:, which does a bunch of stuff, doesn't call its super implementation but goes through -select..., and -select..., which calls -set... on super. I mean, what could possibly go wrong ? Why is it written like that ? Which method should I override so I — as a subclass of QSSearchObjectView, like QSCollectingSearchObjectView is — am sure I get informed when my object changed ? What's the actual difference between -set, -select, and -redisplay ?

@skurfer
Copy link
Member Author

skurfer commented Aug 19, 2016

AFAIK -selectObjectValue: should be merged with -setObjectValue:, because there should be only one way to say "this view displays this object", and the NSView API for that is -setObjectValue:.

I’m inclined to agree (but that’s not going to happen on this branch).

What's the actual difference between -set, -select, and -redisplay ?

I think we’ve all wondered that. I at least looked into that and documented the answer recently.

https://github.com/quicksilver/Quicksilver/blob/master/Quicksilver/Code-QuickStepInterface/QSSearchObjectView.h#L98

But in the end, not having confusing behavior would be better than documenting it.

@skurfer skurfer merged commit caab0ea into master Dec 24, 2016
2 checks passed
skurfer added a commit that referenced this issue Dec 24, 2016
@tiennou tiennou deleted the multidrag branch Apr 16, 2017
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