[Differential] [Updated] D1722: Import next-gen libtaskmanager.

[hein (Eike Hein)]

hein marked 11 inline comments as done.
hein added inline comments.

INLINE COMMENTS

> graesslin wrote in waylandtasksmodel.cpp:115-119
> 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.

I've added a FIXME until we figure that out.

> graesslin wrote in waylandtasksmodel.cpp:233
> shouldn't you check whether the index is valid?

If this check were necessary, something else would be broken somewhere. dataChanged() is called with window instances which are in the list, so indexOf will return something sensible. index() then can't fail because hasIndex() just does a bounds check against rowCount() (which operates on the same list) before calling createIndex() (which doesn't really do anything with data). So the index could only be invalid if dataChanged() were called with a window that's not in Private::windows, which makes no sense.

> graesslin wrote in waylandtasksmodel.cpp:244-246
> WaylandTasksModel::~WaylandTasksModel() = default;

Ok.

> graesslin wrote in waylandtasksmodel.cpp:256
> any specific reason why you use an if-else structure instead of a switch?

Not really.

> graesslin wrote in waylandtasksmodel.cpp:359
> 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.

Removed FIXME.

> graesslin wrote in waylandtasksmodel.cpp:490
> why don't you use the namespace more globally?

No particular reason. Might change as the code evolves.

> graesslin wrote in xwindowtasksmodel.cpp:115
> why not KSharedConfig::openConfig?

Changed.

> graesslin wrote in xwindowtasksmodel.cpp:118
> coding style nitpick: missing space between foreach and (

Ok.

> graesslin wrote in xwindowtasksmodel.cpp:168
> you are connecting to a deprecated signal. see https://api.kde.org/frameworks/kwindowsystem/html/classKWindowSystem.html#ac8d368d83fa5e137f38e7c885d9a18ce

Changed.

> graesslin wrote in xwindowtasksmodel.cpp:198
> coding style nitpick: missing whitespace

Ok.

> graesslin wrote in xwindowtasksmodel.cpp:419
> I don't understand this valid check. Why are you inserting non valid KWindowInfo into the cache in the first place?

Paranoia. I'm not inserting invalid ones into the cache, but I figured if there's a valid() it would be a good idea to call it before using it, so that block checked valid() and evicted if false. I've removed the block now.

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/75ceb2b2/attachment.html>