Need advice on change 291

Development
1

Hi all,

In change 291 I’ve added new API functions setPluginPath and pluginPath.
I think it would be reasonable to split directories ‘colors’, ‘engines’,
‘extensions’, ‘tools’, and ‘contrib’ into separate elements of plugin path,
so

foreach (const QString& path, d->searchDirs) {
qDebug() << “Loading plugins:” << path;
loadPluginDir(path + “/colors”, settings);
loadPluginDir(path + “/engines”, settings);
loadPluginDir(path + “/extensions”, settings);
loadPluginDir(path + “/tools”, settings);
loadPluginDir(path + “/contrib”, settings);
}

will be replaced with simple iteration on searchDirs. Call of setPluginPath
from avogadro-app will set five search directories instead of one (or one,
if we run from build tree). So we (and any other application using libavogadro)
will not be bound to fixed plugin directory structure anymore

AVOGADRO_PLUGINS will be interpreted in old way for compatibility, but
it’s usage should be discouraged anyway

Is it all OK?

http://review.avogadro.openmolecules.net:8080/#change,291

–
Regards,
Konstantin

2

In change 291 I’ve added new API functions setPluginPath and pluginPath.
I think it would be reasonable to split directories ‘colors’, ‘engines’,
‘extensions’, ‘tools’, and ‘contrib’ into separate elements of plugin path,
so

This seems strange to me. I don’t know of any other program that does this. Everything I can think of (Eclipse, GIMP, etc.) all have subdirectories for particular types of plugins.

Do you have a particular use case that you can explain why the new approach would be better?

Thanks,
-Geoff

3

22.11.2010, 04:38, “Geoffrey Hutchison” geoff.hutchison@gmail.com:

In change 291 I’ve added new API functions setPluginPath and pluginPath.
I think it would be reasonable to split directories ‘colors’, ‘engines’,
‘extensions’, ‘tools’, and ‘contrib’ into separate elements of plugin path,
so

This seems strange to me. I don’t know of any other program that does this. Everything I can think of (Eclipse, GIMP, etc.) all have subdirectories for particular types of plugins.

But we don’t really distinguish which kinds of plugins are loaded from certain subdirectory
(however, it could be implemented to improve security)

BTW, I don’t propose to move all plugins into one directory, I just propose to simplify change logic
of plugin loading

Do you have a particular use case that you can explain why the new approach would be better?

  1. Opera-like configuration of plugin paths from UI. User adds directory from which he/she wants
    to load plugins, and they are loaded. E.g, all user-developed plugins could leave in one “flat” directory

  2. Crutchy hack for running from the build tree - thing could be done more logically

  3. One libavogadro-based application which I’ll probably write someday will have 1-2 tools which
    are not useful for Avogadro at all => no need for extensions/ and other empty subdirs in my app

–
Regards,
Konstantin

4

But we don’t really distinguish which kinds of plugins are loaded from certain subdirectory
(however, it could be implemented to improve security)

BTW, I don’t propose to move all plugins into one directory, I just propose to simplify change logic
of plugin loading

Instead, why don’t you do the following. Keep the current code, but:

  • Check directory itself (which should solve your use-case)
  • Check for subdirectories “colors/” “engines/” etc.

This way, you can point at a directory – if it has no organization, everything is loaded. If there is organization (like currently) it will work too.

I don’t see much benefit for making multiple function calls.

That’s my $0.02,
-Geoff

5

22.11.2010, 16:12, “Geoffrey Hutchison” geoff.hutchison@gmail.com:

But we don’t really distinguish which kinds of plugins are loaded from certain subdirectory
(however, it could be implemented to improve security)

BTW, I don’t propose to move all plugins into one directory, I just propose to simplify change logic
of plugin loading

Instead, why don’t you do the following. Keep the current code, but:

  • Check directory itself (which should solve your use-case)
  • Check for subdirectories “colors/” “engines/” etc.

This way, you can point at a directory – if it has no organization, everything is loaded. If there is organization (like currently) it will work too.

Thanks for good idea! And what should be done if organization is different? Recursive loading?

–
Regards,
Konstantin

6

On Nov 22, 2010, at 8:17 AM, Konstantin Tokarev wrote:

Thanks for good idea! And what should be done if organization is different? Recursive loading?

No, I think the code should be something like this:

// Load the plugins
 foreach (const QString& path, d->searchDirs) {
       qDebug() << "Loading plugins:" << path;
       loadPluginDir(path, settings);
       loadPluginDir(path + "/colors", settings);
       loadPluginDir(path + "/engines", settings);
       loadPluginDir(path + "/extensions", settings);
       loadPluginDir(path + "/tools", settings);
       loadPluginDir(path + "/contrib", settings);
 }

This way, the directory itself is searched, not (just) the subdirectories.

Cheers,
-Geoff

7

On Nov 22, 2010, at 8:34 AM, Geoffrey Hutchison wrote:

On Nov 22, 2010, at 8:17 AM, Konstantin Tokarev wrote:

Thanks for good idea! And what should be done if organization is different? Recursive loading?

No, I think the code should be something like this:

// Load the plugins
foreach (const QString& path, d->searchDirs) {
qDebug() << “Loading plugins:” << path;
loadPluginDir(path, settings);
loadPluginDir(path + “/colors”, settings);
loadPluginDir(path + “/engines”, settings);
loadPluginDir(path + “/extensions”, settings);
loadPluginDir(path + “/tools”, settings);
loadPluginDir(path + “/contrib”, settings);
}

This way, the directory itself is searched, not (just) the subdirectories.

I was just about to suggest this, and I think it is the best approach. It would work well for Kalzium too, without forcing overly verbose calls.

Marcus