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

Jump to bottom

Event Trigger support #1784

Merged
merged 5 commits into from Jun 9, 2014
Merged

Event Trigger support #1784

merged 5 commits into from Jun 9, 2014

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Feb 23, 2014

setEnabled: calls [[QSTriggerCenter sharedInstance] triggerChanged:self], which calls setEnabledDoNotNotify:, which contains the line I removed from here.

While working on Event Triggers some more, I saw some things being overreleased because disableTrigger: was getting called twice.

skurfer added 4 commits Feb 23, 2014
@skurfer
just set enabled for the trigger, but don't signal the trigger manger
75c13a8 
prevents multiple calls to enableTrigger:/disableTrigger:
@skurfer
activated should depend on the result of enable/disable
359d1f0 
enableTrigger: and disableTrigger: return a BOOL. We should take the
value of activate from that, instead of assuming the method always
succeeds.
@skurfer
more explicit behavior when reactivating triggers
8d081f7 
Calling setEnabled: has side-effects:
  * sends a notification
  * causes Triggers.plist to be written to disk
  * calls enableTrigger: or disableTrigger:

Only the last of these side-effects is desirable, and only when the
trigger is enabled. (Calling disableTrigger: at launch, when it’s sure
to be disabled already, can be bad.)

So instead of relying on side-effects here, explicitly call the desired
behavior.
@skurfer
prevent duplicate calls to triggerChanged: when enabling/disabling tr…
a988d1f 
…iggers
@skurfer
Copy link
Member Author

skurfer commented Feb 24, 2014

There are apparently all kinds of side-effects that will cause triggers to get disabled multiple times.

I’ve fixed most of them, but there are two more things I’m looking at in -[QSTriggersPrefPane observeValueForKeyPath:ofObject:change:context:] before this is merged.

  1. It’s only observing scope settings, which aren’t changing, so I’m not sure why it’s getting called at all.
  2. It uses [self selectedTrigger], which refers to the first trigger when nothing is selected. (Clicking the “enabled” check-box doesn’t actually select the trigger, so it often refers to the wrong one.)

@skurfer
Copy link
Member Author

skurfer commented Feb 25, 2014

I’ve added the QSEventTriggerProvider protocol here as discussed.

I’m giving up on the other issues for now. We’ve got KVO stuff causing triggers to be “updated” all over the place. Instead, I just made the event trigger providers handle multiple disable messages gracefully.

@skurfer skurfer mentioned this pull request Feb 25, 2014
Merged
@pjrobertson
Copy link
Member

pjrobertson commented Mar 11, 2014

Mostly seems fine. I'm sure there's more to be done, and yes, the 'saving everytime anything is changed' bug is annoying :D

@skurfer
Copy link
Member Author

skurfer commented Apr 25, 2014

Mostly seems fine.

So… merge? 😉 This isn’t required for the Event Triggers updates to work. It just makes things more efficient for triggers across the board.

@skurfer
Copy link
Member Author

skurfer commented Apr 25, 2014

Scratch that. I think it is required because we moved the QSEventTriggerProvider protocol here.

@skurfer skurfer added this to the 1.2.0 milestone Apr 25, 2014
@pjrobertson
Copy link
Member

pjrobertson commented May 1, 2014

OK, so is this really required? I thought you said RE the event trigger plugin that you'd just manually copy/paste the protocol into the plugin before building so that it worked with QS 1.1?

@skurfer skurfer removed this from the 1.2.0 milestone May 1, 2014
@skurfer
Copy link
Member Author

skurfer commented May 1, 2014

OK, so is this really required? I thought you said RE the event trigger plugin that you'd just manually copy/paste the protocol into the plugin before building so that it worked with QS 1.1?

Oh, true. Been a while, sorry. 😃 Yeah, this can wait.

@skurfer skurfer mentioned this pull request Jun 4, 2014
Closed
pjrobertson added a commit that referenced this issue Jun 9, 2014
@pjrobertson
Merge pull request #1784 from quicksilver/disableTrigger
0e8d30d 
Event Trigger support
@pjrobertson pjrobertson merged commit 0e8d30d into master Jun 9, 2014
1 check passed
@pjrobertson pjrobertson deleted the disableTrigger branch Jun 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@skurfer @pjrobertson