D8493: Make Folder View screen aware

[Kai Uwe Broulik]

broulik added a comment.


  Regardless of whether this is the right way to go, I put some coding style and practise comments on the code, so you know what to look for in the future / in general :)

INLINE COMMENTS

> main.xml:40
>  
> +    <entry name="screenMapping" type="StringList" hidden="true">
> +        <default></default>

What's the purpose of `hidden=true`?

> foldermodel.cpp:1310
> +
> +    const QString name = item.url().toString();
> +    if (m_usedByContainment) {

Why not store a `QUrl`?

> foldermodel.cpp:1314
> +        if (m_screen != -1) {
> +            if (screen == -1)
> +                m_screenMapper->addMapping(name, m_screen);

This can never be reached

> foldermodel.cpp:1316
> +                m_screenMapper->addMapping(name, m_screen);
> +            else  if (m_screen != screen)
> +                return false;

Coding style: one space between `else` and `if`, also put braces even around one line statements.

> foldermodel.cpp:1659
> +{
> +    const auto mapper = dynamic_cast<ScreenMapper*>(screenMapper);
> +    if (!mapper || m_screenMapper == mapper)

Use `qobject_cast` (but this would not be neccessary, see my comment on the `Q_PROPERTY` above)

> foldermodel.h:100
> +    Q_PROPERTY(int screen READ screen WRITE setScreen NOTIFY screenChanged)
> +    Q_PROPERTY(QObject* screenMapper READ screenMapper WRITE setScreenMapper NOTIFY screenMapperChanged)
>  

You can use `ScreenMapper *` as property type if you did `qmlRegisterType<ScreenMapper>()` on it.

Also note that QML engine sucks at resolving namespaces, so you might need to fully qualify it should it be in a namespace.

> foldermodel.h:190
> +
> +        QObject* screenMapper() const;
> +        void setScreenMapper(QObject* screenMapper);

Coding style: space //before// `*`, ie `QObject *foo()`

> folderplugin.cpp:66
>  
> +void FolderPlugin::initializeEngine(QQmlEngine *engine, const char *uri)
> +{

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.

> screenmapper.cpp:50
> +{
> +    QStringList result;
> +    auto it = m_screenUrlMap.constBegin();

Add `reserve()` call, ie `result.reserve(screenUrlMap.count() * 2)`

> screenmapper.cpp:51
> +    QStringList result;
> +    auto it = m_screenUrlMap.constBegin();
> +    while (it != m_screenUrlMap.constEnd()) {

Range-for?

> screenmapper.cpp:79
> +{
> +    if (m_screenUrlMap.contains(url)) {
> +        return m_screenUrlMap[url];

Avoid double look-up:

  return m_screenUrlMap.value(url, -1);

the second argument is the default value to return if the item does not exist.

> screenmapper.h:21
> + ***************************************************************************/
> +#ifndef SCREENMAPPER_H
> +#define SCREENMAPPER_H

You can use `#pragma once`

> screenmapper.h:34
> +    explicit ScreenMapper(QObject *parent = nullptr);
> +
> +    QStringList screenMapping() const;

We typically explicitly add a default destructor.

  ~ScreenMapper() override = default;

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/bd2f1df8/attachment-0001.html>