ENH: add snapshot view / compare / restore page (only view functional) #94
Conversation
shilorigins
added
enhancement
shilorigins
force-pushed
the
devagr/restore-page
branch
from
441f431 to
2b50c0c
Compare
|
I grabbed the changes and they look pretty good. No major bugs, though there are a limited number of interaction points thus far. From interactive testing:
I'll go through the code in a bit more detail now |
|
I was just adding some (any) tests before requesting review. Is there a setpoint / readback page to connect to |
whoops sorry I jumped the gun a bit. I have pages in #95, and have been adding tests there as well (though I haven't managed to finish and push them yet 💀 ) |
shilorigins
force-pushed
the
devagr/restore-page
branch
3 times, most recently
from
5bc6afe to
f871071
Compare
The existing code would occassionally cause this error when closing
a tab containing a LivePVTableModel:
Traceback (most recent call last):
File "/afs/slac.stanford.edu/u/cd/devagr/src/superscore/superscore/widgets/views.py", line 1021, in run
self._update_data(pv_name)
File "/afs/slac.stanford.edu/u/cd/devagr/src/superscore/superscore/widgets/views.py", line 1008, in _update_data
if not isinstance(val, Exception) and self.data[pv_name] != val:
KeyError: 'VAC:GUNB:TEST1'
This error appears to be caused by LivePVTableModel.stop_polling
clearing _PVPollThread's data before calling its stop method, causing
the thread to be able to run for another cycle and try to access its
now-empty data. Calling stop() and then waiting before clearing the
data seems to prevent the error.
shilorigins
force-pushed
the
devagr/restore-page
branch
from
e943055 to
6e8a0a5
Compare
|
Since a branch merged into master replaced the default |
shilorigins
force-pushed
the
devagr/restore-page
branch
from
6e8a0a5 to
fe29791
Compare
shilorigins
changed the title
| @@ -10,6 +10,9 @@ | |||
| from superscore.client import Client | |||
| from superscore.widgets.window import Window | |||
|
|
|||
| DEFAULT_WIDTH = 1400 | |||
| DEFAULT_HEIGHT = 800 | |||
There was a problem hiding this comment.
nitpick: I'm always wary of setting raw defaults for things like sizes, since peoples' display setups vary so much. There's probably a way to set this relative to a user's total screen real estate 🤔 .
I also won't argue that superscore needs more space than qt was originally providing it. So I don't object strongly to this
There was a problem hiding this comment.
I agree, but I don't know how to manage screen details without researching it a bit. Like how your setup reports one combined screen while mine reports two screens. I suppose we could do percent of primary screen with a maximum absolute size?
There was a problem hiding this comment.
Yea, there's a few ideas but we can address these in more detail later
| @@ -311,6 +311,8 @@ def linac_data(): | |||
| pv_name=lasr_gunb_pv1.pv_name, | |||
| description=lasr_gunb_pv1.description, | |||
| data="Off", | |||
| status=Status.NO_ALARM, | |||
There was a problem hiding this comment.
A shame you had to specify all these, but I do think our defaults still make sense.
There was a problem hiding this comment.
I agree. I couldn't come up with a nice way to generate test snapshots without taking too much time thinking about it, but we might want to revisit if we need to generate more. The best idea I had was mocking uuid4() and ControlLayer.get with consistent, pre-selected data and then using Client.snap.
There was a problem hiding this comment.
Related but not directly applicable: I think if we can set up some way of selecting a client/backend-data combination using our fixtures, that would be ideal. We already have a variety of datasets, but our sample_client only used the filestore json.
You could also take a snapshot of your controls system and save it in a filestore-backend, then we could add that as a backend to test with?
The existing code to resize and recenter the main window had a bug where multiple monitors were apparently being reported as one large screen, causing the window to appear in between two monitors. This commit centers the window on the primary screen specifically.
shilorigins
force-pushed
the
devagr/restore-page
branch
from
38731ac to
1ee381e
Compare
Description
Add restore page template and logic
Add ability to open restore page from root tree
QTreeViewIncrease default size of the main window
Fix bug where closing a
LivePVTableModelsometimes causes an error trying to access deleted dataMotivation and Context
This page can currently show all leaf
Entrys in a snapshot and can compare stored values to live values. The same page is intended to be able to compare values from two snapshots and to restore saved values, but both of those capabilities are postponed for later PRs.The AD control system has a convention where middle clicking on a UI element shows the underlying PV and copies it to the clipboard, so I added middle-click-to-copy as a partial implementation and convenience feature. We can fill out the PV-display-and-copy later if desired.
Double-clicking an entry on the tree view opens the entry page and no longer expands the tree item because I found that use case intuitive, and items can still be expanded by clicking on the items' arrows. Happy to alter this functionality if doing so would be more usable.Since the
Snapshotentry page is now associated with the tree view's double-click, I added an action to open the restore page to the tree view's context menu.I increased the size of the main window to better show the data-dense tables without having to manually expand the window every time. I didn't optimize the new size, but it works well enough on my dev monitor and laptop.
How Has This Been Tested?
Interactively.
Where Has This Been Documented?
Screenshots:
Page when a snapshot is opened:
Page when
Compare to Livebutton is clicked:Pre-merge checklist
docs/pre-release-notes.shand created a pre-release documentation page