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

Default the third pane for "Assign Abbreviation" to the matched string. #1548

Merged
merged 1 commit into from Sep 17, 2013

Conversation

lgarron
Copy link
Contributor

@lgarron lgarron commented Jul 31, 2013

I've always thought that a blank third pane is not very useful.

Often I want to fix the ranking of an object after I've arrowed down to it. This diff makes it easy to fix such a problem:

  • Run "Assign Abbreviation" on the item, which will default to using what you searched.
  • Invoke QS again, and run the intended action.

screenshot 2013-07-31 16 45 30

matchedString appears to be the proper accessor exposed by QSSearchObjectView, although I don't know if it might have issues with e.g. special keyboards/characters.
However, this does work if you use the action with a dismissed-and-re-invoked QS direct object selected via text. (A hackier approach using partialString didn't work.)

Note that selections sent to Quicksilver don't have a matchedString, so there's no sensible suggestion (although you could make an argument for either the entire string version of the direct object, or for a string of the first letter of every alphabetic substring).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 1, 2013

Welcome aboard the QS Dev team ;-)

I guess by making the change yourself you've answered "how hard it would be to implement"!
Seems to work as advertised, but I don't like how we have to go through the delegate to get the dSelector. But right now I can't seem to think of any better alternative.

I also think what you've done is right RE no sensible suggestion. There's no way we could ever make a reasonable assumption as to what people want to assign for an abbreviation, so I don't think we should try...

@lgarron
Copy link
Contributor Author

@lgarron lgarron commented Aug 1, 2013

Seems to work as advertised, but I don't like how we have to go through the delegate to get the dSelector. But right now I can't seem to think of any better alternative.

I certainly don't have a good sense of best practices here. Poking around in the code seemed to suggest that the dSelector object was the place to get the matched string, and I found somewhere else in the code that got access to it like that. If there's a better way, I'd love to learn what, but I trust you if you say that you can't think of anything better. :-P

(I also don't know if textProxyObjectWithDefaultValue is the right way to create the textObject, but that's what the code below did, and it worked for this.)

Any testing/documenting/whatever I should do before this gets merged?

@lgarron
Copy link
Contributor Author

@lgarron lgarron commented Aug 1, 2013

Unrelated, but I just noticed: Is there an easy way to get the description at the top to respond according to text change in the third pane? All actions seem to do it for changes in the first pane, but none for the third (Try "Copy To" vs. "Rename").

@skurfer
Copy link
Member

@skurfer skurfer commented Aug 6, 2013

Is there an easy way to get the description at the top to respond according to text change in the third pane?

This is done by calling [commandView setStringValue:@"xyz"], where commandView is an iVar in QSInterfaceController. I see that the Web Search's "Search For…" action updates on-the-fly, as well as the "Process Text…" action, so that would be a good place to start.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 10, 2013

OK, I can't think of a better way of doing this at the moment, so you're good to go.

Do you want to do something with the command view string value, as Rob suggested before we merge?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 17, 2013

@lgarron is there anything more you want to do with this? Otherwise I'll merge :)

@lgarron
Copy link
Contributor Author

@lgarron lgarron commented Sep 17, 2013

I was interested in fixing the commandView issue, but it seemed too general when I looked into it. I think merging now would be good; I'll make a note to look at commandView later, for all actions.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 17, 2013

Cool, thanks Lucas

pjrobertson added a commit that referenced this issue Sep 17, 2013
Default the third pane for "Assign Abbreviation" to the matched string.
@pjrobertson pjrobertson merged commit c37917a into quicksilver:master Sep 17, 2013
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

3 participants