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

Show the alternate action (if available) when ⌘ is pressed. Fixes #189 #1163

Merged
merged 23 commits into from Oct 29, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 9, 2012

Adds a (horribly named) property used by the actionSelector to remember its original action and flip back/forward between it and the alternate as ⌘ is pressed and de-pressed.
Since the aSelector's objectValue is set to the alternate action immediately once ⌘ is pressed, this change also removes the need to do a further check down the line (in QSInterfaceController.m). Mmmmm tidiness :)

Something to discuss is whether or not we want to alter the appearance of the action to indicate to users that it is an 'alternate' action. Options could include:

  • Add an 'alternate' bubble to the action icon
  • Add an image like http://i.imgur.com/UzeRq.png to the action
  • Append (alternate) to the label/name

…cksilver#189

Adds a (horribly named) property used by the actionSelector to remember its original action and flip back/forward between it and the alternate as ⌘ is pressed and de-pressed.
Since the aSelector's objectValue is set to the alternate action immediately once ⌘ is pressed, this change also removes the need to do a further check down the line (in QSInterfaceController.m). Mmmmm tidiness :)
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 9, 2012

Cool. It works! Some problems, though.

  1. The list of actions (if visible) is closed when you press ⌘
  2. Only the selected action is changed. Shouldn't they all change (if you could see them)?
  3. The list of actions is wiped out. Only the one you had selected remains in the list.

I had it working for all actions on my branch for this (but there was a deal-breaker I can't recall now). If you want, I can dig it up and see what I did.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 9, 2012

I had it working for all actions on my branch for this (but there was a
deal-breaker I can't recall now). If you want, I can dig it up and see what
I did.

I knew there should have been more haste and less speed ;-)
I'll take a look at what you've said, but if you can push your branch to GH
it might be useful as a reference (plus, I know how much you like having
loads of branches :D )

On 9 October 2012 21:39, Rob McBroom notifications@github.com wrote:

Cool. It works! Some problems, though.

  1. The list of actions (if visible) is closed when you press ⌘
  2. Only the selected action is changed. Shouldn't they all change (if
    you could see them)?
  3. The list of actions is wiped out. Only the one you had selected
    remains in the list.

I had it working for all actions on my branch for this (but there was a
deal-breaker I can't recall now). If you want, I can dig it up and see what
I did.


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

@hmelman
Copy link

@hmelman hmelman commented Oct 9, 2012

I might have an opinion if I had any clue what you mean by alternate action. I didn't follow the description in the original post.

And in the proposed image is the red background indicative of an invisible property? Otherwise I think it would clash with various interfaces (this appears in the results list for actions? ok maybe that doesn't change much with interfaces).

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 9, 2012

I put my stashed work from months ago in a branch. Have a look.

In playing with it, the only problem I see is that the selection jumps back to the first item in the list when you hit ⌘. That must have been why I never submitted it.

As for alternate action @hmelman, those are the things that you can run with ⌘. For instance, "Reveal" is an alternate for "Open", "Open URL in Background" is an alternate for "Open URL", "SSH as root" is an alternate for "SSH", etc.

Fix code where the resultArray is cast to a mutable array. Very dangerous!

Fix a typo and use a property for a resultController's resultTable iVar
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 10, 2012

I have fixed the first and thrid of @skurfer's points,

The 2nd one is an interesting one: I liked your idea of changing all the actions (I see from your code) but I think there might be a few quirks in practice. For example:

You currently have 'Open, Reveal, Open Url, Open URL in Background' in your aSelector results view.
With your idea, if you hold ⌘ you will get 'Reveal, Reveal, Open URL in Background, Open URL in Background':
repeating some of the actions

What I've done is to jump to the associated alternate action (and back) if it exists in the results list when ⌘ is pressed.
I do think your idea is better, if you can come up with a way to solve that quirk ^^

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 10, 2012

What I've done is to jump to the associated alternate action (and back) if it exists in the results list when ⌘ is pressed.

I really like this after seeing it in action, but I still wish there was a way to indicate that the other actions have alternates too.

With your idea, if you hold ⌘ you will get 'Reveal, Reveal, Open URL in Background, Open URL in Background':
repeating some of the actions

I knew that would happen before I even made the changes, but it never bothered me. Pressing ⌘ on an action that has no alternate just runs the normal action, so leaving them in place when ⌘ is held properly reflects what's going on. If the goal is to make them more obvious, this does the trick. In fact, it might even be more obvious what's going on if you see one action change into another. Maybe. :-)

Is it just me, or does text ⇥ Find With… completely lock up Quicksilver on this branch?

And I don't think you can use a copy for the resultArray or sourceArray. If an action tries to update the results, the first thing it does is check to see if the current array is the same object as the one the action refers to. (Although this appears to be partially broken on master already. Might need to do a git bisect.)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 10, 2012

I have added an extra column to the actions pref pane table, showing an action's alternate action. Hopefully it'll make them easier to find.
I have also added a few more alternates to actions in the core plugin.

Todo: A bug I've found - the 3rd pane gets wiped when you change between the alternate and original actions. I know how to fix :)

@hmelman
Copy link

@hmelman hmelman commented Oct 10, 2012

This sounds pretty cool. Is there a list of all the alternative actions? Are these all pairs or is there possibly a chain? Given the above I could see different people ranking the "in background" actions either higher or lower than the normal versions. So it makes sense that the pairs are alternates for each other. That's also easier to learn. It's not clear to me that Open being the alternative of Reveal is that useful, but as a pair, maybe it makes sense. I'm curious about the other ones to see if there's a pattern to the pairings or not.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 10, 2012

I'm curious about the other ones to see if there's a pattern to the pairings or not.

I've added a few more 'pairs' to this branch, and you'll be able to see them in the Actions prefs in the next version of QS (hopefully). For the time being, here's a screenshot of the ones I have:
alternate actions

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 10, 2012

Since there seems to be a fair amount of space available, what do you think about appending keystrokes to the column names?

So they'd become “Action ()” and “Alternate Action (⌘)” or something like that. Make the feature even more discoverable.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 10, 2012

Since there seems to be a fair amount of space available, what do you
think about appending keystrokes to the column names?

I'd stretched the prefs window out way bigger than it normally is, tbh
there isn't much space.
I was just going to add another sentence to the top of the window
explaining what the 'alternate actions' column meant. Of course, then we'll
need to localise it all over again,.. :)

Thoughts?

On 10 October 2012 20:16, Rob McBroom notifications@github.com wrote:

Since there seems to be a fair amount of space available, what do you
think about appending keystrokes to the column names?

So they'd become “Action ()” and “Alternate Action (⌘)” or something
like that. Make the feature even more discoverable.


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

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 10, 2012

Regardless of window size, the names of the actions are still going to be much longer than the headings, so I think we can afford the extra. And I think it pretty clearly gets the idea across in three (unlocalized) characters instead of adding a bunch of text. :-)

@hmelman
Copy link

@hmelman hmelman commented Oct 10, 2012

Sounds like a clever solution to me.

Howard

On Oct 10, 2012, at 3:34 PM, Rob McBroom notifications@github.com wrote:

Regardless of window size, the names of the actions are still going to be much longer than the headings, so I think we can afford the extra. And I think it pretty clearly gets the idea across in three (unlocalized) characters instead of adding a bunch of text. :-)


Reply to this email directly or view it on GitHub.

pjrobertson and others added 3 commits Oct 11, 2012
Also: Save the original action for the mnemonic, not the alternate action
disable the paste action for multiple items. Fixes quicksilver#593
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 16, 2012

One small thing is that the alternate action remains visible when other modifiers are held. For instance, if I'm about to hit ⇧⌘I to run the Get Info action. Seems like it should only show the alternate if ⌘ is the only modifier held. I think I struggled with the same thing on my branch and couldn't quite figure out the correct mask.

Related to that, if the "capitalized keys" pref is enabled I wonder if we could move focus to the aSelector when ⇧ (and only ⇧) is held down. Much like the original goal with alternate actions, that would make the behavior more obvious and discoverable, and more importantly, it might be a viable work-around for #1030.

skurfer added 7 commits Oct 16, 2012
This was getting called immediately after `makeKeyAndOrderFront:`, causing a lot of code to be run twice. The documentation says it generally shouldn't be necessary and I don't see any difference without it.
The call to `nextEventMatchingMask:` seemed to be "replaying" the event, causing a lot of code to be run again.
The details were being appended to the `status` string *before* figuring out which result was currently selected.
Previously, the `details` method would need to be invoked *twice* to yield the correct result. (The first invocation would cache the details, and the second would return it from the cache.)

close quicksilver#1148
Whether or not the details can fit in the results list has nothing to do with whether or not they can fit in the main interface. In the context of the results list, the details string is already being handled appropriately by `QSResultController`.
reproduces work done by @pjroberston - fixes quicksilver#525
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 19, 2012

Found another small issue. To reproduce:

  1. Call up a Web Search in the 1st pane
  2. Make sure the action is “Search For…”
  3. Go to the third pane and paste some text with ⌘V

Holding command prior to hitting V switches to the alternate action (which is fine), but once you hit V, the alternate sticks. I assume it stops watching for modifiers once you press a “real” key.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 25, 2012

I've fixed the two issues related to 'alternate actions'.

I like your idea on ⇧ highlighting the 2nd pane, and as a potential fix to #1030. I want to discuss this some more so I'll wait for you to be on IRC :)

Also: only remove the `alternateActionCounterpart` if the aSelector `clearSearch` is called
@skurfer
Copy link
Member

@skurfer skurfer commented Oct 25, 2012

Looks good now. Some weird things happen with details, but I think it's fixed when this is combined with #1151. I want to try it out for a few hours before merging.

skurfer and others added 2 commits Oct 25, 2012
I think this was meant to be a fallback value, but since primaryType is *always* a string, it was returning the fallback the first time this method was run. After that, the cached value would be returned.
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 26, 2012

I couldn't reproduce your hitting ⌘ 50 times issue. I do notice, even with #1151 merged, that the details in the interface are still not updated.
In either Nostromo (top) or Bezel (bottom) tab to the 2nd pane and hit ⌘. The text still reflects the previous action. Something else to fix :)

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 26, 2012

In either Nostromo (top) or Bezel (bottom) tab to the 2nd pane and hit ⌘. The text still reflects the previous action.

Ah yes, the ol’ “command string”. I think you just need to post a SearchObjectChanged notification when toggling the actions.

* Use the `updatesSilently` BOOL to only disable the certain areas that should not be updated when the action is changed (using ⌘)
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 26, 2012

Ah yes, the ol’ “command string”. I think you just need to post a SearchObjectChanged notification when toggling the actions.

Hmmm... that may be a solution, but not one I want to take. I've specifically disabled that notif in this case since it messes with other things (see QSSearchObjectView.m:L571)

.... a few hours down the line:
I've changed things round a little, so this should work now :)

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 26, 2012

Merge remote-tracking branch 'origin/issue1148' into alternateActions

I don't suppose you meant to do that? :-)

Well, we can close that pull request and work from this one, eh? It was complete as far as I'm aware.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 26, 2012

I don't suppose you meant to do that? :-)

Oh, not again. ;-)
OK yeah close #1148 and I'll check to make sure it's all OK from my end,
then we can joint merge :)

On 26 October 2012 20:36, Rob McBroom notifications@github.com wrote:

Merge remote-tracking branch 'origin/issue1148' into alternateActions

I don't suppose you meant to do that? :-)

Well, we can close that pull request and work from this one, eh? It was
complete as far as I'm aware.


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

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on b0a31c1 Oct 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skurfer
I think the reasoning behind doing this was so that when the line height is < 34, the details aren't shown at all in the results list.

If you change the results list height to say 26 you'll notice that the details still show, even though they're half chopped off. But for some reason this happens in ß70 so I don't think it was something you added. Still something we should fix.

Also - BezelHUD never shows details in the results list (no matter what the line height) so really details should always show in the gutter, not just when lineHeight < 34... but this is an anomaly which I don't think we should bother working around

skurfer
Copy link
Member Author

@skurfer skurfer commented on b0a31c1 Oct 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reasoning behind doing this was so that when the line height is < 34, the details aren't shown at all in the results list.

But this isn't the results list, so it shouldn't be checked here.

If you change the results list height to say 26 you'll notice that the details still show, even though they're half chopped off.

What the…? I've never seen that with any version, and I played with the setting a lot when working on this. But it definitely happens. Anyway, that's a bug in the results list, not here.

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented on b0a31c1 Oct 27, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skurfer
Copy link
Member Author

@skurfer skurfer commented on b0a31c1 Oct 29, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results list uses QSObjectCells to display the object

Oh, well that explains a lot. I'll look into it.

@skurfer skurfer mentioned this pull request Oct 27, 2012
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 27, 2012

Other than my 2 comments on #1151 (let me know if you don't get notified about them), this is ready to go from my end.

@skurfer
Copy link
Member

@skurfer skurfer commented Oct 29, 2012

OK, pull down the latest issue1148 branch and cherry-pick these two:

  • a2d1c31e1a26760e0b603ad2416931a92da88a6a
  • abfffc249dd920bce4114e88b9e51c6b59167f76

On that second one, it only checks the row height on start up, so it can still get screwed up if you change the height, but only until the next relaunch. I couldn't find a method that runs when the row hight changes, and testing it every time an object is displayed seemed way too expensive.

skurfer added 2 commits Oct 29, 2012
There's probably a good reason someone wrote code to generate them in real-time instead of storing them up-front when the object is created.
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Oct 29, 2012

OK picked and pushed.

I am currently looking into using KVO to change the setting.

On 29 October 2012 18:08, Rob McBroom notifications@github.com wrote:

OK, pull down the latest issue1148 branch and cherry-pick these two:

On that second one, it only checks the row height on start up, so it can
still get screwed up if you change the height, but only until the next
relaunch. I couldn't find a method that runs when the row hight changes,
and testing it every time an object is displayed seemed way too expensive.


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

skurfer added a commit that referenced this issue Oct 29, 2012
Show the alternate action (if available) when ⌘ is pressed. Fixes #189
@skurfer skurfer merged commit e92006e into quicksilver:master Oct 29, 2012
@lgarron
Copy link
Contributor

@lgarron lgarron commented Nov 30, 2012

I think the following bug breaks past behaviour unnecessarily:

  • Press Cmd-Enter (my combination) to invoke QS with the last item selected.
  • Let go of Space. The alternate action is not selected.
  • Let go of Cmd, press Cmd again. The alternate action is selected now.

(This keeps catching me when I do something with a file that leaves it as the QS selection, then try to reveal it in Finder immediately afterwards. The current behaviour in B71 keeps opening instead of revealing if I don't let go of Cmd before pressing Cmd-Enter.)

For contrast, I have "Select Current Selection in QS Window" set to Ctrl-Cmd-Space, and letting go of Ctrl after space does select the alternate action.

@lgarron
Copy link
Contributor

@lgarron lgarron commented Nov 30, 2012

Secondary consideration: copying an action (sometimes necessary to construct a trigger) using Cmd-C doesn't work anymore if it has an alternate. You can right-click and copy, but this might be confusing to some power users.

(Overall, I really like this feature, just trying to give early feedback. I've tried to check this thread for similar comments, but I didn't find any.)

@petonic
Copy link

@petonic petonic commented Dec 18, 2012

Any way to disable this? My fingers don't seem to work well with it :-). I use CMD-Shift-L to invoke my Shortmarks trigger. If I leave a lazy hand on the CMD key, it invokes the alternate to "Search", which is "Show search results." Maybe I'll get used to it later, but I'd like to disable this.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Dec 18, 2012

Currently there's no way to disable this. I think we may well roll with it
how it is in ß71, and if enough people find they also have lazy hands, we
will add a hidden preference in ;-)

One quick question: Do you need to have Quicksilver open to invoke your
short marks trigger? It should be possible to press ⌘ ⇧L without having to
open QS

On 18 December 2012 16:12, petonic notifications@github.com wrote:

Any way to disable this? My fingers don't seem to work well with it :-). I
use CMD-Shift-L to invoke my Shortmarks trigger. If I leave a lazy hand on
the CMD key, it invokes the alternate to "Search", which is "Show search
results." Maybe I'll get used to it later, but I'd like to disable this.


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

@petonic
Copy link

@petonic petonic commented Dec 18, 2012

Hi Patrick -- Thanks for responding. No, I don't need QS to be open. I can just invoke it from a neutral state (assuming QS is running, of course).

This relatively new behavior surprised me. I must be lazier with my typing than I had thought. It wasn't until I read the release notes of this version of QS that I finally figured out what was happening. When I hold CMD down, it'll respond with a QS Dialog and . If I tab over into and press the down arrow for , it'll then work. Kind of a pain in the butt, but at least all of the effort didn't go to the ether.

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

5 participants