[Differential] [Commented On] D2345: use a separate configuration per look and feel

[ivan (Ivan Čukić)]

ivan added a comment.


  All in all, seems ok.
  
  I think we need to start commenting code like this - what it is doing. I do realize it is popular for the code to be self-documenting, but that has not been working for us that well :)
  
  Can you write short instructions on how to test this?

INLINE COMMENTS

> containment.cpp:186
>      Plasma::Applet *applet = 0;
> +    int id = -1;
> +    if (context->argumentCount() > 1 && context->argument(1).isNumber()) {

It would be nice if we started putting comments with reasoning what this should do and similar in front of the blocks like these - it will be problematic to understand this code to future contributors

> shellcorona.cpp:136
>      connect(this, &Plasma::Corona::startupCompleted, this,
> -            []() {
> +            [this]() {
>                  qDebug() << "Plasma Shell startup completed";

Why is `this` captured?

> shellcorona.cpp:325-326
> +    QStringList hierarchy;
> +    QList<KConfigGroup> groups;
> +    groups << rootGroup;
> +    QSet<QString> visitedNodes;

QList<KConfigGroup> groups{rootGroup};

> shellcorona.cpp:373
> +
> +QString ShellCorona::dumpCurrentLayoutJS()
> +{

I have a bad feeling about this. When will the generated JS code be used?

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D2345

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: mart, #plasma
Cc: ivan, plasma-devel, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160803/9b4788c9/attachment.html>