[Differential] [Requested Changes To] D1722: Import next-gen libtaskmanager.

[graesslin (Martin Gräßlin)]

graesslin requested changes to this revision.
graesslin added a reviewer: graesslin.
graesslin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> waylandtasksmodel.cpp:115-119
> +    KService::Ptr service = KService::serviceByStorageId(window->appId());
> +
> +    if (service) {
> +        serviceCache.insert(window, service);
> +    }

at the point of adding a window you can be quite sure that the appId is not yet pushed. Maybe we should introduce a dedicated event in the protocol to signal that the initial known data is pushed to the client. That's e.g. how Output does it.

Which could be nice to ensure that the task model doesn't start adding any elements before it knows what to do with it. Also for the case that the window is already unmapped.

> waylandtasksmodel.cpp:233
> +{
> +    QModelIndex idx = q->index(windows.indexOf(window));
> +    emit q->dataChanged(idx, idx, QVector<int>{role});

shouldn't you check whether the index is valid?

> waylandtasksmodel.cpp:244-246
> +WaylandTasksModel::~WaylandTasksModel()
> +{
> +}

WaylandTasksModel::~WaylandTasksModel() = default;

> waylandtasksmodel.cpp:256
> +
> +    if (role == Qt::DisplayRole) {
> +        return window->title();

any specific reason why you use an if-else structure instead of a switch?

> waylandtasksmodel.cpp:359
> +
> +        // FIXME Timestamp on Wayland?
> +        new KRun(QUrl::fromLocalFile(service->entryPath()), 0, false);

there is no global timestamp like on X11 on Wayland. Timestamps are only used for frame rendered callbacks and on input events. Both are defined to have an undefined base and there is nowhere specified that they have the same base.

> waylandtasksmodel.cpp:490
> +
> +    using namespace KWayland::Client;
> +    Surface *surface = Surface::fromWindow(itemWindow);

why don't you use the namespace more globally?

> xwindowtasksmodel.cpp:115
> +{
> +    rulesConfig = new KConfig(QStringLiteral("taskmanagerrulesrc"));
> +    configWatcher = new KDirWatch(q);

why not KSharedConfig::openConfig?

> xwindowtasksmodel.cpp:118
> +
> +    foreach(const QString &location, QStandardPaths::standardLocations(QStandardPaths::ConfigLocation)) {
> +        configWatcher->addFile(location + QLatin1String("/taskmanagerrulesrc"));

coding style nitpick: missing space between foreach and (

> xwindowtasksmodel.cpp:168
> +
> +    void (KWindowSystem::*myWindowChangeSignal)(WId window, const unsigned long *dirty) = &KWindowSystem::windowChanged;
> +    QObject::connect(KWindowSystem::self(), myWindowChangeSignal, q,

you are connecting to a deprecated signal. see https://api.kde.org/frameworks/kwindowsystem/html/classKWindowSystem.html#ac8d368d83fa5e137f38e7c885d9a18ce

> xwindowtasksmodel.cpp:198
> +    // Add existing windows.
> +    foreach(const WId window, KWindowSystem::windows()) {
> +        addWindow(window);

coding style nitpick: missing whitespace

> xwindowtasksmodel.cpp:419
> +        return info;
> +    } else if (!windowInfoCache.value(window)->valid()) {
> +        delete windowInfoCache.take(window);

I don't understand this valid check. Why are you inserting non valid KWindowInfo into the cache in the first place?

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: hein, #plasma, graesslin
Cc: graesslin, broulik, davidedmundson, plasma-devel, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160603/79531568/attachment-0001.html>