Feature request: suggestion for cPlugin

Message ID 4304DA44.7030906@gmx.de
State New
Headers

Commit Message

Udo Richter Aug. 18, 2005, 6:58 p.m. UTC
  Klaus Schmidinger wrote:
> Udo Richter wrote:
>> A far better way would be of course if VDR's cPlugin would have such a
>> general purpose interface. My approach would be like this:
>>
>>   virtual bool ExtensionInterface(const char *ExtensionId, void *Data)
>>
>> Plugins return false unless ExtensionId matches a known unique protocol
>> ID. If the plugin supports this protocol, true will be returned in any
>> case. If *Data is not NULL, it points to a protocol-specific buffer. If
>> *Data is NULL, this is just a protocol-supported call.
>>
>> cPluginManager should have methods to call ExtensionInterface for all
>> plugins (broadcast-like), or for all plugins until one plugin returns
>> true (find one that offers the interface).
>>
>>
>> @Klaus: I can implement that if interested.
> 
> Well, since I'm not going to use that interface, anyway, and it shouldn't
> have any impact on plain vanilla VDR, just go ahead and send a patch.
> 
> And don't forget to adjust PLUGINS.html ;-)

Ok, here's a first implementation of what I had in mind. The attached
patch implements the interface for VDR 1.3.20 and newer. PLUGINS.html
and newplugin still need to be updated. ;)

The attached vdr-extintf-0.1.0.tgz implements two sample plugins that do
some communication: Direct communication to a specific plugin, detecting
presence of plugins that offer a service, use a service provided by some
plugin and broadcast a message to all interested plugins.
Install as usual. Load the two plugins with vdr -P extcli -P extsvr.

@Plugin developers:
Suggestions, reviews and comments are welcome!

Cheers,

Udo
  

Comments

Martin Wache Aug. 18, 2005, 8:14 p.m. UTC | #1
Udo Richter schrieb:

> The attached vdr-extintf-0.1.0.tgz implements two sample plugins that do
> some communication: Direct communication to a specific plugin, detecting
> presence of plugins that offer a service, use a service provided by some
> plugin and broadcast a message to all interested plugins.
> Install as usual. Load the two plugins with vdr -P extcli -P extsvr.
> 
> @Plugin developers:
> Suggestions, reviews and comments are welcome!

I like interface for cPlugin::ExtensionInterface() it will be very
usefull for the softplay/softdevice communication!

In my opinion the method CallPluginExtension() is not needed, the same
can easily done by just calling
cPluginManager::GetPlugin("name")->ExtensionInterface().

About the other methods CallFirstExtension() or CallAllExtension(), I'm
not sure. Currently I don't think that I will need them. But who knows?
If they are there, I think it would be nice to be able to know which
plugin actually answered the request, so that following request can be
sent directly to that plugin. That way one could first query for a
specific extension and then send all requests directly.

Bye,
Martin
  
Christian Wieninger Aug. 18, 2005, 9:13 p.m. UTC | #2
Hi,

> > @Plugin developers:
> > Suggestions, reviews and comments are welcome!
>

Great job! Thats exactly how it should be.

> In my opinion the method CallPluginExtension() is not needed, the same
> can easily done by just calling
> cPluginManager::GetPlugin("name")->ExtensionInterface().

ok, but perhaps it looks better ;-) 

> If they are there, I think it would be nice to be able to know which
> plugin actually answered the request, so that following request can be
> sent directly to that plugin. That way one could first query for a
> specific extension and then send all requests directly.

I think so too. Perhaps CallAllExtensions could also return a pointer to the 
plugin like CallFirstExtension does. 

Thanks again. The next release of epgsearch will support this patch, and I 
hope I can soon drop the currently needed #ifdef PATCH_EXTENSIONINTERFACE ;-)

Hope other developers also integrate it.

BR,

Christian
  
Udo Richter Aug. 19, 2005, 12:30 a.m. UTC | #3
Martin Wache wrote:
> In my opinion the method CallPluginExtension() is not needed, the same
> can easily done by just calling
> cPluginManager::GetPlugin("name")->ExtensionInterface().

The only (very minor) difference is that your call uses the first plugin
of that name, while CallPluginExtension() queries all plugins of that
name until one answers.

> I think it would be nice to be able to know which
> plugin actually answered the request, so that following request can be
> sent directly to that plugin.

CallPluginExtension() and CallFirstExtension() actually return a pointer
to the plugin that answered the request, or NULL if none answered.

If you're worried about speed, you could pass through a pointer to a
callback function, so further communication uses the callback and not
the interface.

Christian Wieninger wrote:
> I think so too. Perhaps CallAllExtensions could also return a pointer to the 
> plugin like CallFirstExtension does. 

The point of CallAllExtensions() is that the call goes to all
extensions, not just one. It would need a whole list of plugins to solve
this.

> Thanks again. The next release of epgsearch will support this patch, and I 
> hope I can soon drop the currently needed #ifdef PATCH_EXTENSIONINTERFACE ;-)

I've used #if VDRVERSNUM >= 10330 || defined(PATCH_EXTENSIONINTERFACE)
in the sources by purpose, so the code will be used for VDR 1.3.30 and
any previous version that is patched. Why drop backwards compatibility?

Cheers,

Udo
  
Udo Richter Aug. 19, 2005, 12:42 p.m. UTC | #4
Martin Wache wrote:
> In my opinion the method CallPluginExtension() is not needed, the same
> can easily done by just calling
> cPluginManager::GetPlugin("name")->ExtensionInterface().

Why didn't I see that before? Obviously, this call also misses an
important NULL test, so your way blows up to:

cPlugin *p=cPluginManager::GetPlugin("name");
if (p) p->ExtensionInterface(...);

In the end, these functions are just convenient helper functions. All
these functions including GetPlugin("Name") can be substituted just by
using GetPlugin(int) and a few more lines of code.

Cheers,

Udo
  
Klaus Schmidinger Aug. 19, 2005, 12:43 p.m. UTC | #5
Udo Richter wrote:
> ...
> Ok, here's a first implementation of what I had in mind. The attached
> patch implements the interface for VDR 1.3.20 and newer. PLUGINS.html
> and newplugin still need to be updated. ;)
> 
> The attached vdr-extintf-0.1.0.tgz implements two sample plugins that do
> some communication: Direct communication to a specific plugin, detecting
> presence of plugins that offer a service, use a service provided by some
> plugin and broadcast a message to all interested plugins.
> Install as usual. Load the two plugins with vdr -P extcli -P extsvr.
> 
> @Plugin developers:
> Suggestions, reviews and comments are welcome!
> 
> Cheers,
> 
> Udo
> 
> ...
> +  virtual bool ExtensionInterface(const char *ExtensionId, void *Data = NULL);
> ...
> +  static cPlugin *CallFirstExtension(const char *ExtensionId, void *Data = NULL);
> +  static cPlugin *CallPluginExtension(const char *PluginName, const char *ExtensionId, void *Data = NULL);
> +  static bool CallAllExtensions(const char *ExtensionId, void *Data = NULL);

I'm not really fond of this "extension" thing.
IMHO this is more of an "inter plugin communication" thing than an "extension".
Or, maybe "service" is the right name. After all, that's what it is: a plugin
can offer a service that other plugins can make use of.

So I suggest s/Extension/Service/g, and plain "Service" instead of "ExtensionInterface".

You also might want to give some guidelines as to how to create
those "ServiceIDs" - otherwise most likely every plugin will come
up with a totally different scheme.

Klaus
  
Klaus Schmidinger Aug. 19, 2005, 12:43 p.m. UTC | #6
Udo Richter wrote:
> Martin Wache wrote:
> 
>>In my opinion the method CallPluginExtension() is not needed, the same
>>can easily done by just calling
>>cPluginManager::GetPlugin("name")->ExtensionInterface().
> 
> 
> The only (very minor) difference is that your call uses the first plugin
> of that name, while CallPluginExtension() queries all plugins of that
> name until one answers.

There is only _one_ plugin of any given name.
So I also think that CallPluginExtension() is superfluous.

Any caller who wants to direct a service call to a particular
plugin should first check whether that plugin is there, anyway.
Otherwise it wouldn't know whether the request was denied because
the parameters were wrong, or whether the plugin wasn't there
at all.

   cPlugin *Plugin = cPluginManager::GetPlugin("someplugin");
   if (Plugin)
      Plugin->Service("someserviceid", somedata);

>>I think it would be nice to be able to know which
>>plugin actually answered the request, so that following request can be
>>sent directly to that plugin.
> 
> 
> CallPluginExtension() and CallFirstExtension() actually return a pointer
> to the plugin that answered the request, or NULL if none answered.
> 
> If you're worried about speed, you could pass through a pointer to a
> callback function, so further communication uses the callback and not
> the interface.
> 
> Christian Wieninger wrote:
> 
>>I think so too. Perhaps CallAllExtensions could also return a pointer to the 
>>plugin like CallFirstExtension does. 
> 
> 
> The point of CallAllExtensions() is that the call goes to all
> extensions, not just one. It would need a whole list of plugins to solve
> this.

I also think that returning a bool telling the caller whether _any_
plugin has processed the call should be enough.

>>Thanks again. The next release of epgsearch will support this patch, and I 
>>hope I can soon drop the currently needed #ifdef PATCH_EXTENSIONINTERFACE ;-)
> 
> 
> I've used #if VDRVERSNUM >= 10330 || defined(PATCH_EXTENSIONINTERFACE)
> in the sources by purpose, so the code will be used for VDR 1.3.30 and
> any previous version that is patched. Why drop backwards compatibility?

Please note my suggestion regarding the names (s/Extension/Service/g)
in my reply to Udo.

Klaus
  
Klaus Schmidinger Aug. 19, 2005, 12:47 p.m. UTC | #7
Klaus Schmidinger wrote:
> Udo Richter wrote:
> 
>> ...
>> Ok, here's a first implementation of what I had in mind. The attached
>> patch implements the interface for VDR 1.3.20 and newer. PLUGINS.html
>> and newplugin still need to be updated. ;)
>>
>> The attached vdr-extintf-0.1.0.tgz implements two sample plugins that do
>> some communication: Direct communication to a specific plugin, detecting
>> presence of plugins that offer a service, use a service provided by some
>> plugin and broadcast a message to all interested plugins.
>> Install as usual. Load the two plugins with vdr -P extcli -P extsvr.
>>
>> @Plugin developers:
>> Suggestions, reviews and comments are welcome!
>>
>> Cheers,
>>
>> Udo
>>
>> ...
>> +  virtual bool ExtensionInterface(const char *ExtensionId, void *Data 
>> = NULL);
>> ...
>> +  static cPlugin *CallFirstExtension(const char *ExtensionId, void 
>> *Data = NULL);
>> +  static cPlugin *CallPluginExtension(const char *PluginName, const 
>> char *ExtensionId, void *Data = NULL);
>> +  static bool CallAllExtensions(const char *ExtensionId, void *Data = 
>> NULL);
> 
> 
> I'm not really fond of this "extension" thing.
> IMHO this is more of an "inter plugin communication" thing than an 
> "extension".
> Or, maybe "service" is the right name. After all, that's what it is: a 
> plugin
> can offer a service that other plugins can make use of.
> 
> So I suggest s/Extension/Service/g, and plain "Service" instead of 
> "ExtensionInterface".

Arghh - come to think of it, "service ID" is already used in a different context.

Well, maybe somebody finds a better name.
I'm still not happy with "extension".

Klaus
  
Christian Wieninger Aug. 19, 2005, 1:55 p.m. UTC | #8
Hi,

> You also might want to give some guidelines as to how to create
> those "ServiceIDs" - otherwise most likely every plugin will come
> up with a totally different scheme.

>
> Klaus

I also think this should be done, and atleast a version info has to be
part of the interface to avoid problems with changed protocols in newer
plugins. Udo made it part of the service string itself in his sample 
plugins.
Perhaps it should be a separate value. Don't know ;-)

Since this interface can not only be used to call a service of the server 
plugin,
but also to broadcast a message from the client to possibly many servers,
this should also be considered with respect to the naming problem.

Some suggestions:

- CommunicationInterface
- InterchangeInterface
- ExchangeService

BR,

Christian
  
Udo Richter Aug. 19, 2005, 2:06 p.m. UTC | #9
Klaus Schmidinger wrote:
> There is only _one_ plugin of any given name.
> So I also think that CallPluginExtension() is superfluous.

Wrong. One plugin can be loaded multiple times. ;)
Theoretically, a plugin _may_ behave differently each time it is loaded,
based on an internal counter or on command line parameters.

Of course two plugins cannot share the same name without dirty tricks.

Cheers,

Udo
  
Clemens Kirchgatterer Aug. 19, 2005, 2:28 p.m. UTC | #10
"Christian Wieninger" <cwieninger@gmx.de> wrote:

> - CommunicationInterface
> - InterchangeInterface
> - ExchangeService

bool cPlugin::IPC(const char *MsgId, void *Data)

b.t.w. IPC stands for inter plugin communication :o)
  
Klaus Schmidinger Aug. 20, 2005, 8:58 a.m. UTC | #11
Udo Richter wrote:
> Klaus Schmidinger wrote:
> 
>>There is only _one_ plugin of any given name.
>>So I also think that CallPluginExtension() is superfluous.
> 
> 
> Wrong. One plugin can be loaded multiple times. ;)
> Theoretically, a plugin _may_ behave differently each time it is loaded,
> based on an internal counter or on command line parameters.
> 
> Of course two plugins cannot share the same name without dirty tricks.

Gee, I hate it when that happens ;-)
I really thought I had that covered.

Since cPluginManager::GetPlugin(const char *Name) only makes real
sense if there can't be two plugins with the same name, I guess
loading the same plugin more than once isn't a good idea. It might
also cause irritation with the plugin's setup parameters.

I'd therefore suggest that we simply assume that each plugin is
loaded only once, and if somebody really thinks he/she has to
load a plugin twice, they should know what they are doing...

Klaus
  
Klaus Schmidinger Aug. 20, 2005, 9:17 a.m. UTC | #12
Christian Wieninger wrote:
> Hi,
> 
>> You also might want to give some guidelines as to how to create
>> those "ServiceIDs" - otherwise most likely every plugin will come
>> up with a totally different scheme.
> 
> 
>>
>> Klaus
> 
> 
> I also think this should be done, and atleast a version info has to be
> part of the interface to avoid problems with changed protocols in newer
> plugins. Udo made it part of the service string itself in his sample 
> plugins.
> Perhaps it should be a separate value. Don't know ;-)

Well, I'll leave that to Udo ;-)
My original arguments regarding the use of dedicated functions were
dismissed, saying that this would require including header files for
definitions. Well, if you take a look at Udo's example plugins, there is

struct ReportBoredPlugin {
   cPlugin *BoredPlugin;
};

struct AddService {
   int a,b;
   int sum;
};

in both of them. So there's a code duplication right there, and if the
server decides to change something, the client will have to change it's
code as well. It will be fun to watch this stuff go through protocol
changes... ;-)

> Since this interface can not only be used to call a service of the 
> server plugin,
> but also to broadcast a message from the client to possibly many servers,
> this should also be considered with respect to the naming problem.
> 
> Some suggestions:
> 
> - CommunicationInterface
> - InterchangeInterface
> - ExchangeService

Hmm, still no "oh yes!" feeling here.

Maybe my original "service" suggestion isn't that obsolete, yet.
What drove me off of it was the "ServiceID", which is already used
for something completely different. But the generic word "service"
should still be able to be used.

So how about this:

cPlugin:

   virtual bool Service(const char *Id, void *Data = NULL);

cPluginManager:

   static cPlugin *CallFirstService(const char *Id, void *Data = NULL);
   static cPlugin *CallPluginService(const char *PluginName, const char *Id, void *Data = NULL);
   static bool CallAllServices(const char *Id, void *Data = NULL);

Actually CallPluginService() should be superfluous because the caller could
first get the plugin by name and then call its Service() function (thus allowing
it to actually know whether there is such a plugin at all).
So personally I'd vote for not implementing CallPluginService(), but if you
absolutely insist, I won't argue any more.

Klaus
  
Udo Richter Aug. 20, 2005, 1:45 p.m. UTC | #13
Klaus Schmidinger wrote:
> My original arguments regarding the use of dedicated functions were
> dismissed, saying that this would require including header files for
> definitions.

As far as I can tell, Martin's argument is the k.o. criteria for this,
as access to member functions requires both plugins to share a linker
symbol context. Sharing just the headers imho only gives access to
member variables and virtual methods derived from cPlugin. One could use
a function pointer, but thats not really elegant.

> Well, if you take a look at Udo's example plugins, there is
> struct ReportBoredPlugin 
> struct AddService 
> in both of them. So there's a code duplication right there, and if the
> server decides to change something, the client will have to change it's
> code as well. It will be fun to watch this stuff go through protocol
> changes... ;-)

A dedicated struct is a lot safer than the whole class, as it wont
change that frequently. And the documentation will clearly warn, that
any change to the struct requires changing the ID string too.

> Hmm, still no "oh yes!" feeling here.
> 
> Maybe my original "service" suggestion isn't that obsolete, yet.

Ok, I'll come up with another idea: Why not call it a message interface?

cPluginManager::SendMessage(...)
cPluginManager::BroadcastMessage(...)
cPlugin::ProcessMessage(...) or cPlugin::Message(...)

> So personally I'd vote for not implementing CallPluginService(), but if you
> absolutely insist, I won't argue any more.

Since most people think it is not needed, I agree to drop it. I've added
it only for completeness anyway.

Cheers,

Udo
  
Klaus Schmidinger Aug. 20, 2005, 1:56 p.m. UTC | #14
Udo Richter wrote:
> Klaus Schmidinger wrote:
> 
>>My original arguments regarding the use of dedicated functions were
>>dismissed, saying that this would require including header files for
>>definitions.
> 
> 
> As far as I can tell, Martin's argument is the k.o. criteria for this,
> as access to member functions requires both plugins to share a linker
> symbol context. Sharing just the headers imho only gives access to
> member variables and virtual methods derived from cPlugin. One could use
> a function pointer, but thats not really elegant.
> 
> 
>>Well, if you take a look at Udo's example plugins, there is
>>struct ReportBoredPlugin 
>>struct AddService 
>>in both of them. So there's a code duplication right there, and if the
>>server decides to change something, the client will have to change it's
>>code as well. It will be fun to watch this stuff go through protocol
>>changes... ;-)
> 
> 
> A dedicated struct is a lot safer than the whole class, as it wont
> change that frequently. And the documentation will clearly warn, that
> any change to the struct requires changing the ID string too.

Ok, let's end this - there shall be those functions you suggested.

>>Hmm, still no "oh yes!" feeling here.
>>
>>Maybe my original "service" suggestion isn't that obsolete, yet.
> 
> 
> Ok, I'll come up with another idea: Why not call it a message interface?
> 
> cPluginManager::SendMessage(...)
> cPluginManager::BroadcastMessage(...)
> cPlugin::ProcessMessage(...) or cPlugin::Message(...)

Well, "Message" IMHO implies that this is sort of a one way thing,
while "Service" can go either way. In your example you have a plugin that
adds numbers. Now if a client "sends a message" to that server, saying
"add 2 and 3", the server might think "well, nice message". OTOH if the
client "requests the service" of "add 2 and 3", this (at least to me)
implies much more that there is a result to be expected.

If I'm not mistaken the whole point of this interface is to allow one
plugin to make use of a service provided by another plugin. Therefore
I'd still consider "service" the better choice.

Klaus
  
Clemens Kirchgatterer Aug. 20, 2005, 2:27 p.m. UTC | #15
Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:

> > Ok, I'll come up with another idea: Why not call it a message
> > interface?
> > 
> > cPluginManager::SendMessage(...)
> > cPluginManager::BroadcastMessage(...)
> > cPlugin::ProcessMessage(...) or cPlugin::Message(...)
> 
> Well, "Message" IMHO implies that this is sort of a one way thing,
> while "Service" can go either way. In your example you have a plugin
> that adds numbers. Now if a client "sends a message" to that server,
> saying"add 2 and 3", the server might think "well, nice message". OTOH
> if the client "requests the service" of "add 2 and 3", this (at least
> to me) implies much more that there is a result to be expected.

i once designed a RPC library that supported both semantics you describe
here. there were RPC::Functions and RPC::Messages. the difference was
that messages were simple one way atoms, though could be sent ither way.
functions on the other hand, were strictly syncronous, and the caller
was blocked, until the "service provider" sent back the return argument.

in the vdr context, this would be even simpler, as there is no network
magic needed to make things work reliably. for example (keeping udos
syntax):

cPluginManager::SendMessage(PluginID dest, MsgID msg, void *data)
cPluginManager::BroadcastMessage(MsgID msg, void *data)

cPluginManager::CallFunction(void *ret, PluginID dest, FnID id, void
*data)

i think other rpc-implementations do things similar, like D-BUS, DCOP.

so despite the fact that somebody might confuse cPlugin::Messages with
OSD::Messages, i think the term "Messages" fit well into this context.

clemens
  
Klaus Schmidinger Aug. 20, 2005, 2:40 p.m. UTC | #16
Clemens Kirchgatterer wrote:
> Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:
> 
> 
>>>Ok, I'll come up with another idea: Why not call it a message
>>>interface?
>>>
>>>cPluginManager::SendMessage(...)
>>>cPluginManager::BroadcastMessage(...)
>>>cPlugin::ProcessMessage(...) or cPlugin::Message(...)
>>
>>Well, "Message" IMHO implies that this is sort of a one way thing,
>>while "Service" can go either way. In your example you have a plugin
>>that adds numbers. Now if a client "sends a message" to that server,
>>saying"add 2 and 3", the server might think "well, nice message". OTOH
>>if the client "requests the service" of "add 2 and 3", this (at least
>>to me) implies much more that there is a result to be expected.
> 
> 
> i once designed a RPC library that supported both semantics you describe
> here. there were RPC::Functions and RPC::Messages. the difference was
> that messages were simple one way atoms, though could be sent ither way.
> functions on the other hand, were strictly syncronous, and the caller
> was blocked, until the "service provider" sent back the return argument.
> 
> in the vdr context, this would be even simpler, as there is no network
> magic needed to make things work reliably. for example (keeping udos
> syntax):
> 
> cPluginManager::SendMessage(PluginID dest, MsgID msg, void *data)
> cPluginManager::BroadcastMessage(MsgID msg, void *data)
> 
> cPluginManager::CallFunction(void *ret, PluginID dest, FnID id, void
> *data)
> 
> i think other rpc-implementations do things similar, like D-BUS, DCOP.
> 
> so despite the fact that somebody might confuse cPlugin::Messages with
> OSD::Messages, i think the term "Messages" fit well into this context.

Well, it's the same as always - if you start discussing such things as
naming stuff, things get tedious... ;-)

I'll give up - name it whatever you want and make it whatever you like.
I won't be needing nor using this stuff, anyway, so what do I care.

Klaus
  
Udo Richter Aug. 20, 2005, 5:34 p.m. UTC | #17
Hi.

Ok, I'm now aiming for the service variant:

bool cPlugin::Service(const char *Id, void *Data = NULL)
static cPlugin *cPluginManager::CallFirstService(const char *Id, void
*Data = NULL);
static bool cPluginManager::CallAllServices(const char *Id, void *Data =
NULL);

(I thought about CallFirstService being stupid, but could not find
anything better that makes clear, that just one plugin will handle this)


New patch including docs etc. later this day.


There's a minor taste issue that I want to bring up before its too late.
The current interface suggests (but not enforces) that
Service("someid",NULL) asks whether this protocol is supported or not,
and doesn't cause any action. If a plugin just wants to signal something
and has no additional data, enforcing this rule would require some dummy
pointer to be sent. However, the plugin could just take action even with
Data=NULL, violating the rules.

Variants for this:
1. Keep it as-is
2. Drop the supported check from docs - each plugin can offer a check if
it likes
3. Introduce a constant handle, so Service("someid",QUERY_SERVICE) is
the supported check

Cheers,

Udo
  
Christian Wieninger Aug. 20, 2005, 6:28 p.m. UTC | #18
Hi,

> Variants for this:
> 1. Keep it as-is
> 2. Drop the supported check from docs - each plugin can offer a check if
> it likes
> 3. Introduce a constant handle, so Service("someid",QUERY_SERVICE) is
> the supported check
> 
> Cheers,

IMHO 3. is the best solution.

BR,

Christian
  
Klaus Schmidinger Aug. 20, 2005, 7:12 p.m. UTC | #19
Udo Richter wrote:
> ...
> There's a minor taste issue that I want to bring up before its too late.
> The current interface suggests (but not enforces) that
> Service("someid",NULL) asks whether this protocol is supported or not,
> and doesn't cause any action. If a plugin just wants to signal something
> and has no additional data, enforcing this rule would require some dummy
> pointer to be sent. However, the plugin could just take action even with
> Data=NULL, violating the rules.
> 
> Variants for this:
> 1. Keep it as-is
> 2. Drop the supported check from docs - each plugin can offer a check if
> it likes
> 3. Introduce a constant handle, so Service("someid",QUERY_SERVICE) is
> the supported check

4. Service("?someid"), maybe...

(Not "someid?", which would be the "natural" way to form a question, since
"?someid" makes it simple to check for the '?').

Klaus
  
Udo Richter Aug. 21, 2005, 1:15 p.m. UTC | #20
Klaus Schmidinger wrote:
> Udo Richter wrote:
>> Variants for this:
>> 1. Keep it as-is
>> 2. Drop the supported check from docs - each plugin can offer a check if
>> it likes
>> 3. Introduce a constant handle, so Service("someid",QUERY_SERVICE) is
>> the supported check
> 
> 4. Service("?someid"), maybe...

cwieninger@gmx.de wrote:
> IMHO 3. is the best solution.


Since I don't want to enforce extra supported checks, I tend to a
variation of 2: Suggest to use NULL as install check without action.
Enforcing an install check in docs and not using it is inconsistent.

----8<----
<tt>Data</tt> points to a custom data structure. For each id string
there should be a specification that describes the format of the data
structure, and any change to the format should  be reflected by a change
of the id string.
<p>
The function shall return true for any service ID string it handles, and
false otherwise. The plugins have to agreee in which situations the
service may be called, for example whether the service may be called
from every thread, or just from the main thread. Plugins can implement a
'service supported' check by returning <tt>true</tt> without performing
any actions when called with <tt>Data=NULL</tt>.
----8<----

Cheers,

Udo
  
Klaus Schmidinger Aug. 21, 2005, 1:32 p.m. UTC | #21
Udo Richter wrote:
> Klaus Schmidinger wrote:
> 
>>Udo Richter wrote:
>>
>>>Variants for this:
>>>1. Keep it as-is
>>>2. Drop the supported check from docs - each plugin can offer a check if
>>>it likes
>>>3. Introduce a constant handle, so Service("someid",QUERY_SERVICE) is
>>>the supported check
>>
>>4. Service("?someid"), maybe...
> 
> 
> cwieninger@gmx.de wrote:
> 
>>IMHO 3. is the best solution.
> 
> 
> 
> Since I don't want to enforce extra supported checks, I tend to a
> variation of 2: Suggest to use NULL as install check without action.
> Enforcing an install check in docs and not using it is inconsistent.
> 
> ----8<----
> <tt>Data</tt> points to a custom data structure. For each id string
> there should be a specification that describes the format of the data
> structure, and any change to the format should  be reflected by a change
> of the id string.
> <p>
> The function shall return true for any service ID string it handles, and
> false otherwise. The plugins have to agreee in which situations the
> service may be called, for example whether the service may be called
> from every thread, or just from the main thread. Plugins can implement a
> 'service supported' check by returning <tt>true</tt> without performing
> any actions when called with <tt>Data=NULL</tt>.
> ----8<----

Since the Data parameter in the function calls has NULL as default,
a plugin implementing Service() _must_ be prepared to handle this
case. This text makes it look like this is somehow optional and up
to the individual plugin. If you want it that way, then we'll have to
remove the NULL default parameter. Personally I'd rather keep it as it
is right now.

Klaus
  
Udo Richter Aug. 21, 2005, 1:46 p.m. UTC | #22
Klaus Schmidinger wrote:
>> from every thread, or just from the main thread. Plugins can implement a
>> 'service supported' check by returning <tt>true</tt> without performing
>> any actions when called with <tt>Data=NULL</tt>.
>> ----8<----
> 
> Since the Data parameter in the function calls has NULL as default,
> a plugin implementing Service() _must_ be prepared to handle this
> case. 

----8<----
Plugins should expect to be called with <tt>Data=NULL</tt> and may use
this as a 'service supported' check without performing any actions.
----8<----

Cheers,

Udo
  

Patch

diff -au vdr-1.3.29-orig/plugin.c vdr-1.3.29/plugin.c
--- vdr-1.3.29-orig/plugin.c	2005-01-30 15:05:20.000000000 +0100
+++ vdr-1.3.29/plugin.c	2005-08-18 18:20:05.000000000 +0200
@@ -99,6 +99,11 @@ 
   Setup.Store(Name, Value, this->Name());
 }
 
+bool cPlugin::ExtensionInterface(const char *ExtensionId, void *Data)
+{
+  return false;
+}
+
 void cPlugin::RegisterI18n(const tI18nPhrase * const Phrases)
 {
   I18nRegister(Phrases, Name());
@@ -372,6 +377,43 @@ 
   return NULL;
 }
 
+cPlugin *cPluginManager::CallFirstExtension(const char *ExtensionId, void *Data)
+{
+  if (pluginManager) {
+     for (cDll *dll = pluginManager->dlls.First(); dll; dll = pluginManager->dlls.Next(dll)) {
+         cPlugin *p = dll->Plugin();
+         if (p)
+            if (p->ExtensionInterface(ExtensionId, Data)) return p;
+         }
+     }
+  return NULL;
+}
+
+cPlugin *cPluginManager::CallPluginExtension(const char *PluginName, const char *ExtensionId, void *Data)
+{
+  if (pluginManager) {
+     for (cDll *dll = pluginManager->dlls.First(); dll; dll = pluginManager->dlls.Next(dll)) {
+         cPlugin *p = dll->Plugin();
+         if (p && strcmp(p->Name(), PluginName) == 0)
+            if (p->ExtensionInterface(ExtensionId, Data)) return p;
+         }
+     }
+  return NULL;
+}
+
+bool cPluginManager::CallAllExtensions(const char *ExtensionId, void *Data)
+{
+  bool found=false;
+  if (pluginManager) {
+     for (cDll *dll = pluginManager->dlls.First(); dll; dll = pluginManager->dlls.Next(dll)) {
+         cPlugin *p = dll->Plugin();
+         if (p)
+            if (p->ExtensionInterface(ExtensionId, Data)) found=true;
+         }
+     }
+  return found;
+}
+
 void cPluginManager::StopPlugins(void)
 {
   for (cDll *dll = dlls.Last(); dll; dll = dlls.Prev(dll)) {
diff -au vdr-1.3.29-orig/plugin.h vdr-1.3.29/plugin.h
--- vdr-1.3.29-orig/plugin.h	2005-01-30 15:03:48.000000000 +0100
+++ vdr-1.3.29/plugin.h	2005-08-18 20:22:20.000000000 +0200
@@ -50,6 +50,8 @@ 
 
   void RegisterI18n(const tI18nPhrase * const Phrases);
 
+  virtual bool ExtensionInterface(const char *ExtensionId, void *Data = NULL);
+
   static void SetConfigDirectory(const char *Dir);
   static const char *ConfigDirectory(const char *PluginName = NULL);
   };
@@ -88,8 +90,12 @@ 
   static bool HasPlugins(void);
   static cPlugin *GetPlugin(int Index);
   static cPlugin *GetPlugin(const char *Name);
+  static cPlugin *CallFirstExtension(const char *ExtensionId, void *Data = NULL);
+  static cPlugin *CallPluginExtension(const char *PluginName, const char *ExtensionId, void *Data = NULL);
+  static bool CallAllExtensions(const char *ExtensionId, void *Data = NULL);
   void StopPlugins(void);
   void Shutdown(void);
   };
+#define PATCH_EXTENSIONINTERFACE
 
 #endif //__PLUGIN_H