I made a commit (r4485) that has far-reaching implications that shouldn't really affect anyone, but I wanted people to know of and take advantage of the change, if they can.
If you've written a plugin, you're probably aware of writing a set of these:
public function filter_plugin_config( $actions, $plugin_id ) { if ( $plugin_id == $this->plugin_id() ){ $actions[] = _t( 'Configure' ); } return $actions;
}
public function action_plugin_ui( $plugin_id, $action ) { if ( $plugin_id == $this->plugin_id() ){ switch ( $action ){ case 'Configure' : $ui = new FormUI(); $ui->out(); break; } }
}
In each of these, the plugin needs to check if the UI being configured or displayed is the plugin itself. This allowed plugins to add UI to other plugins, but is very rarely used.
I made a commit tonight that alters this behavior.
By default, both of these methods will execute only for the plugin that they affect. Said a different way, the if() statements in the two functions above will ALWAYS evalutate to true because Habari will not call those methods on the plugin unless that is the case.
If you still want to add UI for other plugins, you may do so at the hooks filter_plugin_config_any and action_plugin_ui_any. Those two are called for every plugin.
The parameters for both of these have not changed -- the $plugin_id value is still passed into the method. The net result is that most existing plugins should do exactly the same thing they've been doing all along.
An additional change is one that is related to something that Habari has done all along that people should have been taking more advantage of than they do.
When you return the array from filter_plugin_config(), the keys of the array are used as the $action value when calling action_plugin_ui(). If you do not provide a key, then the value is used as the $action. In either case, the value is used in the display of the plugin's configuration button.
If you are properly translating your plugins, you should have been doing something like this:
public function filter_plugin_config( $actions, $plugin_id ) { if ( $plugin_id == $this->plugin_id() ){ $actions['configure'] = _t( 'Configure' ); } return $actions;
}
This causes the $action value passed to action_plugin_ui to be 'configure', regardless of what 'Configure' translates to. I've seen code inside action_plugin_ui that puts a _t() in the switch. Don't do this! Use the key instead.
Along those lines, it is now possible to name functions using the key of the action array that respond to those calls in addition to action_plugin_ui. For example, the following function will do the same thing as the action_plugin_ui() shown above, but only for the 'configure' option:
public function action_plugin_ui_configure( $plugin_id, $action ) { if ( $plugin_id == $this->plugin_id() ){ $ui = new FormUI(); $ui->out(); break; }
}
This is much more elegant than writing a switch() with cases for each menu option, although that method still works if there's some specific need to do it (like in the case where you dynamically generate the keys, for example, the podcast plugin).
All that said, using this recent commit, you can reduce the plugin configuration UI code I wrote at the beginning of this message to just this:
public function filter_plugin_config( $actions, $plugin_id ) { $actions['configure'] = _t( 'Configure' ); return $actions;
}
public function action_plugin_ui_configure() { $ui = new FormUI(); $ui->out();
}
This reads a lot better, is less effort to write, and generally seems to be more what people expect when writing the code.
Obviously, the configure() method shortcut still works, and maps only to the current plugin (as it should, and has been).
There should be no impact on existing plugins, at least as far as I am aware, because there are currently no plugins that affect the menus or UI's of other plugins in this way.
If you have any questions, problems, or concerns, please voice them in this thread.
Hi Owen a very good improvement, but still does not make sense why on one case you are using 'any' and in the other you are using a more specific approach.
I'd be in favour or having two different apis (and this should also leave things working without breaking anything) in the form:
// The usual one. public function action_plugin_ui( $plugin_id, $action ) {
}
And I'd have included the
public function action_plugin_ID_ui($action) {
}
So modules will only answer to this plugin ID ui actions when they know the ID instead of having to check if they are their own action handlers, leaving the action_plugin_ui for a more generic approach in case your code includes additional actions for other plugins IDs. With time all extras would move their action_plugin_ui to action_plugin_id_ui(), but all current code would work without problems. Right now I don't know any extra plugin that includes additional menu entries for other plugin's configuration, but probably there could be some of them.
Anyway, code looks much more readable, however I'm unsure about the implications of translating dynamically name actions..
On Mon, Nov 1, 2010 at 3:42 AM, Owen Winkler <epit...@gmail.com> wrote: > I made a commit (r4485) that has far-reaching implications that shouldn't > really affect anyone, but I wanted people to know of and take advantage of > the change, if they can.
> If you've written a plugin, you're probably aware of writing a set of > these:
> public function filter_plugin_config( $actions, $plugin_id ) > { > if ( $plugin_id == $this->plugin_id() ){ > $actions[] = _t( 'Configure' ); > } > return $actions; > }
> public function action_plugin_ui( $plugin_id, $action ) > { > if ( $plugin_id == $this->plugin_id() ){ > switch ( $action ){ > case 'Configure' : > $ui = new FormUI(); > $ui->out(); > break; > } > } > }
> In each of these, the plugin needs to check if the UI being configured or > displayed is the plugin itself. This allowed plugins to add UI to other > plugins, but is very rarely used.
> I made a commit tonight that alters this behavior.
> By default, both of these methods will execute only for the plugin that > they affect. Said a different way, the if() statements in the two functions > above will ALWAYS evalutate to true because Habari will not call those > methods on the plugin unless that is the case.
> If you still want to add UI for other plugins, you may do so at the hooks > filter_plugin_config_any and action_plugin_ui_any. Those two are called for > every plugin.
> The parameters for both of these have not changed -- the $plugin_id value > is still passed into the method. The net result is that most existing > plugins should do exactly the same thing they've been doing all along.
> An additional change is one that is related to something that Habari has > done all along that people should have been taking more advantage of than > they do.
> When you return the array from filter_plugin_config(), the keys of the > array are used as the $action value when calling action_plugin_ui(). If you > do not provide a key, then the value is used as the $action. In either > case, the value is used in the display of the plugin's configuration button.
> If you are properly translating your plugins, you should have been doing > something like this:
> public function filter_plugin_config( $actions, $plugin_id ) > { > if ( $plugin_id == $this->plugin_id() ){ > $actions['configure'] = _t( 'Configure' ); > } > return $actions; > }
> This causes the $action value passed to action_plugin_ui to be 'configure', > regardless of what 'Configure' translates to. I've seen code inside > action_plugin_ui that puts a _t() in the switch. Don't do this! Use the > key instead.
> Along those lines, it is now possible to name functions using the key of > the action array that respond to those calls in addition to > action_plugin_ui. For example, the following function will do the same > thing as the action_plugin_ui() shown above, but only for the 'configure' > option:
> public function action_plugin_ui_configure( $plugin_id, $action ) > { > if ( $plugin_id == $this->plugin_id() ){ > $ui = new FormUI(); > $ui->out(); > break; > } > }
> This is much more elegant than writing a switch() with cases for each menu > option, although that method still works if there's some specific need to do > it (like in the case where you dynamically generate the keys, for example, > the podcast plugin).
> All that said, using this recent commit, you can reduce the plugin > configuration UI code I wrote at the beginning of this message to just this:
> public function filter_plugin_config( $actions, $plugin_id ) > { > $actions['configure'] = _t( 'Configure' ); > return $actions; > }
> public function action_plugin_ui_configure() > { > $ui = new FormUI(); > $ui->out(); > }
> This reads a lot better, is less effort to write, and generally seems to be > more what people expect when writing the code.
> Obviously, the configure() method shortcut still works, and maps only to > the current plugin (as it should, and has been).
> There should be no impact on existing plugins, at least as far as I am > aware, because there are currently no plugins that affect the menus or UI's > of other plugins in this way.
> If you have any questions, problems, or concerns, please voice them in this > thread.
> Thanks, > Owen
> -- > To post to this group, send email to habari-dev@googlegroups.com > To unsubscribe from this group, send email to > habari-dev-unsubscribe@googlegroups.com > For more options, visit this group at > http://groups.google.com/group/habari-dev
On Mon, Nov 1, 2010 at 8:36 AM, Iñaki Lopez <inaki.lo...@gmail.com> wrote: > Hi Owen a very good improvement, but still does not make sense why on one > case you are using 'any' and in the other you are using a more specific > approach.
> I'd be in favour or having two different apis (and this should also leave > things working without breaking anything) in the form:
> // The usual one. > public function action_plugin_ui( $plugin_id, $action ) {
> }
> And I'd have included the
> public function action_plugin_ID_ui($action) { > }
> So modules will only answer to this plugin ID ui actions when they know the > ID instead of having to check if they are their own action handlers, leaving > the action_plugin_ui for a more generic approach in case your code includes > additional actions for other plugins IDs. With time all extras would move > their action_plugin_ui to action_plugin_id_ui(), but all current code would > work without problems. Right now I don't know any extra plugin that includes > additional menu entries for other plugin's configuration, but probably there > could be some of them.
> Anyway, code looks much more readable, however I'm unsure about the > implications of translating dynamically name actions..
> Cheers!
> On Mon, Nov 1, 2010 at 3:42 AM, Owen Winkler <epit...@gmail.com> wrote:
>> I made a commit (r4485) that has far-reaching implications that shouldn't >> really affect anyone, but I wanted people to know of and take advantage of >> the change, if they can.
>> If you've written a plugin, you're probably aware of writing a set of >> these:
>> public function filter_plugin_config( $actions, $plugin_id ) >> { >> if ( $plugin_id == $this->plugin_id() ){ >> $actions[] = _t( 'Configure' ); >> } >> return $actions; >> }
>> public function action_plugin_ui( $plugin_id, $action ) >> { >> if ( $plugin_id == $this->plugin_id() ){ >> switch ( $action ){ >> case 'Configure' : >> $ui = new FormUI(); >> $ui->out(); >> break; >> } >> } >> }
>> In each of these, the plugin needs to check if the UI being configured or >> displayed is the plugin itself. This allowed plugins to add UI to other >> plugins, but is very rarely used.
>> I made a commit tonight that alters this behavior.
>> By default, both of these methods will execute only for the plugin that >> they affect. Said a different way, the if() statements in the two functions >> above will ALWAYS evalutate to true because Habari will not call those >> methods on the plugin unless that is the case.
>> If you still want to add UI for other plugins, you may do so at the hooks >> filter_plugin_config_any and action_plugin_ui_any. Those two are called for >> every plugin.
>> The parameters for both of these have not changed -- the $plugin_id value >> is still passed into the method. The net result is that most existing >> plugins should do exactly the same thing they've been doing all along.
>> An additional change is one that is related to something that Habari has >> done all along that people should have been taking more advantage of than >> they do.
>> When you return the array from filter_plugin_config(), the keys of the >> array are used as the $action value when calling action_plugin_ui(). If you >> do not provide a key, then the value is used as the $action. In either >> case, the value is used in the display of the plugin's configuration button.
>> If you are properly translating your plugins, you should have been doing >> something like this:
>> public function filter_plugin_config( $actions, $plugin_id ) >> { >> if ( $plugin_id == $this->plugin_id() ){ >> $actions['configure'] = _t( 'Configure' ); >> } >> return $actions; >> }
>> This causes the $action value passed to action_plugin_ui to be >> 'configure', regardless of what 'Configure' translates to. I've seen code >> inside action_plugin_ui that puts a _t() in the switch. Don't do this! Use >> the key instead.
>> Along those lines, it is now possible to name functions using the key of >> the action array that respond to those calls in addition to >> action_plugin_ui. For example, the following function will do the same >> thing as the action_plugin_ui() shown above, but only for the 'configure' >> option:
>> public function action_plugin_ui_configure( $plugin_id, $action ) >> { >> if ( $plugin_id == $this->plugin_id() ){ >> $ui = new FormUI(); >> $ui->out(); >> break; >> } >> }
>> This is much more elegant than writing a switch() with cases for each menu >> option, although that method still works if there's some specific need to do >> it (like in the case where you dynamically generate the keys, for example, >> the podcast plugin).
>> All that said, using this recent commit, you can reduce the plugin >> configuration UI code I wrote at the beginning of this message to just this:
>> public function filter_plugin_config( $actions, $plugin_id ) >> { >> $actions['configure'] = _t( 'Configure' ); >> return $actions; >> }
>> public function action_plugin_ui_configure() >> { >> $ui = new FormUI(); >> $ui->out(); >> }
>> This reads a lot better, is less effort to write, and generally seems to >> be more what people expect when writing the code.
>> Obviously, the configure() method shortcut still works, and maps only to >> the current plugin (as it should, and has been).
>> There should be no impact on existing plugins, at least as far as I am >> aware, because there are currently no plugins that affect the menus or UI's >> of other plugins in this way.
>> If you have any questions, problems, or concerns, please voice them in >> this thread.
>> Thanks, >> Owen
>> -- >> To post to this group, send email to habari-dev@googlegroups.com >> To unsubscribe from this group, send email to >> habari-dev-unsubscribe@googlegroups.com >> For more options, visit this group at >> http://groups.google.com/group/habari-dev
> Hi Owen a very good improvement, but still does not make sense why on > one case you are using 'any' and in the other you are using a more > specific approach.
> I'd be in favour or having two different apis (and this should also > leave things working without breaking anything) in the form:
> // The usual one. > public function action_plugin_ui( $plugin_id, $action ) {
> }
> And I'd have included the
> public function action_plugin_ID_ui($action) { > }
There's no need to include "ID" in the function name. You've already defined a class for your plugin, and that should be enough to declare that the method belongs only to that plugin.
Also, what would "ID" consist of? Plugin ids are usually generated by Habari and are only used for comparison; There is no guarantee that the plugin id will be any particular string from install to install. Adding some other parameter for the plugin to pull from (like in the info/xml, or some other function) just makes it more complicated than it was already, or at best, makes it no easier than it originally was when you had to compare the plugin ids yourself.
Adding "any" as the hook that responds to any plugin_ui call makes sense because it explicitly states that it will respond to any request.
As I mentioned in my original post, the expected behavior when observing the the function in use is that the non-any hook will respond only for that plugin. New plugin devs are usually surprised to find out this is not the case. With this commit, we're simply enforcing what people already expect to be true, what results by default in the least amount of code, and what makes the non-standard behavior explicit. I think that's pretty good reasoning, altogether.
Hi Owen, and in fact yours is a very good reasoning. I was just pointing that currently, all implementations of action_plugin_ui() in 'extras' are just what now is expected to be on action_plugin_ui_any(). So as long as all extras are implementing this action to change the ui (its own, or other's extras ui), to avoid conflict it makes more sense to me to leave this method as is, and include a 'custom' only self action. I used ID, but I was not thinking in uid at all, it could be the extra name, or whatever you want to.
If you are going to change this behavior (considering a new action '_any' affecting all plugins) then it could be also a good idea to think about the action's name. I don't mean to start an end-less discussion about it, but 'action_plugin_ui' is not exactly what I was expecting the first time I saw this action, so I'm not surprised that you said: "New plugin devs are usually surprised to find out this is not the case", in fact, the first surprise is that plugin_ui is only interacting with current's adminhandler (that seems not to be easy to replace) to handle a part of the admin interface, not all the plugin's user interfaces, so unless you already know Habari's internals, it is very dificult to notive that plugin_ui is only a part of the plugin UI, and only to be used in a specific part of the administration section. Just 2 cents, good critic however, I like Habari.
Thanks Owen, I'm pretty sure that new commit has some performance improvements that you didn't mention and I'd like to bump for others.
On Tue, Nov 2, 2010 at 4:13 AM, Owen Winkler <epit...@gmail.com> wrote: > On 11/1/2010 3:36 AM, Iñaki Lopez wrote:
>> Hi Owen a very good improvement, but still does not make sense why on >> one case you are using 'any' and in the other you are using a more >> specific approach.
>> I'd be in favour or having two different apis (and this should also >> leave things working without breaking anything) in the form:
>> // The usual one. >> public function action_plugin_ui( $plugin_id, $action ) {
>> }
>> And I'd have included the
>> public function action_plugin_ID_ui($action) { >> }
> There's no need to include "ID" in the function name. You've already > defined a class for your plugin, and that should be enough to declare that > the method belongs only to that plugin.
> Also, what would "ID" consist of? Plugin ids are usually generated by > Habari and are only used for comparison; There is no guarantee that the > plugin id will be any particular string from install to install. Adding > some other parameter for the plugin to pull from (like in the info/xml, or > some other function) just makes it more complicated than it was already, or > at best, makes it no easier than it originally was when you had to compare > the plugin ids yourself.
> Adding "any" as the hook that responds to any plugin_ui call makes sense > because it explicitly states that it will respond to any request.
> As I mentioned in my original post, the expected behavior when observing > the the function in use is that the non-any hook will respond only for that > plugin. New plugin devs are usually surprised to find out this is not the > case. With this commit, we're simply enforcing what people already expect > to be true, what results by default in the least amount of code, and what > makes the non-standard behavior explicit. I think that's pretty good > reasoning, altogether.
> Thanks for your feedback.
> Owen
> -- > To post to this group, send email to habari-dev@googlegroups.com > To unsubscribe from this group, send email to > habari-dev-unsubscribe@googlegroups.com > For more options, visit this group at > http://groups.google.com/group/habari-dev
