qui-devices refactor #55
Conversation
marmarta
force-pushed
the
qui-devices-rewrite
branch
2 times, most recently
from
885802a to
51b98d0
Compare
| "Detaching device {0} from {1} failed. Error: {2}".format( | ||
| self.device.description, vm, ex), | ||
| Gio.NotificationPriority.HIGH, | ||
| error=True) |
There was a problem hiding this comment.
This is inconsistent. Since this method is also used in attach_item(), it should give some indication whether detach succeeded or not. Otherwise, attach_item() will try to attach even if detach failed (leading to device being attached to multiple VMs). The purpose of detach_item() call in attach_item() was to avoid this situation.
On the other hand, the use case of switching device to another VM is (I think) rare, compared to simply detaching it. So, maybe the UI should be optimized for this common case of just detaching the device? In this case, I'd list only VM(s) to which device is currently connected, to ease clicking on it.
@andrewdavidwong, is my assumption correct here?
There was a problem hiding this comment.
On the other hand, the use case of switching device to another VM is (I think) rare, compared to simply detaching it. So, maybe the UI should be optimized for this common case of just detaching the device? In this case, I'd list only VM(s) to which device is currently connected, to ease clicking on it.
@andrewdavidwong, is my assumption correct here?
In my experience, it's not extremely common, but I'm not sure I'd call it "rare." I think it's reasonable to optimize for the most common "detaching" use case, but some users will certainly want to switch devices to other VMs. Perhaps the VM to which the device is currently connected should be visually more prominent (e.g., shown at the top of a list, with a separator below it, and all the other VMs below that).
There was a problem hiding this comment.
I've changed it to be bolded - to be honest I'm a bit wary about changing the VM order, it makes it harder to find the one you're looking for. I can be convinced otherwise, but as for now it seems for me that bolding it made it quite prominent enough.
qui/tray/devices.py
Outdated
|
|
||
| class VM: | ||
| def __init__(self, vm): | ||
| self.hash = hash(vm) |
There was a problem hiding this comment.
This internal attribute, must not be modified from outside and preferably also accessed. Please rename to __hash.
qui/tray/devices.py
Outdated
| '<b>Qubes Devices</b>\nView and manage devices.') | ||
| for domain in self.vms: | ||
| if domain.name == name: | ||
| domain.icon = vm.icon |
There was a problem hiding this comment.
This is real QubesVM object, so it should be vm.label.icon.
There was a problem hiding this comment.
Icon is also cached in devices (vm_icon attribute).
marmarta
force-pushed
the
qui-devices-rewrite
branch
from
2f75c79 to
148f42a
Compare
Thorough refactoring of qui-devices; instead of maintaining a list of updating Gtk widgets, only data is maintained and updated, and the menu is created on demand. fixes QubesOS/qubes-issues#4849 fixes QubesOS/qubes-issues#4720 fixes QubesOS/qubes-issues#4355 fixes QubesOS/qubes-issues#3215 fixes QubesOS/qubes-issues#2970
Bolded the VM name, to make it easier to find it when detaching.
marmarta
force-pushed
the
qui-devices-rewrite
branch
from
148f42a to
9478109
Compare
Thorough refactoring of qui-devices; instead of maintaining a list of
updating Gtk widgets, only data is maintained and updated, and the menu
is created on demand.
fixes QubesOS/qubes-issues#4849
fixes QubesOS/qubes-issues#4720
fixes QubesOS/qubes-issues#4355
fixes QubesOS/qubes-issues#3215
fixes QubesOS/qubes-issues#2970