Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

"Add Trigger" has no Effect #515

Closed
Orion751 opened this Issue · 13 comments

4 participants

@Orion751

(QS B61, Snow Leopard 10.6.8)

Using the "Add Trigger" action after pressing control + enter during a Quicksilver invocation opens the third pane in text mode.

Regardless of whether the third pane is left empty or contains text when then enter key is pressed, nothing appears to change Quicksilver's Triggers list.

@skurfer
Owner

Interesting. I didn’t even know about this one. When I run it, I get a new empty trigger (which is better but still broken). I’m using the latest master, which includes a ton of trigger fixes, so maybe that’s why. I’ll look into this.

@skurfer skurfer was assigned
@pjrobertson
Owner

Confirmed. Like Rob, I didn't even know this existed! Rob's on the job

(ß62 me thinks Rob...?)

@skurfer
Owner

So I won’t forget what I’ve found so far:

The debugger seems to think the direct object passed to addTrigger:withInfo: in QSCommand.m is empty. I’ve traced it back to [dSelector objectValue] in QSInterfaceController.m#L140 (which is also empty according to the debugger). This is hard to believe though, because then nothing would work. If you run the command instead of encapsulating, it hands things off to executeCommandThreaded, which also calls [dSelector objectValue]. There, the dObject says “summary unavailable”, which is slightly different from the empty value elsewhere, but in any case, we know the object is OK there.

@Orion751

Wow. I've kept checking for it on each update thinking it must've been a de-prioritized feature. Glad I finally spoke up!

As far as creating empty Triggers go, I've also had that behavior throughout my recollection. As a matter of fact, a whole bunch of them appeared this morning.

@skurfer
Owner

More notes…

It looks like the problem is in the call to performSelector on QSTriggerCenter.m#L181. The writeTriggersNow method never runs.

@pjrobertson
Owner

Can anybody else reproduce a crash if you

a) Launch QS
b) Straight off (before the prefs have ever been opened) run "command" > Add Trigger.

There's a crash when loading the prefs - the loading of the webkitview is not being done on the main thread.

@skurfer
Owner

When I was in the debugger, I saw some messages on the console related to WebKit and it would stop execution, but it didn’t appear to have crashed. I couldn’t nail down what made it happen, but I think you’re right that it’s when you don’t open the prefs at all. Running a “Release” build, I don’t get a crash. I just get the empty trigger.

@pjrobertson
Owner

This has me completely stumped as well.

In the call to [[QSTriggerCenter sharedInstance] addTrigger:trigger]; QSCommand.m:139 the trigger gets added to the triggers dictionary, but is null. :/

@tiennou
Owner

I know the indirectObject part of this is my "fault" : I wanted to be able to specify a Trigger type before showing the Trigger pref pane. But I found it klunky because I don't think that's a nice enough way of doing it. You can freely drop the indirect stuff (unless you want to actually implement it, but I'm not sure that's the way to go...) if that's cleans your mind, but here are some "implementation thoughts" on creating QS through its main interface.

The best way to handle this IMHO is to add an action for every trigger type (new actions in trigger plugins that means), which would take an encapsulated command as its direct object and a trigger-specific thing for its indirect object. Example:
Direct object : "pizza" > Display In Large Type
Action : "Create hotkey trigger"
Indirect Object : text placeholder for the shortcut.

But then you would have to change the text entry stuff to handle modifiers (or try to provide reasonable defaults, but we're messing with globally-defined system hotkeys...). A way to alleviate that would be to drop the indirect object and bring up the Trigger pref pane with the newly created trigger selected.

Then implement "Create mouse trigger" and have its indirect object resolve to things like Top Border, Top-Left Corner, ...

My 2 cents ;-)

@skurfer
Owner

I’ve followed it further down that that. addTrigger calls writeTriggers, which calls performSelector:@selector(writeTriggersNow). Everything is fine up to that point, but then inside performSelector, it looks like it should call itself, but it never does from what I can tell. (It looks like this was designed to avoid writing triggers over and over if several write requests come in rapid succession.)

I wonder if performSelector should be calling super performSelector instead of self performSelector, but the writeTriggers method obviously works in other contexts, so who knows.

@skurfer
Owner

inside performSelector, it looks like it should call itself, but it never does from what I can tell

To clarify, the line is executed, but when I try to “step into”, it just goes to the next line as though I did “step over”.

@skurfer
Owner

You can freely drop the indirect stuff (unless you want to actually implement it, but I'm not sure that's the way to go...)

Yeah, I was going to look into that more after we got it working. My 2 cents:

  • We should’t try to capture the keystroke, mouse gesture, etc. We should just show the trigger after its created and let people use the existing interface to define that.
  • The indirect object (if we keep it) should be a predefined list of things to pick from rather than a free-form text entry panel.

I was thinking we keep the single action as is and then the indirect object could be used to select between hotkey, mouse, etc. But you never know how well it’ll work until you try, so I’m not committed to any one idea.

@Orion751

All else being equal, I'm sure many would agree that the option involving the least keystrokes would be preferred. This feature is essentially a shortcut, after all...

@tiennou tiennou referenced this issue from a commit in tiennou/Quicksilver
@tiennou tiennou Provoke a KVO notification when a trigger gets updated.
fixes #515
a76a709
@tiennou tiennou referenced this issue
Closed

Add trigger #563

@tiennou tiennou was assigned
@skurfer skurfer closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.