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
