amantia added a comment.
I've added some comments, I will address the ones I did not comment in a follow-up patch. INLINE COMMENTS > broulik wrote in main.xml:40 > What's the purpose of `hidden=true`? AFAIK it is not used in the GUI config. See also the others in the same file. > broulik wrote in foldermodel.cpp:1310 > Why not store a `QUrl`? Mostly due to the hash and also because for the configuration it anyway needs to be turned into a string (and it is the same as the Positioner does). > broulik wrote in foldermodel.cpp:1314 > This can never be reached It can be and it is reached. m_screen != -1 means it is a containment, screen == -1 means there is no screen information stored yet. This happens e.g when using the updated code with an existing plasma-*appletsrc file. > broulik wrote in foldermodel.cpp:1659 > Use `qobject_cast` (but this would not be neccessary, see my comment on the > `Q_PROPERTY` above) This is not a cast between library boundaries, but I don't mind to use qobject_cast either. > broulik wrote in folderplugin.cpp:66 > Using this is asking for trouble. Plasma uses shared engines for all applets, > so this context property would be accessible to anyone. I would prefer that > you used a singleton type. I was thinking about using a singleton, this pushed me towards it. :) > broulik wrote in screenmapper.cpp:51 > Range-for? I need both the key and the value and AFAIK it is not supported in current Qt version. > broulik wrote in screenmapper.h:21 > You can use `#pragma once` Not common in KDE code though (at least wasn't earlier when I was more involved and can't find anything like that in plasma-desktop) REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D8493 To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid Cc: broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
