Plugin support

[Crise / MW]

Ok, I'll leave out the sales pitch... here is a potential patch for adding support for plugins to DC++, compiles cleanly (ie. shouldn't generate any warnings) under mingw and visual studio of course and has been tested and working.

The code itself has been in ApexDC++ for a while so it has gotten real life use as well, of course migrating it to clean DC++ needed a few changes here and there but nothing major.

Should also compile and work on linux as it is... but this I have not had chance to test at all so that is just on paper for now.

The major difference between this patch and what's in ApexDC++ is that it does not include a full settings page for plugins which ApexDC++ has. This is simply because I don't know a squat about DWT. Instead this patch has set of chat commands added, so you can play around with the plugins, but if this gets accepted then I certainly hope someone is willing to invest into a settings page for usability sake.

Oh and few changes to the code come directly from bcdcpp, you'll spot them I am sure :).

As for the plugins currently three exists.. a pure C sample that (you guessed it) really doesn't do anything productive and then a plugin version of bcdcpp's lua (this one is pretty direct port, so it is C++).

The third one is not so impressive... it adds support for various media player chat announces (spam is too negative of a word), although primarily I created it to proof that it is possible to make a plugin that modifies the GUI, even though the API itself has little to no support for such plugin (on windows anyways),

The first two plugins can be compiled with both mingw and visual studio and are also hopefully linux friendly, the third not so much.

Oh and you can mix plugins... so mingw compiled dcpp will cope just fine with visual studio compiled plugins or vice versa (obviously this also means that the stl's used can be different). In theory you should also be able to create plugins with languages such as VB and Delphi but this very much theoretical.

Well do what you want with it... it's posted now.

[Crise / MW (markuwil)]

Here is the source for the C sample plugin. (includes visual studio project files, code::blocks project files (cross-platform ide, capable of woking with various compilers including mingw), and also windows batch file for mingw.

The rest of the plugin source codes and compiled binaries can be found here: https://sourceforge.net/downloads/apexdc/Plugins/1.3.2/#download

[Toast (swetoast-deactivatedaccount)]

I myself is totally all for this imagine this plugin system in the major clients that would give Direct Connect the edge it needs since mods get unmaintained quite often this would actually help development since those mod developers could become plugin developers.

[cologic (cologic)]

I haven't yet looked in detail at this revision of the code, so I can't comment on it, but at a glance it looks similar to the versions I've seen, which were mostly pretty good. In general, of course, I support this.

[Crise / MW (markuwil)]

You could actually apply the patch? when I toast tried it and then later me all we got was patch exe crashing... also just noticed that there is something funny going on with line endings, I thought I made sure all were the unix flavour.

[Crise / MW (markuwil)]

Here is a fixed patch, since the initial one seems to have ended up as corrupted somehow...

[cologic (cologic)]

I haven't tried to apply/compile that patch yet, but will do so. Also, worth being aware that Lua 5.2 is coming along - see http://www.corsix.org/content/look-lua-52-work3 for changes. Shouldn't have a major effect, but probably the Lua implementation should be 5.2-based rather than 5.1-based.

[Crise / MW (markuwil)]

I haven't upgraded to 5.2 lua since it's not marked as stable yet and because of the whole "and everything can still change" thing.

[cologic (cologic)]

Sure, for the moment it makes more sense to stay on 5.1. But 5.2 might be close enough to release that by the time this is committed to DC++ (if it is), then the the initial Lua standard for DC++ to use could be 5.2; alternatively, one could start with 5.1 and just specify that anything that'll break with 5.2 isn't supported for any longer length of time.

[Crise / MW (markuwil)]

Both approaches are good... though in a sense since bcdc still uses 5.1 the latter option might be better to start with, so users can just copy over their scripts from bcdc without changes (this would help in having users move from bcdc to DC++).

Also rember that it's not necessarily practical to think of the plugins (in this case the lua) as too much part of DC++ itself because plugin is an entirely separate entity from the main app itself (which is why they have they own version number).

I mean in theory the development of the lua plugin, at least in terms of released versions for example, doesn't need to be tied to those of DC++ at all (as long as the API does not change in a way that breaks compatibility which should be avoidable in most cases).

[Crise / MW (markuwil)]

Here is an updated patch spotted a stupid mistake I made in AspectChat...

[Steven Sheehy (steven-sheehy)]

- PluginManager::onChatDisaplay - grammar
- New files show copyright arne, but unless the original patch writers explicitly give copyright to arne it should be copyright the original authors. As I understand, DC++ requires original patch writers to hand over copyright.
- It seems strange to me that some plugin commands are called from the core and some from the UI. For example, onOutgoingChat is called from the UI but onIncomingChat is called from core. Seems like they should both be either in the core or in the UI, not mixed. Since the core has knowledge about outgoing chat, I don't see why it shouldn't be able to handle it as well. Besides chat there are a few others like this.
- CONV_ACP_TO_UTF8 / CONV_UTF8_TO_ACP / CONV_UTF8_TO_WIDE / CONV_WIDE_TO_UTF8: None of these are unix compatible.
- Some functions don't follow DC++ naming camel case scheme: MemAlloc, StrConv, etc.
- What is the purpose of StrConv & MemAlloc anyway? I don't see them being used anywhere.

[Toast (swetoast-deactivatedaccount)]

all fixable tnx steven for the feedback

[Crise / MW (markuwil)]

The functions which don't follow camel-case are not for the core to use, but for plugins, well one of them is called from MainWindow but that's an exception because the hook in question does not need any preparatory work so it would be a waste to make another on* function in pluginmanager for it (StrConv is explained below, MemAlloc further down).

As for CONV_ACP_TO_UTF8 / CONV_UTF8_TO_ACP / CONV_UTF8_TO_WIDE / CONV_WIDE_TO_UTF, 8, they are just as compatible with unix as the functions they call in DC++ core I would think. They are just wrappers, and yes thus plugins could rewrite them for themselves but I simply fail to see any benefit in having plugins replicate functions we have perfectly good ones we can supply (especially since what is need for windows and unix is different, ie. by providing them in the API we ease portability of plugins).

As for the onOutgoingChat. I had it calling from core before but that resulted in some "issues" with plugins that try to add /commands and the SEND_UNKNOWN_COMMANDS option (Once again the way around this is to have plugin command use different leading character, but that is hardly practical since / has been as good as reserved for client side commands).

Now then MemAlloc is needed so plugin can safely allocate memory which host app can then free (the use comes in conjunction with StringData and onChatDisplay).

[Crise / MW (markuwil)]

Oh regarding the copyright, well for the pieces that come directly from bcdcpp that's the place you need to ask... as for the rest I wouldn't have made this patch unless I was ready to give over the copyright.

But if you need me to say/write it: The copyright is all yours arne...

[cologic (cologic)]

I'm willing to assign copyright on the bits of BCDC++ I see in there (e.g. the SettingsManager changes) to arne, yes.

[Big Muscle (bigmuscle)]

Does this plugin mechanism support current DC++ listeners? I mean that if something in core will call fire(something,a,b,c), plugins will be able to execute its code for such event?

[cologic (cologic)]

I certainly hope not.

(The answer is no, in general, with the current patch. It's not a complete 1:1 mapping between DC++ internals and a somewhat more abstracted set of DC semantics, nor should it be.)

[cologic (cologic)]

Ah, I should re-state that comment, since the phrasing got confused: "It's not a complete 1:1 mapping onto the current DC++ internals. Instead, it maps onto a slightly more abstracted set of useful DC semantics. This is intentional."

[Crise / MW (markuwil)]

Short answer: not directly...

Long answer: It doesn't support them right now directly... because the listeners use so many C++ specific features it was easier just to expose select listeners through hooks also it allows more precise control over what info plugin has access to.

There is a way to add full support for this in theory but I am not sure how much work that would actually be as it would essentially mean turning or hook subscriber into an universal listener. Also when DC++ fires an event most of the time the a, b and c are something that can not be used by a plugin directly, because plugins do not know DC++ internal types. So it would also be necessary to be able to wrap any object members we want plugins to have access to into structs etc.

This is what is essentially done for the listeners exposed to plugins right now.

If we give up on the API being pure C then this comes notably easier but still not necessarily easy.

[Crise / MW (markuwil)]

Also as cologic stated above... this limited exposure is intentional (I hate not being able to edit my comments), not only because the other path would be significantly more complex to create while keeping it a pure C api but also because this way, as stated, have more precise control over what plugins have access to and what kind of things they can and can't do.

[cologic (cologic)]

This is the same patch as before, but converted from LF to CR/LF formats. The GnuWin32 patch 2.5.9 version seems to prefer it this way.

[Steven Sheehy (steven-sheehy)]

On Tue, 2010-06-01 at 09:08 +0000, Crise wrote:
> The functions which don't follow camel-case are not for the core to use,
> but for plugins, well one of them is called from MainWindow but that's
> an exception because the hook in question does not need any preparatory
> work so it would be a waste to make another on* function in
> pluginmanager for it (StrConv is explained below, MemAlloc further
> down).

Regardless of what your naming standards are, those are not the naming standards of DC++.

> As for CONV_ACP_TO_UTF8 / CONV_UTF8_TO_ACP / CONV_UTF8_TO_WIDE /
> CONV_WIDE_TO_UTF, 8, they are just as compatible with unix as the
> functions they call in DC++ core I would think. They are just wrappers,
> and yes thus plugins could rewrite them for themselves but I simply fail
> to see any benefit in having plugins replicate functions we have
> perfectly good ones we can supply (especially since what is need for
> windows and unix is different, ie. by providing them in the API we ease
> portability of plugins).

They are not compatible with unix, as I stated. I wrote the text conversion support for unix inside DC++. ACP has no concept in unix systems, it is Windows specific (side note: ANSI Code Page is a misnomer, it should be called Windows Code Page). In fact, you will not see Text::*acp* functions called by the core; they are only to be called by the Windows GUI. If you need conversion support, you need to use Text::fromUtf8 and Text::toUtf8 or the generic Text::convert functions.

> As for the onOutgoingChat. I had it calling from core before but that
> resulted in some "issues" with plugins that try to add /commands and the
> SEND_UNKNOWN_COMMANDS option (Once again the way around this is to have
> plugin command use different leading character, but that is hardly
> practical since / has been as good as reserved for client side
> commands).

Sounds like you ran into bugs and implemented a workaround instead of fixing the original, more logical design.

[Crise / MW (markuwil)]

Pardon me, but I was not arguing against DC++ naming standards, I was simply stating my reasons for naming them the way I did. Besides (if it is decided that this gets into DC++) you don't need me to change some capitalisations, anyone can do that since I won't be the one putting it in anyways as I don't have any access to the repository.

Regarding the ACP stuff I do see your point and that is easy to sort out. But if the API provides text conversions it is only logical to provide utf-8 to wide char conversion as well if unix does not need this then it's no-ones loss.

Regarding your comment about onOutgoingChat, sure that might be partly true but the idea of onOutgoingChat & co. is to allow plugins to react to outgoing chat before UI does anything with it. If you want it is easy enough to change it to something like onPreprocessChat and move the hook event(s) under the UI hook id.

One scenario that mandates onOutgoingChat be called from UI is the case where plugin wants to extend an existing client side command for example (/help being the first one that comes to mind).

There are some other things that have come up in recent(ish) discussions about the implementation anyways, so I'll be updating the patch soon.

[cologic (cologic)]

For the record: Thor's Lua API proposal lives at <http://dcpp.hu/bcdcluaapi.html>.

[cologic (cologic)]

Er. Lua API as presented by Lua, that is (it doesn't propose any particular changes that I can tell to the C side).

[Crise / MW (markuwil)]

Updated patch, still doesn't have all the changes I had wanted to include but at the same time I realised months had passed.

The attached patch also has one line change to RichTextBox.cpp which is unrelated to plugins but I was unable to compile under mingw without it (oddly enough QueueFrame compiles just fine). Oh, and naturally the patch is against latest bzr rev at the time of writing this.

[Steven Sheehy (steven-sheehy)]

Looks like most of the things I mentioned are fixed. I still have some major issues with it though.

- Can you explain why the calls to the PluginManager will return if they are false? For example: if(PluginManager::getInstance()->onIncomingConnectionData(this, aLine)) return; in dcpp/UserConnection.cpp. I would think that regardless of what happens in the call to the plugin that you want to continue the flow and setup the connection. If a plugin fails that should not impact the main program. Am I missing something?

- What safeguards are there that a plugin won't cause the core to enter an unsafe state or crash? As far as I can tell, plugin callbacks are executed within the same core thread that initiated it so it's possible that the plugin can cause a deadlock or other resource contention.

- The plugin architecture does not seem to be extensible. If a program using the dc++ core (like say perhaps LinuxDC++) and wants to add a custom event that may or may not be applicable to DC++ there is no way to dynamically add it without modifying the core. I would think this needs to be designed to be extensible through inheritance or other means.

- The plugin notification system is synchronous and procedure based. All throughout the core we see mentions of the plugin system via PluginManager::getInstance()->onFooEvent() calls. This strongly couples the core classes with the plugin system. It is not the responsibility of these classes to deal with plugins and thus they should not have any mention of them. I strongly feel that it should be an asynchronous event driven architecture. There is already a precedent for an event driven architecture since that is how all the listeners are utilized. For example, instead of calling PluginManager::getInstance()->onFooEvent() inside a core class, it should fire(FooListener::FooEvent(), args); and the PluginManager should subscribe to that event. Where possible the PluginManager should listen to existing core events and if no event exists then they should be added to an applicable listener. This hides the implementation of the plugin system from the rest of the core and would allow us to modify the plugin system without affecting the others. This abstraction would be useful, for example, if in the future we want to put plugin events into a queue and process on a separate thread.

- subscribtion -> subscription

- This big of a patch should've really been a bzr branch so we could track the changes and review the code before merging.

- Has Jacek reviewed this? This a major core change and I don't see much discussion on it here (besides my ramblings). I see major issues with the patch as documented above and would like to get his view on the patch.

[Toast (swetoast-deactivatedaccount)]

Jacek hasn't reviewed this but we requested it several times over the last year so for now we rely on your reviews and adjust to em and we have internal discussions in dcdev over how it works so for now it has to do with keeping atleast this discussion up most of the core developers are on board already.

ill let crise answer the changes in the patch just wanted to give a clear status.

[Crise / MW (markuwil)]

The calls to pluginmanager return if they are true, because that implies plugin handled that event for us, ie. we don't want core doing anything about that particular instance of it anymore.

I agree that the plugins should not be given the possibility to put the core into an unsafe state, however, if we prevent that what can plugin do at that point anymore?

Take a simple example... if plugin wants to do something when user writes "/foo arg .... narg" to some chat... we can't have the core let it send the original text user inputted anymore, especially if the command invoked chat related action. It's really a simple catch and block (filter) based system.

About extensibility, there is registerMessage, registerRange and seekMessage to use... and you can also add constants to plugindefs enums below the *_USER values, provided they don't interfere with existing ones (also the *_USER constants do not have to be the same for all implementations, ie. they can be moved out of the way, since plugins shouldn't really use them directly they are just base values for the aforementioned message register functions). The createHook, destroyHook, setHook unHook and callHook (now in PluginApiImpl) are also usable by the plugin host just as they are to a plugin.

So to add a hook you would do something like:
id = PluginApiImpl::registerMessage(HOOK_ID, "ldcpp.DummyHook")
hook = PluginApiImpl::createHook(id, HOOK_EVENT, ...);
PluginApiImpl::callHook(id, ....);

And for plugin to subscribe to this:
id = dcpp->seek_message("ldcpp.DummyHook");
hook = dcpp->set_hook(id,...);

(Above written on the spot, so mistakes possible, error checking omitted)

About the plugin events being asynchronous, and listeners... it already uses some listeners when feasible, ie. plugin doesn't need to be able block the event. And suffice to say it is already causing me a headache as it is when it comes to creating a plugin... since dcpp thinks it is ok to call ClientDisconnected to clients when ClientConnected hasn't necessarily been fired for that client... and it is not ideal for a plugin.

To be continued...

[poy (poy)]

just a nitpick: the current event driven architecture (Speaker class) is not asynchronous; that is the reason GUIs must try not to do any lentghy operations when they process these events. still, using it would be nice.

[Crise / MW (markuwil)]

I agree, but as explained earlier plugins need to be able to affect what happens after plugin has processed an event... which is why using the existing listeners is not the ideal solution in 9 cases out of 10... to be completely honest I even considered not using any listeners, except timermanager and settings, for plugin hooks at one point.

Currently it uses the following listeners (in addition to aforementioned):

ClientManagerListener::ClientConnected
ClientManagerListener::ClientDisconnected
QueueManagerListener::Added
QueueManagerListener::Moved
QueueManagerListener::Removed
QueueManagerListener::Finished

And in doing so it already creates the following limitations,
 - Plugin can't stop certain hub from being connected to.
- Same about being unable to prevent certain file from being added to Queue.

So to put it in other words, for events that can be seen as purely informal like removal of hub or queue item, or the download destination for queue item being changed etc. the listeners are fine.

To use listeners (as they are now) for everything, however, would mean to limit plugin to being little more than an observer. This is fine I guess for plugins that will do things like: SFV/Antivirus checking or Report/Log generation, but for anything else it is too restrictive kind of defeating the purpose of plugins.

While it can be argued that overriding user action such as adding download or connecting to a hub is not desirable action by a plugin, the valid counter argument for that would be that since every plugin is user installed user would not install a plugin that does this without some logic behind it (unless they want to make their own life hard, the point being there is consent from user to this to an extent).

Regarding desirable and undesirable behavior from plugins, there are two options to address this, one (the easy) is to implement guid based blacklisting of some sort for rogue plugins and the other would be to generate a "permissions" request, on install, based on what hooks the plugin subscribes to. The former approach has no impact on the API while the later might need subtle modification to it (I haven't brought this up since it is only very externally part of the API).

[Big Muscle (bigmuscle)]

I have one thing. What about putting Client abstract class as a part of plugin API? Then new procotol could be added by plugins (for example DHT could be a separate part).

[Crise / MW (markuwil)]

I had a goal of keeping it pure C API... which allows it, in theory, to be used with languages such as delphi and (yack) VB.

If this is doable as C++ only extension, without breaking the underlying C API then it is certainly worth it I'd say... although it would be strictly tied to dcpp based plugin hosts.

Another door that we probably want to keep open is for dc library X that we have yet to know, being able to implement and reuse the same API, so that plugins could in theory be shared between the two.

It would have been easy to make a very powerful plugin api strictly for dcpp using f.ex. swig, or even simply using interface classes instead of abstracting everything and sticking to C.

[Steven Sheehy (steven-sheehy)]

Personally, I would prefer a less powerful but cleaner and safer plugin system that could not affect what happens after the plugin processes the event. Cleaner because it could use the listener system for all communication. Safer because it would be read only data passed and could be processed in a separate thread (or even a separate process like Chrome). This may help us to reduce the likelihood of putting the core into an unsafe state. Because regardless of how much we try to blame plugin writers for causing crashes, freezes, etc., the users won't care and will blame DC++. I don't fault them either because I get pissed at Firefox when it crashes even though I know it is a plugin since Firefox should've prevented it in the first place.

When I was originally looking at this I assumed it would be used for mainly plugins that affect the UI and did not realize that it was powerful enough to affect the core behavior. However, I can see the advantages of having a more powerful plugin system and if that's what people want I won't stand in the way. The overall purpose of the plugin system is to reduce the multitude of DC++ mods and if this capability is necessary to do it, then so be it.

But I have to be adamant in calling for an event driven architecture. Whether this uses the Speaker system or some enhanced version of it is not as important. The point is that the core should not be calling the pluginmanager directly, but instead it should generate an event and notify all observers of that event. This is the observer pattern and it allows us to decouple the plugin system from the rest of the core.

As for a pure C API, I think we should support a predetermined set of languages to write plugins in instead of trying to make it as generic as possible. For example, say we want to support C++ and lua only then we don't need to worry about supporting VB or delphi. Thus if it was possible to write a plugin system in C++ for lua and C++ (I've no idea if that's possible, just an example), then that is the approach we should take. I would take C++ over C if it supports the desired languages.

On that topic, one question I had was how do non-C++ plugins utilize the dcptr_t object types within the data structures? Most of the types within the structs passed to the plugin callback are POD types and can be utilized easily, but how is a pointer to, say, Client* used in a non-C++ plugin? Don't we need to extract the useful elements from the object being pointed to and send them as POD types?

[Crise / MW (markuwil)]

I'll first answer about the dcptr_t objects, plugins don't use them... think of them as "handles". They could even be obfuscated instead of being direct pointers to dcpp objects.

The object members of the structs are simply something the party which implements callbacks use (unless otherwise stated) ie. they are link to what is assumed to implement the functionality of what the pod type used by the plugin represents (hence why callbacks use the object member of the structs as a frequent argument).

Note: when I use the term callback above I refer to inbound hooks (from dcpp perspective), you probably referred to outbound hooks, which I simply refer to as hooks. Read the top of PluginDefs,h for some clarification.

About extracting all useful elements from the object being pointed to and sending them as pod types in the structs... isn't this done for quite a bit of data already? (well we aren't making copies but taking advantage that f.ex. getHubUrl() returns a reference, so c_str() is fine), but callbacks need something to trigger actions such as sending a chat message.

About less powerful and safer plugin api... I can understand your reasoning for wanting this, however, if we go down that path we shouldn't load a plugin to begin with. At least in windows simply loading a dll can allow it to do various things even without using an API provided. (see msdn: http://msdn.microsoft.com/en-us/library/ms632589%28VS.85%29.aspx) For example a loaded dll can with relative ease hijack the host processes entire window procedure. For instance various addons built for apps that don't have public api probably use similar techniques to this.

About UI and the patch here, the only way plugins with this could affect the UI in any drastic manner is by using OS specific methods such as the one presented above, the only thing that we provide about the UI (aside from the few choice chat related events) is a notification about the UI being created which has platform/implementation specific argument.

We also don't have any UI specific callbacks to do things such as adding menu items etc. which is a severe limitation, because the lack of these will lead plugin authors to research into other hackish methods to accomplish them, because I have no doubt they would want to do things like add a menu item for their plugins.

I have intentionally not provided API's for the UI stuff, with this patch, though because they would need more code to be inserted into the UI side (nor do I have any idea how to abstract UI events in OS/UI independant fashion, if that is even possible, if only dcpp was using a common UI for all operating systems things here would be bit different).

I had something else... but maybe it will come back to me later.

[Toast (swetoast-deactivatedaccount)]

Why add a compiled DC++ here ? seems kinda redundant since we all are able to compile it ourself

[Crise / MW (markuwil)]

Updated patch, for you lazy people not bothered to fix 3 extremely simple conflicts... no other changes (haven't had the time unfortunately)

[Crise / MW (markuwil)]

So, to address the issue of PluginManager::onFooEvent calls that Steven brought up... to do that the existing listeners in dcpp are useless since most of them are fired in "past tense" in other words fired after an action is complete.

So even if it is possible to address the listeners not being able to affect what happens after the call, to make any sensible use of them in cases where they are not used right now (for plugins) would need either new events added for everything or drastically changing the existing events.

About onFooEvent calls in general, they are purely convenience functions anyways each eventually leading to PluginManager::callHook, so if individual events having functions in PluginManager is the root of the problem it is possible to make the calls with a single function provided you initialize the needed structs "on the spot" for every call.

The only sensible solution to make use of listeners as much as possible that I could think of is to add PluginEvent() event to each needed listener and member method that returns the POD representation of the object to the needed classes. The question is, how much is gained by doing this contrary to having it the way it is now... the number affected lines will only stand to increase. Though I can see certain advantages of having the POD representations returned by a member method.

[Jacek Sieka (arnetheduck)]

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 ...

[iceman50 (bdcdevel)]

Ok, so out of pure interest I decided to start making a plugin page for settings, although it's very imcomplete and needs a bit of help id figure id post the diff here and maybe get some of the things finished off to work properly my biggest problem currently is adding entries to the list, the add button does work and it successfully loads plugins (i've already tested), so hopefully someone will help out =)

[iceman50 (bdcdevel)]

Updating my original patch to a somewhat more working one enjoy =) (will try to update asap)

[iceman50 (bdcdevel)]

Well finally completed the plugin page, everything is fully functional =)

[Jacek Sieka (arnetheduck)]

iceman, could you open a separate bug for the gui...?

[iceman50 (bdcdevel)]

will do it was requested to be added in here but no problem

[Steven Sheehy (steven-sheehy)]

I've brought this up once before but not sure if anyone noticed it. With the latest developments of a plugin page, this seems even more relevant:

Wouldn't it make sense to track the code changes for this bug in a separate branch? Patches are great for small changes but become cumbersome when introducing major changes like this. Branches allow users to track the ongoing progress being made on the feature, test the latest changes, and contribute back by creating their own branch on top of it. The latter of which would be useful for iceman50 and his plugin page bug instead of trying to create a patch on top of an already enormous patch. This is the whole point of using a distributed revision control system like Bazaar, after all.

Just an observation. Take it or leave it. :)

[Jacek Sieka (arnetheduck)]

+1 for branch...

[iceman50 (bdcdevel)]

Well a branch has been created by Crise for plugins support, just waiting for him to finish what he's doing in regards to changes with the API
here's the branch adress https://code.launchpad.net/~markuwil/dcplusplus/dcpp-plugins

[cologic (cologic)]

http://lcsd05.cs.tamu.edu/slides/keynote.pdf has a nice presentation on this topic, entitled "How to Design a Good API and Why it Matters" by Joshua Bloch. It points out some specific hazards and advantages. Some of the more interesting suggestions:

-It's probably useful to have three distinct plugins to test an API to ensure it actually supports distinct usages.
-It's harder to remove things later than add them, so it's worth being conservative and risking perhaps not supporting all use cases rather than getting it wrong and having to support that indefinitely.

It has some other comments too, but those are less notable (documentation is useful. Surprise!).

[Jacek Sieka (arnetheduck)]

Any updates?

[Crise / MW (markuwil)]

So far mainly in my head, since various things have kept me busy... that said I do agree with your comments for the most part.

[iceman50 (bdcdevel)]

Any updates? considering the last 10 months of updates to the DC++ it would be nice to know the status of any progress or an updated patch if possible Crise.

[Crise / MW (markuwil)]

Ok, few comments in response to the points brought up in dcdev chat:

1. Regarding the types of id'ing used... and the differentiation between different groups. The meaning between generic vs dcpp is very much obvious as stated; other than that the ids can be described as:
[generic|dcpp].[something relevant].[interface struct|event name].

Basically the middle bit is there just to add another level of uniqueness to the names (as i felt using windows type guid as a bad idea, if for nothing else for the sake of debugging plugin calls). Plugins should generally use a full random guid (such as a low collision hash) to identify themselves though: so the above could also be interpreted as, [guid].[something relevant].[interface struct|event name]. DC++ doesn't have a guid though being the host hence the use of dcpp). The "something relevant" bit can also be used to link events and interfaces together somewhat. Generally speaking the guids used by the system can be anything so long as the definition of "globally unique" is met at least in the small universe that is DC & Plugins (I chose human readable naming for my own convenience as noted, but the logic doesn't require it, granted I see possibilities for plugins to take advantage of it).

2. Versioning: I ended up going with each interface having an independent version (mostly) which is represented by single int, all interfaces under this versioning are considered backwards compatible indefinitely (however, the version of DCCore can naturally be used to eliminate very old plugins from being loaded, if necessary).

3. UI hook(ing): this is somewhat of a hairy subject because properly abstracting controlled ui hooks without essentially either forcing people to re-implement a processing structure of certain ui lib or os is just near impossible (or then we would have to go with "make it equally difficult for everyone" and come up with independent schematics for those hooks). If you ask me the most clean way we can implement ui hooking is to piggyback on the user commands system the protocol defines. Of course this has pretty severe limitations, but it can be done... ie. plugin feeds the core a self generated user command then when it comes in (or out, technically) through the protocol interface block and act on it.

Outside abusing the user commands I don't see it as practical to implement many ui hooks... notepad++ does ui and plugins in a fairly decent manner (ie. give them a "Plugins" top level menu, then under it an item for each plugin, that requests it, that it can then customize). This is something that can be looked into but for now it is more important to get the first "final" version in. If we go more in depth with the ui hooks we will only invite a nightmare on the ui code (if the ui supported say, skins through xml or some other independent form of defining ui elements then ui could be hooked into simply by letting plugins modify these "blueprints" the ui is based on but alas that is not the case).

[Crise / MW (markuwil)]

Regarding thread safety, being something arne brought up... the result is to sum it up in one word: ugly. The two major functional interfaces (DCHub and DCQueue) now have release(...) and copy(...) functions in them, the respective *Data structs also have an isManaged member that is False if release must be called for said object.

Where copies are not made by dcpp thread safety is achieved by string cache and use of locks. Which means if plugins need to take the passed managed object outside calls which are direct descendants of the call to PluginManager::runHook they are responsible of making a copy of the object using the copy function and also taking care not to use resulting objects after their "handle" (ie. object member of f.ex. HubData) is out of scope, which they can track with certain events (such as onHubOnline/onHubOffline).

Of course the object handle of UserData has been eliminated as unsafe... we could also eliminate the equivalent handle in HubData, but i see no reason for that because we can track the situation where that handle (or any copies plugins created) becomes invalid on plugin side.

[Crise / MW (markuwil)]

Oh one point I forgot to cover... missing settings types, tbh I'm not sure if I want to mimic dcpp here... as in why floats over doubles (yes floats could be technically faster, on the x86 architecture, but if you want precision for math then double should be the way to go) for example... or why not dedicated bool settings type. Or how about more complex serialized settings (yes this came out slightly sarcastic).

The point above is... we can't support every kind of setting value that there might be now and in the future. I don't even remember what settings dcpp uses floats for tbh. Originally I just had strings as settings values because a string can represent anything... only when I added support for querying core settings some other value types were added (because it would have been highly inconvenient to convert ints to strings for the api and then back to int on the plugin side for use).

[poy (poy)]

Fixed in DC++ 0.800.