Details
- Reviewers
davidedmundson - Group Reviewers
Plasma - Commits
- R119:3d5d15a76d5f: Import Task Manager widgets ports to the new library.
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| applets/CMakeLists.txt | ||
|---|---|---|
| 7 | Why is icon tasks now in plasma-desktop instead of kdeplasma-addons? | |
| applets/icontasks/CMakeLists.txt | ||
| 2 | This stays "ng"? | |
| applets/taskmanager/package/contents/code/tools.js | ||
| 45 | var taskIndexList = []; | |
| applets/taskmanager/package/contents/ui/ContextMenu.qml | ||
| 124 | can be simplified to | |
| 217 | I suppose this is also emitted when I press return on the keyboard? | |
| 314 | I think this should be a checkable item "[X] Allow this program to be grouped" | |
| 325 | 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) | |
| applets/taskmanager/package/contents/ui/GroupDialog.qml | ||
| 40 | id at the top | |
| 109 | Where does that 6 come from? | |
| applets/taskmanager/package/contents/ui/MouseHandler.qml | ||
| 86 | That's the default | |
| applets/taskmanager/package/contents/ui/Task.qml | ||
| 234 | Can't you just bind prefix: TaskTools.taskPrefix(basePrefix)? | |
| applets/taskmanager/package/contents/ui/TaskList.qml | ||
| 29 | Doesn't that assumption break when you have launchers, ie. the first one being a tiny square instead of a wide task? | |
| applets/taskmanager/package/contents/ui/ToolTipDelegate.qml | ||
| 353 | TextMetrics { font: tooltipMaintextPlaceholder.font text: mainText } ? | |
| applets/taskmanager/plugin/backend.cpp | ||
| 115 | check KDesktopFile::isDesktopFile(...) like in the recentDocumentsActions | |
| 119 | Other than being incremented, this variable is not used | |
| 163 | QLatin1String | |
| 180–181 | init in one line? | |
| 299 | mapToScene? (QQuickItem 5.7 will have a mapToGlobal btw) | |
| 363 | Do we need an isDesktopFile check here too? | |
| applets/taskmanager/plugin/backend.h | ||
| 53 | Q_ENUM here instead of Q_ENUMS above | |
| 58 | coding style: QQuickItem *foo, also below | |
| 78 | coding style: const QUrl &url | |
Addressed the comments relevant to the purpose of the review,
in particular the incorrect install names.
| applets/CMakeLists.txt | ||
|---|---|---|
| 7 | 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 ... | |
| applets/icontasks/CMakeLists.txt | ||
| 2 | No, that's a bug from my rename orgy :) Thanks! | |
| applets/taskmanager/package/contents/code/tools.js | ||
| 45 | Really? :P Ok ... | |
| applets/taskmanager/package/contents/ui/ContextMenu.qml | ||
| 217 | Yup! | |
| 314 | This is the way it was before -- I don't mind changing it I guess ... | |
| 325 | "follow visible"? | |
| applets/taskmanager/package/contents/ui/GroupDialog.qml | ||
| 109 | Not sure - this is code that goes back to 4.11. It's not new in this review. | |
| applets/taskmanager/package/contents/ui/MouseHandler.qml | ||
| 86 | Being explicit is nice and aids readability. | |
| applets/taskmanager/package/contents/ui/Task.qml | ||
| 234 | IIRC Marco wrote this code, it's unrelated to the backend change anyway. | |
| applets/taskmanager/package/contents/ui/TaskList.qml | ||
| 29 | 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. | |
| applets/taskmanager/package/contents/ui/ToolTipDelegate.qml | ||
| 353 | Unrelated to this review. Please file a patch later. | |
| applets/taskmanager/plugin/backend.cpp | ||
| 115 | Aye. | |
| 119 | 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). | |
| 163 | Ok. | |
| 180–181 | Ok ... | |
| 299 | Am I supposed to guess what you mean here? Please elaborate. | |
| 363 | I don't think so -- hasApplicationType() should return false. | |
| applets/taskmanager/plugin/backend.h | ||
| 53 | Ok. | |
| 58 | Ok. | |
| 78 | Ok. | |
| applets/taskmanager/package/contents/ui/ContextMenu.qml | ||
|---|---|---|
| 124 | Ok. | |
| applets/taskmanager/package/contents/ui/Task.qml | ||
|---|---|---|
| 204 | maybe using an inline Component{} ? | |
| applets/taskmanager/package/contents/ui/Task.qml | ||
|---|---|---|
| 204 | Leaving that to Kai (this is not new code, and his) | |
Fix some regressions in layout and initial exporting of
minimize geo that crept in along the way.
Tested with multiscreen, and found a bug already. ;)
Not sure if it's in libtm or in the applets: but if you move a window to another screen, the corresponding task will not get updated until I minimize the task. IOW:
- 2 task managers, one in screen 1, and one in screen 2, both configured with "only show in current screen"
- Move app to screen 2 from screen 1
- Task entry stays on TM in screen 1
- Minimize app
- Task entry appears in screen 2
Second multiscreen issue found: right clicking to get the menu from a task gets the task in the wrong screen. Screenshot attached (left: screen 1, right: screen 2).
It's also wrong for tasks in the same screen (right click was on Dolphin's entry):
Tested it on a single screen system. No issues with context menus. Notice: I don't see jump lists with the new task manager, should I see them?
Not sure if it's in libtm or in the applets: but if you move a window to another screen, the corresponding task will not get updated until I minimize the task.
This is a cache eviction issue in the library. I'll update D1722 with the needed change.
Second multiscreen issue found: right clicking to get the menu from a task gets the task in the wrong screen. Screenshot attached (left: screen 1, right: screen 2).
It's also wrong for tasks in the same screen (right click was on Dolphin's entry):
I don't understand these explanations well, could you elaborate? Is the problem that the popups are mispositioned, or are context menus shown for the wrong task entirely? (Because if it's the former the bug is most likely in plasma-framework and unrelated to the code under review.)