Review request: kdelirc

[Michael Zanetti]

On Tuesday 21 April 2009 12:03:17 David Faure wrote:
>
> The READMEs still mention DCOP.

fixed.

>
> There is funny syntax in the Modes class (which inherits from qmap) :
>     Mode retMode = operator[](remote)[mode];
> Why not value(remote)[mode]? You could use value() to save a lot of
> contains() checks, too (currently, the lookup is done twice, contains and
> then []; in non-const methods where contains isn't checked, like add() and
> erase(), operator[] even risks doing unwanted insertions)
>
> Modes::generateNulls says:
>         if (!contains(*i) || !operator[](*i).contains(""))
> operator[](*i)[""] = Mode(*i, ""); if (!theDefaults.contains(*i))
> theDefaults[*i].isEmpty();
> The first line could use some simplificiations, but the second line is
> even wierder: it doesn't do anything? (it calls the isEmpty() method and
> discards the result, well and it creates an entry in theDefaults, as a side
> effect).
> Was this meant to be theDefaults.insert(*it, "") ?
> (well or even QString() instead of "", everywhere).

All places where you can find such funny syntax (I'd rather call it PITA 
syntax) is still the original code of KDE3's kdelirc. We have refactored most 
parts of if but have not reached 100% yet. For example look into EditAction or 
AddAction. There you will find our new code. 

For the next step we are looking to rewrite also the IRAction, Modes etc. But 
we didn't have the time to complete it for 4.3. So we decided to take a break, 
make it as stable as possibe, ship it to the users and continue refactoring.

Currently I think it is already way more stable as it ever was in KDE3. But I 
know that there are still such ugly parts left.

>
> The #include <KApplication> in arguments.h is unneeded.
>

I have added that because else I get compile errors about KDE_EXPORT beeing 
unrecognized. What is the correct way to include it if there are no other k-
includes needed?

> ProfileServer::getAction:
>         if (getProfileById(appId)->actions()[actionId]) {
>             return getProfileById(appId)->actions()[actionId];
> Use a local variable please, to do all this stuff only once.
>

Fixed but: As I've said, this part will be replaced in the future.

> Prototype::argumentCount() and getArguments() [the "get" isn't needed]
> should be const.
>

Fixed, same as above...

> RemoteServer::theInstance is never deleted (would be useful for memory
> leak checking), use K_GLOBAL_STATIC.

fixed.

Cheers,
Michael