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

[Sjors G. <sj...@km...>]

Op 28 dec 2010, om 23:45 heeft Sjors Gielen het volgende geschreven:

>> Also I'm not sure if this is a problem with my distro or so, but it seems that all the plugins are enabled by default when you get to the enable/disable window (they are configured to be disabled by default, though), I'll appreciate if somebody can confirm this.
> 
> I'll read through the patch tomorrow as soon as I can - if it looks OK I'll apply it locally and check if I have that behaviour too. Will let you know.

Hi Daniel,

And read through the patch I did. There's a few things I want to say, first, but most of them are merely warnings: you can fix most of them in a matter of seconds. If you could spend some time on this (it's an important part of delivering a good patch) I'll merge it right after!

1. I noticed in your `diff` commandline that you --exclude=lib. Are you sure you didn't change anything in the library?
2. My compliments with using the .desktop file! Using a standard for that is really nice.
3. You have this change in your CMakeLists.txt:
-TARGET_LINK_LIBRARIES( ${PROJECT_NAME} ${kmess_LIBS} )
+TARGET_LINK_LIBRARIES( ${PROJECT_NAME} ${kmess_LIBS} ${KDE4_KROSSCORE_LIBS} ${KDE4_KROSSUI_LIBS} ${X11_LIBRARIES} )
You should add the Kross libraries to ${kmess_LIBS} like the rest of the libraries. Also, adding ${X11_LIBRARIES} here is really not right: if KrossUI needs it it'll be in KROSSUI_LIBS and you can't assume KMess will eventually be using X11. For example, Mac builds don't use X11 at all.
4. Great job on the IRC-like commands plugin! Removing that from the core code and adding it again as a plugin is a huge plus for your patch.
5. What is ChatWindow::text_ for? It's only assigned and emited once, but not used anywhere.
6. Adding KMESSDEBUG definitions to kmessdebug.h that aren't relevant to the plugin system or your patch, is added noise, and you should probably send them in as a seperate patch.
7. TODO in your code on line 54 of kmessaccount.h. You should look at that, or remove it.
8. What's the comment for on line 293 of kmesschat.cpp?
9. Why is messageRecieved (which is wrongly spelled) commented out on line 75 of kmesschat.h?
10. Why is clearMessageEditor() commented out on line 82 of kmesscontactlist.h?
11. KMessInfo line 75: getKmessName() is not really a slot: it's const so it can't 'do' anything, there's no reason to use it as a true slot. This is true for Session too, where most of your getters are slots for no reason. I personally think it's "clean" to make them pure functions, but it doesn't matter in functionality.
12. Line 207 of plugin.cpp: why "interpreter", why not .interpreternameForFile() ? Why detect by filename later? This will break if Kross support changes over time, which it undoubtedly will. I.e. if Perl is added, KMess can never understand ".pl", but Kross will...
13. Why are only KPluginInfo and bool in that struct MetaInfo? It's never used by itself, so it could very well be directly private to the object too. (It's OK, just interested)
14. The PluginsMaster constructor contains an old TODO and commented out Account code, what about that? (The destructor contains stuff too)
15. You left QScriptValue PluginsMaster::evaluateScript() in, but that won't work anymore, will it? (Any other stuff left in that should be removed? QScriptEngine *engine_?)
16. getPluginList() contains old commented out code that can be removed. You might want to make the return value of that method a reference (const QList<>&) so it doesn't need to be copied. Then, getPluginsCount() may be removed too.
17. What's this for? line 443 of pluginsmaster.cpp
+  enableRunPlugin( selected.count() > 0 && num_not_running > 0 );
+  enableStopPlugin( selected.count() > 0 && num_running > 0 );
18. It's normal to say "foo *bar" instead of "foo* bar". I'll explain why if you want.
19. The windowIcon of plugininfo.ui is wrong:
+    <normaloff>../../../../../usr/share/icons/oxygen/16x16/actions/help-about.png</normaloff
>../../../../../usr/share/icons/oxygen/16x16/actions/help-about.png</iconset>
20. Who's Joris Guisson? pluginsdelegate.h line 3:
+ * (C) Copyright 2009 Joris Guisson <jor...@gm...>              *
And Ivan Vasic?
+ * (C) Copyright 2008 Ivan Vasic <iv...@gm...>                        *
Note that if you used their code, you have to make a 1:1 copy of their copyright notice, even if that means there are three copyright notices. You can't only copy their names and the year, iirc.
21. Some code in PluginsModel is the same as the code in PluginsMaster. Is that really needed? You can use PluginsMaster::addPluginFromDesktopFile() too, right?
22. Why don't PluginsPage::loadSettings() and PluginsPage::saveSettings() do anything? (Q_UNUSED() there only hides that it doesn't do anything, you should leave that out if it's a todo or add a #warning!)

Let me know what you think about most of these! Hopefully we can merge the patch ASAP :-)

Sjors