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
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
