KRemoteControl moved to kdereview

[Friedrich W. H. Kossebau]

Hi Michael,

Vendredi, le 12 mars 2010, à 18:54, Michael Zanetti a écrit:
> Hi all,
> 
> I have moved KRemoteControl to kdereview. It is a branch of kdelirc with
[...]
> If there are no objections I would move it to trunk/kdeutils after the two
> week review period.

Huh, a lot of code, I fear I could only do a superficial review, as I missed 
the big picture a little. But it looked well done in principle ;)
Could not do much ui testing, as I also miss an Irda device (well, working 
drivers for the existing hardware).

Sometimes you name the files after the namespace, sometimes after (one of) the 
classes, this is a little irritating if one browses the sources by the 
filesystem.

So all there is from me are nitpicks. All minor improvements (and I left some 
out, like QLatin1String wrappers for faster string comparisons and similar), 
found nothing grave in my eyes.

signals/slots: Better use Q_SIGNAL/Q_SLOTS, grep for it :)

README looks a little outdated (using name kdelirc etc.).

The daemon (D-Bus) interface has "get*" getters, e.g. getConfiguredRemotes, 
getModesForRemote. Is this D-Bus style, or could this be changed to the Qt/KDE 
style with no "get" prefix for getters?

kded/kremotecontroldaemon.h:
Q_DECLARE_PRIVATE(KRemoteControlDaemon):
No need to do a pimpl here, no BC needed.

kded/kremotecontroldaemon.cpp
return ""; // use QString();

krcdnotifieritem/main.cpp:
Hm, a separate program/process just for the notifier? Could this notifier not 
also be a plugin to some hardware-notifier shell? Is there some (Plasma-less) 
so it also works in other workspaces (write once, run everywhere)?

libkremotecontrol/remotelist.h:
Remote* getRemote() -> remote() ?
could be a static function/constructor (RemoteList::fromConfig()):
        void loadFromConfig(const QString &configName);
And the class rather a QVector subclass than a QList one, you store only 
pointers and won't do a lot resizing.

libkremotecontrol/prototype.cpp:
do
Prototype(...) : m_name(name), m_args( args )
instead of 
m_name = name;
m_args = args
saves useless default constructor calls to members.


libkremotecontrol/profileserver.cpp:
You could set ProfileXmlContentHandler as friend class to 
ProfileActionTemplate, then create a ProfileActionTemplate instance and fill 
the data member by member, as parsed.
ProfileActionTemplate is also a little bit too large to be passed by value 
(see also Profile class), you might want to think about using QSharedData as 
private class container for it.

libkremotecontrol/mode.h:
m_actionList could be a QVector, too.

libkremotecontrol/executionengine.cpp:
ExecutionEnginePrivate() could just do a
DBusActionExecutor* defaultExecutor = new DBusActionExecutor();
executors.insert(Action::DBusAction, defaultExecutor);
executors.insert(Action::ProfileAction, defaultExecutor);

libkremotecontrol/dbusinterface.h:
"get*" getters, cmp. kremotecontroldaemon above.
This class looks strange to me, but you may have your reason to do all the 
hand-crafted D-Bus calls.

dataengine/kremotecontrolengine.cpp:
just call DBusInterface::getInstance() once and store the instance in a local 
var

kcmremotecontrol/kcmremotecontrol.cpp:
word puzzle, no i18n:
            ui.lActions->setText(remote->name() + " in mode \"" + mode->name() 
+ "\"");

kcmremotecontrol/modedialog.cpp:
QString()?
   ui.cbButtons->addItem(i18n("No button"), "");
    addItem(i18n("No Button"), "");
And why not just the default QVariant()?

kcmremotecontrol/model.cpp:
could use a i18nc:
  i18n("Application / Node")

The other way round, first comment, than content :)
            comboBox->addItem(i18nc("True", "Value is true"));
            comboBox->addItem(i18nc("False", "Value is false"));

word puzzle, use string with placeholders i18nc("Author: %1 (%2) ..."):
        tooltip.append(i18nc("Profile author", "Author")).append(": ");
        tooltip.append(profile->author()).append(" (").append(i18nc( "Profile 
Version", "Version")).append(": ").append(profile->version()).append(")");
strings could be shorter, use tooltip for more details:
	ActionTemplateModel::headerData()
try to call qRegisterMetaType in some more global place, not every time the 
constructor of RemoteItem is called:
RemoteItem::RemoteItem(Remote *remote) {
    qRegisterMetaType<Remote*>("Remote*");
    qRegisterMetaType<Mode*>("Mode*");


So, besides the word puzzles I did not see any large problem. But this may be 
due to the size and that I could not test the functionality. So I just have to 
trust the authors and hope it works, unless someone else can give it some 
stress-testing and report the results :)

The kcm UI looks clean (and grey ;) ), but not much consistent with other UIs 
in the systemsettings :( We definitely need some style guide for UIs with 
lists (best enforced by default widget classes).

So, besides the word puzzles green light from the kdeutils coordinator. Unless 
bad reports from function testers arrive, of course. Good reports even more 
welcome. Anybody done some? Please do, if you can.

Cheers
Friedrich
-- 
KDE Okteta - a simple hex editor - http://utils.kde.org/projects/okteta