Manual device management dialog #413
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
==========================================
+ Coverage 74.92% 78.87% +3.95%
==========================================
Files 56 56
Lines 2839 2874 +35
==========================================
+ Hits 2127 2267 +140
+ Misses 712 607 -105 ☔ View full report in Codecov by Sentry. |
alexdewar
force-pushed
the
manual_device_config_dialog
branch
from
a2d2d8c to
7894715
Compare
There was a problem hiding this comment.
It looks good! Anything that declutters the main window is a step in the right direction. This looks a bit odd, though, and I'm not sure if its behaviour is the expected one.
The following screenshot is FINESSE just after opening it:
Note the odd dangling POLL Server light in the EM27 Monitor, with nothing else in that area. Then, I connect the (dummy) EM27 monitor in the brand new dialog, and I get a bunch of readings, displacing shuffling a bit the layout (specially the log, which get's squeeze):
Finally, I close the EM27 monitor in the dialog. The sensor readings remain, but greyed out.
Is this behaviour intentional? To be honest, I'm not entirely sure what I was expecting, but maybe some sort of symmetric behavior when connecting/disconnecting, the POLL light flushed to the bottom of the area and a more... maybe "rigid" layout, so adding/removing widgets do not upset the GUI?
As I say, I'm not sure what I was expecting and therefore I cannot advise what to change.
|
@dalonsoa It's less that it was intentional and more that that's just how it's ended up 😛. I think there are three related problems here:
See also: #405 How about I open separate issues for these and tackle them later? |
|
That's totally fine with me :) |
|
We could also add to the list: 4. The temperature plot on the right is too big |
|
Trying to position the POLL Server indicator & label sensibly was an absolute pain - it's definitely not positioned as intended! It bugs me every time I open the window but yeah, from what I recall it's not straightforward to fix. |
| class DeviceTypeControl(QGroupBox): | ||
| """A set of widgets for choosing a device and its params and connecting to it.""" | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
This constructor is quite long. It might make sense to break it down into smaller chunks, roughly matching your commented sections.
There was a problem hiding this comment.
Good point. As I've since modified this code on newer branches, I'll open an issue for it and fix it later.
We don't want to send it unconditionally when the main window loads.
Make a bespoke class for device parameters and use it.
Also, make parameters a Sequence cf. list.
…nstructor This is a harmless bug, which just means that the signal handler is invoked unnecessarily.
This will never be false.
This is needed so we don't get subtle bugs during testing.
alexdewar
force-pushed
the
manual_device_config_dialog
branch
from
f1d5a9c to
8387c0d
Compare
…ig_dialog Manual device management dialog
This PR moves the panel used for opening/closing connections to devices into a separate dialog (#395) and fixes a small associated bug (#397). This makes the main window less cluttered (see #405) and also means that we no longer have to load all plugins unless the user is using the manual device management dialog. Otherwise, the plugins will just be loaded as needed.
As this is (probably) the end of my tinkering with the
DeviceControlclass, I've added tests for it (#398). The coverage for thehardware_setsmodule is at 100% which takes us to 80% coverage overall 🥳Closes #395. Closes #397. Closes #398. Closes #405.