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

[hein (Eike Hein)]

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

INLINE COMMENTS

> graesslin wrote in CMakeLists.txt:18-23
> do we want to have X11 conditional? Other areas of Plasma use X11 unconditionally.

Personally as maintainer I do :). From my POV X11 is a legacy windowing system for libtaskmanager and I want to support building without it. If nothing else being disciplined about that helps a bit to prevent X11 stuff from creeping over the codebase again ...

> graesslin wrote in CMakeLists.txt:51
> Qt5::X11Extras

Done.

> davidedmundson wrote in tasksmodel.cpp:134
> you need a 
>  --d->activityInfoUsers
> 
> otherwise:
> 
> - set sort mode activities
> - delete task bar
> - add a task bar
> - set sort mode activities
> - set sort mode something else
> 
> you leak an activityInfo object.

Done.

> davidedmundson wrote in tasksmodel.cpp:478
> invalidateFilter()?

invalidateFilter() doesn't do a full resort - it only sorts in rows that may now get past the filter, leaving already-sorted rows alone. Unfortunately this was the best way I found to get QSortFilterProxyModel to guaranteed sort all rows.

> davidedmundson wrote in tasksmodel.cpp:1220
> would this be clearer and faster if it connected to groupsModel::rowsInserted ?

Quite possibly, I'll give it a try. I originally had everything in filterAcceptsRow() and the code then evolved more, and this was kind of left on the table along the way I think.

> davidedmundson wrote in tasksmodel.cpp:84
> const

Done.

> davidedmundson wrote in xwindowtasksmodel.cpp:307
> We're missing some changed roles
> 
> IsKeepAbove
> IsKeepBelow
> SkipTaskbar

Done.

> davidedmundson wrote in xwindowtasksmodel.cpp:313
> IsVirtualDesktopChangeable

Done.

> davidedmundson wrote in xwindowtasksmodel.cpp:434
> we don't want to open and parse a config file on every new window.
> It'll be worth keeping the kconfig object.

I've added a FIXME to optimize this at a later time. It's not worse than the old lib (it did this too).

> davidedmundson wrote in xwindowtasksmodel.cpp:756
> you can do all this in the QByteArray ctor

Done.

> davidedmundson wrote in xwindowtasksmodel.cpp:861
> what's with the Q_INVOKABLE?

Copy and paste mistake probably, fixed.

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

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

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