Re: [KMess-devel] Suggestions for improvement on the Gitorious merge request

[Sjors G. <da...@da...>]

Hi Daniel,

I've looked through your improvements and they're looking good! A few things drew my attention while looking through the code:

1. The TODO in the Plugin constructor:
#warning TODO: implement this function when a user adds plugins.
2. The TODO in PluginsPage::loadSettings(Account*) and ::saveSettings(Account*):
#warning TODO: implement this function.
3. The QScriptEngine inclusion (and forward declaration) in pluginsmaster.{cpp,h}.
4. Are you sure you didn't change pluginsdelegate (by Joris Guisson)? If so, disregard this one. If not: update the file, just like you updated PluginsModel :)

I applied the patch locally (not a 100% clean apply, some whitespace you fixed was already fixed, but that's no problem). After that, I had a compilation error in PluginsPage and PluginsMaster. Both of the files included ui_plugininfo.h which is generated from plugininfo.ui which doesn't exist anymore. Maybe you still have that .ui file on your system, creating the ui_plugininfo.h so you don't get a build error? Anyway: if I remove the inclusion and comment out the "private Ui::PluginInfo" in pluginsmaster.h, the code compiles wonderfully.

Except for one warning:
> master/src/plugins/plugin.h: In constructor ‘Plugin::Plugin(const QString&, QObject*)’:
> master/src/plugins/plugin.h:70: warning: ‘Plugin::file_’ will be initialized after
> master/src/plugins/plugin.h:66: warning:   ‘Kross::Action* Plugin::action_’
> master/src/plugins/plugin.cpp:46: warning:   when initialized here

I'll fix that one, but remember to fix your warnings before you send in a patch ;-)

KMess works beautifully with the patch applied, as does the plugin setting screen (nice that you've made it per-account!). However, there's a user-interface bug. I don't have python-kross installed, so probably that's why my plugins won't activate: if I tick a checkbox, it instantly un-ticks without any warning, GUI or terminal. You should definitely display a warning here. (If you want, some information that doesn't look like it should be in GUI, can be outputted via QDebug.)

Anyway, python-kross is not in my repositories, so it'll take me some time to compile and install it. After that, I will test the plugins but I expected you already tested them extensively. We will have to add CMake checker scripts to the CMakeLists.txt to warn people in advance that they have software missing to get everything out of KMess.

With regard to your question earlier;

Op 6 jan 2011, om 12:03 heeft Daniel E. Moctezuma het volgende geschreven:

> 18. It's normal to say "foo *bar" instead of "foo* bar". I'll explain why if you want.
> As far as I know the result would be the same using one or another, I suppose you say it because of a coding standard, maybe? If not, I would appreciate if you can explain please.

Yes, this is a coding standard / style thing. The reason it's the normal convention is because of this case:
> int* a, b;
In this case, 'a' is an int-pointer, and b is simply an int. The coding style makes this a little more clear:
> int *a, b;
And if you want two pointers, you consistently write:
> int *a, *b;

Anyway: I've pushed your patch, I'm fixing most of the things above quickly (as you won't learn anything from fixing them yourself anymore). There's a few things I would like to get from you sometime soon:
- an implementation of Plugin::Plugin and PluginsPage::{load,save}Settings,
- a patch that makes KMess display a message box if enabling a plugin didn't work, including as much reason as you can show,
- if you know how to do it, or are willing to find out: a bit of CMake code to check whether python-kross and/or ruby-kross are available (and/or working, if possible).

Either way: huge thanks for this patch, and for working with me all this time :) you can now print <http://www.gitorious.org/kmess/kmess/commit/1355ba446f8406094dbd89a414b12a1633fc85a3>, frame it, and hang it  somewhere where you'll see it and be proud a few times every day ;-)

Thanks again,
Sjors