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
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel