Action config #2423
Action config #2423
Conversation
| * return nil; | ||
| * }; | ||
| */ | ||
| - (BOOL)setActionUisngBlock:(QSObject *(^)(id, QSObject *))actionBlock selectorName:(NSString *)selName; |
There was a problem hiding this comment.
Any specific reason you're using NSString * instead of SEL ? I'd prefer the latter (as well as a selector: parameter).
There was a problem hiding this comment.
Yes. Because strings are “normal” and easy to create, while selectors are “weird” and difficult to create. “Difficult” meaning I had to look it up. I didn’t say it was a good reason.
Are you saying it should keep the string but also require you to create and pass in a SEL? Or drop the string?
There was a problem hiding this comment.
I'd use -setActionUsingBlock:(…)block selector:(SEL)aSelector.
Take for example -mySelector:. If you mistype "mySelectr:" you will get a "missing method" at runtime. If you use @selector(mySelectr:), you willl/should get a compiler warning that it doesn't know about that particular selector.
There was a problem hiding this comment.
OK, but the only time you ever type the name is during creation. It can be misspelled and still work. Also, I don’t think the compiler is aware of these methods even if you spell them correctly, so you’ll always get a warning. (I haven’t tested that today, but I think I was seeing such warnings during early attempts.)
| IMP actionFunction = imp_implementationWithBlock(actionBlock); | ||
| SEL actionSelector = NSSelectorFromString(selName); | ||
| BOOL actionDefined = class_addMethod([[self provider] class], actionSelector, actionFunction, "@@:@"); | ||
| [self setAction:actionSelector]; |
There was a problem hiding this comment.
Maybe logging the failure and returning NO if the method couldn't be added would be more fool-proof
There was a problem hiding this comment.
I’ve made this a little better.
| - (BOOL)setActionUisngBlock:(QSObject *(^)(id, QSObject *))actionBlock selectorName:(NSString *)selName | ||
| { | ||
| if (![self provider]) { | ||
| NSLog(@"define provider before setting a block as the action"); |
There was a problem hiding this comment.
I'd be tempted to NSAssert that, but maybe I'm being too paranoid…
There was a problem hiding this comment.
It’s mostly there so a developer will catch it during the plug-in’s creation. Wouldn’t an assert crash QS if the mythical developer doesn’t fix things, potentially punishing end users?
There was a problem hiding this comment.
Well, if the mythical lets a crashing assert come up in the wild, isn't the mythical to blame for its sloppiness
NSLogs can and will be ignored by the developer, and by the user. Exceptions will not. I'd say document that "It is a programming error to use this method if the action doesn't have a provider".
There was a problem hiding this comment.
Well, if the mythical lets a crashing assert come up in the wild, isn't the mythical to blame for its sloppiness
😉 ?
Of course, but it’s the user that’ll feel the pain. Not the developer. The creators of all these plug-ins aren’t exactly jumping on issues they day they get reported. Or even the year they get reported.
NSLogs can and will be ignored by the developer, and by the user.
I don’t expect the log message to be what gets their attention. But when they realize their new action doesn’t exist, the message will explain why.
If we’re assuming they’re going to fix the issue during development one way or another, I suppose an exception gets the job done. I’ll change it.
Services Menu Plugin. I’ll address the other stuff later. Thanks! |
and skip trying to set a failed action
|
And it did occur to me after I added all those setters that I could have just used |
|
I think I addressed everything you asked about @tiennou. Anything else? I’ll wait 72 hours for objections, then merge. |
skurfer
dismissed
tiennou’s stale review
There’s a working precedent for creating actions.
Allow more things to be defined for actions. Basically because I needed some of this for the new Audio plug-in.
I could have just created 12 actions by hand in less time than it took to figure out the block stuff, but I think it’s worth it. For an actual use-case, see https://github.com/quicksilver/Audio-qsplugin/blob/master/Audio/AudioAction.m#L31
Another place it can be used is Remote Hosts where many of the actions are just opening a URL with a different scheme.