D8493: Make Folder View screen aware

[Milian Wolff]

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.mantia at 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.mantia at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171101/9dcfe4f0/attachment-0001.html>