Comment 41 for bug 587597

Revision history for this message
Jacek Sieka (arnetheduck) wrote : Re: Plugins support

For now, I'd say it's ok to call the pluginmanager directly - eventually it is probably the Speaker that should be extended so that an observer can halt further processing of the event in which case the pluginmanager would use that to suspend execution. I agree that an observer-based solution would be nice but supplying that level of indirection requires non-trivial work. The most probable way forward here is to have pre/post-events where the pre events allow control while post events remain read-only. Until that happens, I see no alternative to direct calls.

As far as affecting the core goes, in this patch there are well-defined points where the core is being manipulated by the plugins, so those points should eventually be hardened against reasonable abuse. Read-only plugins have equal opportunity to do damage whether they affect the core or not (null pointer writes...?), but leaving out the ability to modify core behavior will lead to plugin authors resolving to hacks or doing mods, neither of which would be desirable according to the state goals of a plugin system.

I'm not entirely comfortable with the api taking locks, but the solution to that issue is to simplify the dc++ threading model which is outside the scope of this patch and will have to be addressed later.

Regarding c vs c++, a c api can generally be consumed without writing a wrapper while c++ always requires a wrapper. On the other hand, as steven points out, there's no real benefit in supporting more than c++ and a scripting language (lua or javascript probably since these are to my knowledge the only popular ones that lend themselves to easy embedding). There's the issue of binary compatibility between releases. I don't think we need to maintain binary compatibility between compilers but we should strive to be compatible between dc++ releases (since most windows users download binaries...) so we can only use a subset of c++ (I think qt has a good document somewhere with reasonable guidelines...) in order not to change the layout of objects etc - in other words a vote for c++, but not the full glory.

Now, to the implementation =)

* Memory:
I don't like the memalloc bits - each entity should manage its own memory. The node that allocates memory should also deallocate it and if any data needs to live past the call it should be copied.

* Usage of event ids/enums:
This is practically a reimplementation of the win32 message loop which is a pain in the ... to work with. Examine any win32 wrapper and you'll see that they try to hide it as well as is possible...Also there's a high probability of hook id clashes around EVENT_USER if plugins start making use of intercom... instead, I think it would be better with something akin to COM / IDL... small structs with function pointers (virtual functions maybe?) that define an api for a specific part. Something like dccore but for each part - for example a queue api would look something like:

struct QueueApi {
void (*add_item)(...);
void (*remove_item(...);
void (*set_priority)(...);

void (*on_item_added)(CallbackType proc);
...
};

or possibly:
struct QueueApi {
virtual void add_item() = 0;
...
};

DCCore would then have functions to associate guid's with api implementations (and get an implementation based on guid), so the core could provide some implementations for queue, clients etc (basically one for each section of tagHookEvent)...also, gui/platform specific parts could be registered by the gui's without letting the core know about platform specifics since the struct containing the definitions for a particular guid would be defined in a platform specific header. As long as these structs only grow at the far end, newer versions are backward compatible retaining the compatibility property of message loops while allowing the compiler to do at least rudimentary static checks and doing away with the global nature of message id:s...
Basically, dccore becomes a registry of other dccore-like structs each identified by a name/guid that others can query for implementations. Whether these come from a plugin or dc++ itself doesn't matter really so the overloadability is retained...

* Guids:
I'm not a big fan of guids - strings or fourcc's spring into mind as friendlier alternatives (if you really intended guids, or uuids as they're called nowadays)

* Api versions
The current approach with current version and compatible version is fine - possibly I'd just use an int for simplicity (what semantics would you attach to 1.0 vs 1.01 vs 2.0 other one is newer than the other?) and perhaps version each part separately (each struct in the above example). In other words, dccore would have it's own version, queueapi it's own, etc.

That's enough for a start I think =)