[Differential] [Updated] D1724: Import Task Manager widgets ports to the new library.

[hein (Eike Hein)]

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

INLINE COMMENTS

> broulik wrote in CMakeLists.txt:7
> Why is icon tasks now in plasma-desktop instead of kdeplasma-addons?

It's not a case of "now": Icontasks has been in plasma-desktop for a long time now. Please keep in mind code reviews are not plasma-devel ...

> broulik wrote in CMakeLists.txt:1
> This stays "ng"?

No, that's a bug from my rename orgy :) Thanks!

> broulik wrote in tools.js:44
> var taskIndexList = [];

Really? :P Ok ...

> broulik wrote in ContextMenu.qml:216
> I suppose this is also emitted when I press return on the keyboard?

Yup!

> broulik wrote in ContextMenu.qml:313
> I think this should be a checkable item "[X] Allow this program to be grouped"

This is the way it was before -- I don't mind changing it I guess ...

> broulik wrote in ContextMenu.qml:324
> I think you should also follow visible, so when kiosk restrictions disallow configuring applets the entry is hidden altogether (like in the rest of the shell)

"follow visible"?

> broulik wrote in GroupDialog.qml:108
> Where does that 6 come from?

Not sure - this is code that goes back to 4.11. It's not new in this review.

> broulik wrote in MouseHandler.qml:85
> That's the default

Being explicit is nice and aids readability.

> broulik wrote in Task.qml:233
> Can't you just bind prefix: TaskTools.taskPrefix(basePrefix)?

IIRC Marco wrote this code, it's unrelated to the backend change anyway.

> broulik wrote in TaskList.qml:28
> Doesn't that assumption break when you have launchers, ie. the first one being a tiny square instead of a wide task?

Yes it does, that's why I asked Marco - who wrote that code - to mark up the related stuff in main.qml with big bad FIXMEs. Also unrelated to what this review is about.

> broulik wrote in ToolTipDelegate.qml:352
> TextMetrics {
> 
>   font: tooltipMaintextPlaceholder.font
>   text: mainText
> 
> } ?

Unrelated to this review. Please file a patch later.

> broulik wrote in backend.cpp:114
> check KDesktopFile::isDesktopFile(...) like in the recentDocumentsActions

Aye.

> broulik wrote in backend.cpp:118
> Other than being incremented, this variable is not used

You realize you are reviewing your own code now, right ...? I'll remove it, it's the same in the old code though (since it's unchanged).

> broulik wrote in backend.cpp:162
> QLatin1String

Ok.

> broulik wrote in backend.cpp:179-180
> init in one line?

Ok ...

> broulik wrote in backend.cpp:298
> mapToScene? (QQuickItem 5.7 will have a mapToGlobal btw)

Am I supposed to guess what you mean here? Please elaborate.

> broulik wrote in backend.cpp:362
> Do we need an isDesktopFile check here too?

I don't think so -- hasApplicationType() should return false.

> broulik wrote in backend.h:52
> Q_ENUM here instead of Q_ENUMS above

Ok.

> broulik wrote in backend.h:57
> coding style: QQuickItem *foo, also below

Ok.

> broulik wrote in backend.h:77
> coding style: const QUrl &url

Ok.

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

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

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

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