mwolff added a comment.
some code review from my side
INLINE COMMENTS
> foldermodel.cpp:77
> #include <unistd.h>
> +#include <KF5/Plasma/Applet>
> +#include <KF5/Plasma/Containment>
wrong location of include, and the KF5 prefix is wrong too, I guess?
> foldermodel.cpp:1314
> +
> + const QString name = item.url().toString();
> + if (m_usedByContainment) {
move into conditional below, it's only used there
> foldermodel.cpp:1316
> + if (m_usedByContainment) {
> + const int screen = m_screenMapper->screenForUrl(name);
> + if (m_screen != -1) {
adding some comment here would help people reading the code in the future. I.e.
something like "exclude items that are shown on a different screen"
also, I personally think this code is not pretty at all. When no mapping is
found, the first model that encounters the item in this `filterAcceptsRow` will
take up the mapping. But... Isn't that too random? Shouldn't the mapping be
done elsewhere? Why can there be items encountered here that have no mapping?
At the very least, can you add a comment here to explain why "first comes,
first served" is the right approach?
> foldermodel.cpp:1689
> + Plasma::Applet *applet =
> appletInterface->property("_plasma_applet").value<Plasma::Applet *>();
> + Plasma::Containment *containment = applet->containment();
> +
if the above fails for any reason, this will crash. Shouldn't you check the
value first?
> foldermodel.cpp:1691
> +
> + if (containment) {
> + Plasma::Corona *corona = containment->corona();
shorten this to:
if (containment && containment->corona())
m_screenMapper->registerCorona(containment->corona());
? Or is asking for the corona too expensive?
> foldermodel.h:191
> +
> + ScreenMapper* screenMapper() const;
> + void setScreenMapper(ScreenMapper* screenMapper);
unused, remove? also, the mapper is now a singleton? so why have it as a member
here, too?
> screenmapper.cpp:5
> + * Author: Andras Mantia <andras.man...@kdab.com> *
> + * Work sponsored by the LiMux project of the city of Munich. *
> *
> + * This program is free software; you can redistribute it and/or modify *
dito, broken *
> screenmapper.cpp:30
> +
> +ScreenMapper *ScreenMapper::instance() {
> + if (s_instance)
{ on its own line
> screenmapper.cpp:92
> +
> +void ScreenMapper::registerCorona(Plasma::Corona *corona)
> +{
`register` sounds like this can be called multiple times and you'll remember
all registered coronas. But it seems like this is actually a `setCorona`, no?
Only the last called corona will be used after all
> screenmapper.cpp:96
> + m_corona = corona;
> + m_availableScreens.clear();
> + if (m_corona) {
shouldn't you reset m_firstScreen and potentially the other maps, too?
> screenmapper.cpp:121
> + QHash<QString, int> newMap;
> + const int count = mapping.count();
> + for (int i = 0; i < count - 1; i += 2) {
count -> size
newMap.reserve(size / 2);
> screenmapper.cpp:123
> + for (int i = 0; i < count - 1; i += 2) {
> + if ( i + 1 < count) {
> + newMap[mapping[i]] = mapping[i + 1].toInt();
remove space after (
> screenmapper.h:5
> + * Author: Andras Mantia <andras.man...@kdab.com> *
> + * Work sponsored by the LiMux project of the city of Munich. *
> *
> + * *
that * at the very end is off
> screenmapper.h:47
> +
> + int screenForUrl(const QString &url) const;
> + Q_INVOKABLE void addMapping(const QString &url, int screen);
a `screenForUrl` taking a `QString url` just asks for trouble, similar below.
If it's a URL, shouldn't it take `QUrl`?
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D8493
To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson,
apol
Cc: mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
