D8493: Make Folder View screen aware

[Andras Mantia]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171026/f4e2bbcf/attachment.html>