| VDG | |
| Plasma |
The KCM allows user to authorize and trust (or revoke trust) Thunderbolt 3 devices.
This is done by communicating with the Bolt daemon via DBus (through libkbolt).
The KDED module listens for new unauthorized devices and shows a
notification with a button to authorize when such device is connected.
Obligatory screenshots:
See https://mail.kde.org/pipermail/plasma-devel/2019-February/093482.html for details and a link to a video.
Tested with my Dell laptop and Dell dock station, I unfortunately don't have access to any other Thunderbolt hardware.
| No Linters Available |
| No Unit Test Coverage |
| Buildable 11325 | |
| Build 11343: arc lint + arc unit |
| autotests/bolt/fakeserver/fakeserver.h | ||
|---|---|---|
| 2 | You forgot to add a GPL license header for this file. Same for:
| |
| kcms/bolt/package/contents/ui/DeviceList.qml | ||
|---|---|---|
| 12 | You might want to be using a ScrollViewKCM and put the enable checkbox in the header and the ListView in view, see for instance KWin's virtual desktop KCM | |
| 30 | Use onToggled which is only fired when the user explicitly clicks it rather than if some binding causes it to change. Also, no need for function() | |
| kcms/bolt/package/contents/ui/main.qml | ||
| 89 | onItemClicked? | |
| kded/bolt/kded_bolt.cpp | ||
| 91 | Now we need a beautiful Breeze Thunderbolt icon :) | |
| 117 | reserve()? | |
| 147 | Please HTML escape the device name | |
| libs/bolt/device.h | ||
| 46 | Does bolt not notify property changes on dbus? Not a huge fan of this Timer refresh hack | |
That's right, the list needs a frame and a white background, rather than just having the items floating there in the middle of the page. The new Virtual Desktops KCM is a good visual template.
Thanks for the feedback, I'll look into it asap. In the meantime, could I ask the VDG to create a Breeze icon for Thunderbolt? @abetts, maybe?
or @andreask or @trickyricky26 or @ndavis. We have a lot of icon design talent here these days. :)
Thanks, much better! Just a few more UI nitpicks. And one more thing: What does "Stored" mean in this context? As a non-Thunderbolt expert, I don't know what this means, and I suspect most users wouln't, either.
| kcms/bolt/package/contents/ui/DeviceList.qml | ||
|---|---|---|
| 42 | "Enable" might be a better word than "Allow" in this string. | |
| kcms/bolt/package/contents/ui/DeviceView.qml | ||
| 51 | Can we give this a label or make it a Button instead? Many users have difficulty recognizing label-less toolbuttons floating inside the page as clickable buttons. | |
| 151 | I'd recommend that we center this horizontally on the page. | |
"Stored" is a term for a remembered device. When you authorize a device, the authorization is "stored", or remembered, so you don't have to authorize it again next time. Maybe "Remember" would be a better term, then? Also works better with the "Forget".
What is a "remembered device"? As in, the settings for this particular device are remembered and re-applied when the device is unplugged and plugged in again later?
How about "Trusted" for remembered devices that are authorized, and "Prohibited" for remembered devices that are not authorized?
"Remembered" would work only if it's clear what's being remembered (i.e. the settings). Maybe it could say "Settings remembered" or something?
How about "Trusted" for remembered devices that are authorized, and "Prohibited" for remembered devices that are not authorized?
"Remembered" would work only if it's clear what's being remembered (i.e. the settings). Maybe it could say "Settings remembered" or something?
“Trusted” and “Untrusted” devices sounds better, considering the nature of thunderbolt devices enabled explicitly by user.
Can the text to the right of the list, "Trusted" be a different color and maybe a smaller font? When using the same font, color and size as the device name, it seems that they are the same thing. One is device name and the other is device status. I am just wondering if we could differentiate them visually.
Hey, that's not a bad idea. The font should probably be the same size, but we could use the color scheme's positive color for trusted and its neutral or negative color for untrusted.
What should happen when the "Help" button at the bottom of KCM page is pressed?
Thanks in advance for your answer.
Remove 'Help', 'Default' and 'Apply' buttons as they serve no purpose here,
there's no help at the moment, there's no default state to revert to and the
changes to devices are applied immediately.
I realized we still need an icon for Thunderbolt (and thus for this KCM) - @abetts, @andreask, @trickyricky26, @ndavis - could one of you help me out, please? I have no clue about icons...
| kded/bolt/kded_bolt.h | ||
|---|---|---|
| 35 | Coding style nitpick:
| |
| libs/bolt/dbushelper.cpp | ||
| 36 | Nitpick: "// anonymous namespace" is more verbose. :-) | |
| libs/bolt/device.cpp | ||
| 96 | Coding style nitpick(?): Maybe it's a matter of taste, but is it a good idea to use auto here? | |
I was wondering, since this thing has like a lib and kded and what not, wouldn't it make sense to have that be a separate module and repository rather than in plasma-desktop "just because" all kcms are in there?
For the lack of Thunderbolt devices I have only tried the checkbox for turning it on and off :) Is there a way to fix it asking for a password right when opening it? No other KCM does that.
@broulik I asked on plasma-devel where to put this and got no definitive answer, so I put it here. Separate repo is fine with me, too. What name for the repo would you prefer? plasma-thunderbolt? I already have kcm_bolt scratch repo, so I can just "backport" the changes from here into it and ask sysadmins to move it to kde/workspace and rename it.
The password prompt, I believe, is from polkit - I did not see it on my system. Maybe the polkit rules in boltd are wrong, but I believe we only do read-only operations when you open the KCM.
Plasma 5.16 has been released and master is wide open for new features. Can we get this landed in some capacity soon? It would be great to have more testing time.
There is already a plasma-thunderbolt repository with this code. IMHO it fits better than adding everything to plasma-desktop, and it can be already tested without requiring the git/master version of plasma-desktop.
Ah OK, I hadn't realized it got moved there yet. I guess this revision can be abandoned then, and we should lobby distros to ship with the new repo/package.
The code has been moved to a standalone plasma-thunderbolt repository, passed through kde-review and will be included in Plasmas 5.17 release.
Thanks everyone for your feedback and comments!
Provide a tarball and I'll gladly turn our git master ebuild into a release ebuild. ;)
