D8493: Make Folder View screen aware

[Milian Wolff]

mwolff added inline comments.

INLINE COMMENTS

> foldermodeltest.cpp:48
> +
> +void FolderModelTest::initTestCase()
> +{

remove the init and cleanup if both are empty?

> screenmappertest.cpp:36
> +
> +void ScreenMapperTest::cleanupTestCase()
> +{

remove if empty

> screenmappertest.cpp:45
> +
> +void ScreenMapperTest::cleanup()
> +{

dito

> foldermodel.cpp:163
> +    if (m_screenMapper) {
> +        m_screenMapper->disconnect(this);
> +        m_screenMapper->removeScreen(m_screen, url());

can you add a comment why this explicit disconnect is needed? is it b/c removeScreen would otherwise emit a signal that we'd try to handle but don't want to? otherwise the QObject dtor would disconnect us

> foldermodel.h:294
>      private:
> +
>          struct DragImage {

ws only, unneeded change, can be unset

> screenmapper.cpp:41
> +            this, &ScreenMapper::screenMappingChanged);
> +    m_screenMappingChangedTimer->setInterval(100);
> +    m_screenMappingChangedTimer->setSingleShot(true);

this is a pretty arbitrary timer, can you add a comment on why 100ms is better than going through the eventloop once via QMetaObject::invokeMethod with the delayed flag set?

> screenmapper.cpp:52
> +    if (screenUrl.scheme().isEmpty())
> +        screenUrl = QUrl::fromLocalFile(path);
> +    const auto screenPathWithScheme = screenUrl.url();

should this maybe be QUrl::fromUserInput with a prefer local file flag set?

> screenmapper.cpp:69
> +        if (firstScreen == screenId) {
> +            if (m_availableScreens.isEmpty()) {
> +                m_firstScreenForPath[path] = -1;

could be rewritten/simplified:

  const auto it = std::min_element(...);
  m_firstScreenForPath[path] = it == m_availableScreens.constEnd() ? -1 : *it;

> screenmapper.cpp:82
> +        *pathIt = pathIt.value() - 1;
> +    } else if (path.isEmpty()) {
> +        pathIt = m_screensPerPath.begin();

so if `path` is empty, this adjusts all paths? can you add a comment on when this happens and what it's for?

> screenmapper.cpp:98
> +
> +    QUrl screenUrl(path);
> +    if (screenUrl.scheme().isEmpty())

these three lines are shared with above, introduce a helper function in an anon namespace for it: `QUrl screenUrlForPath(const QString &path);`

> screenmapper.cpp:108
> +        for (const auto &name: it.value()) {
> +            const auto url = QUrl(name);
> +            // add the items to the new screen, if they are on a disabled screen and their

`m_itemsOnDisabledScreensMap` stores "names" from the `m_screenItemMap`, so I think this line here is wrong and should also use the proposed `screenUrlForPath` above, or `QUrl::fromUserInput`, no? Otherwise you may end up with relative urls for file paths?

> screenmapper.cpp:124
> +    m_availableScreens.append(screenId);
> +    if (!path.isEmpty()) {
> +        auto it = m_screensPerPath.find(path);

again, seems like an empty path has a special meaning, can you document that somewhere please?

> screenmapper.cpp:140
> +
> +void ScreenMapper::addMapping(const QString &name, int screen, MappingSignalBehavior behavior)
> +{

is `name` also a `path`?

> screenmapper.cpp:150
> +
> +void ScreenMapper::removeFromMap(const QString &name)
> +{

dito

> screenmapper.h:43
> +public:
> +    enum MappingSignalBehavior {
> +        DelayedSignal = 0,

this needs to be documented, i.e. why and when is it required to distinguish between these signal behaviors?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D8493

To: amantia, #plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol, mwolff
Cc: anthonyfieroni, ngraham, 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/20171115/85627da7/attachment-0001.html>