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
[email protected]
https://mail.kde.org/mailman/listinfo/plasma-devel
