ENH/REF: refactor shared table views #90

tangkong · 2024-09-27T21:45:21Z

Description

Refactors shared table models to have their own self-contained table views for ease of use and extensibility
Add methods to make columns editable for both shared table models
Adds ValueDelegate to provide constrained editing for columns based on their datatype and metadata
Add more metadata fields to EpicsData (precision, enums, limits, precision)
improve enum handling for LivePVTableView

Motivation and Context

Going about adding more functionality.

How Has This Been Tested?

Interactively, ensuring existing tests run

Where Has This Been Documented?

This PR
enum ValueDelegate

double ValueDelegate (must be < 0)

Pre-merge checklist

Code works interactively
Code follows the style guide
Code contains descriptive docstrings, including context and API
New/changed functions and methods are covered in the test suite where possible
Test suite passes locally
Test suite passes on GitHub Actions
Ran docs/pre-release-notes.sh and created a pre-release documentation page
Pre-release docs include context, functional descriptions, and contributors as appropriate

…om QTableViews for simplicity of use and ease of extensibility

…table flags, improve enum handling

…Add set_editable method to views

…leModel to be editable similar to LivePVTableModel

tangkong · 2024-10-03T23:08:47Z

Not requesting a review from Devan, as he's on vacation. This was starting to snowball (as always), so I'll cut this off here. Everything was originally in service of making the cells editable. Lots of little things got hit along the way

ZLLentz

Partial review: I'll need to start again at the views definitions when I feel less tired

ZLLentz · 2024-10-03T23:51:04Z

superscore/control_layers/_aioca.py

+            value_time = await caget(address, format=dbr.FORMAT_TIME)
+            value_ctrl = await caget(address, format=dbr.FORMAT_CTRL)


Possibly we can do this in parallel for some minor performance gains to cut the network waiting time in half

not 100% sure on syntax etc

Suggested change

value_time = await caget(address, format=dbr.FORMAT_TIME)

value_ctrl = await caget(address, format=dbr.FORMAT_CTRL)

value_time, value_ctrl = await asyncio.gather(

caget(address, format=dbr.FORMAT_TIME),

caget(address, format=dbr.FORMAT_CTRL),

)

ZLLentz · 2024-10-04T00:45:01Z

The result looks great though, next time I'll try to run it myself and read through the code with renewed energy

ZLLentz · 2024-10-07T22:00:21Z

@tangkong what's the best way for me to run superscore and try this out myself?

tangkong · 2024-10-07T22:26:27Z

This should work

$ export SUPERSCORE_CFG=/path/to/repo/superscore/superscore/tests/config.cfg
$ python -m superscore ui

superscore/widgets/views.py

ZLLentz · 2024-10-07T23:25:41Z

Nothing in the code stands out as odd and there's a lot of qt stuff that's hard to parse through, so instead I'm going to reset my environment and approach this from a qa perspective

ZLLentz · 2024-10-08T00:13:26Z

I'm not sure what I'm doing wrong, maybe I don't understand how to use the ui. I can't view any of the configs.

tangkong · 2024-10-08T18:47:49Z

We don't have entry-specific pages for anything but the Collection yet, which itself is just a placeholder. Right now the only pages are the collection builder page and the search page.

What are you trying to get to that isn't working?

ZLLentz · 2024-10-08T19:55:11Z

I guess I was expecting to be able to open a page and see one of these views in action

tangkong · 2024-10-08T23:58:15Z

The collection builder page has these views and should be working.

Sorry I haven't been the most clear with how to test this here. If you navigate File -> New -> Collection, the collection-builder-page will open, from which you can add PVs and sub-collections. This by default doesn't let you edit the entries added, but the type-specific delegates should appear just fine

…utton_cols to keep track of cols to skip setData on

ZLLentz · 2024-10-10T01:41:05Z

Ok I clicked around on the new collection gui
I couldn't find where to see the "stored value" thing I think because I couldn't figure out how to make a snapshot

Feel free to ignore any of these if they are already known or are pre-existing issues

Clicking tags+ crashes the gui

Traceback (most recent call last):
  File "/cds/home/z/zlentz/github/superscore/superscore/widgets/core.py", line 216, in add_tag
    elem = tags_list.add_item('')
  File "/cds/home/z/zlentz/github/superscore/superscore/widgets/core.py", line 297, in add_item
    self.data_list.append(starting_value)
AttributeError: 'QDataclassValueForobject' object has no attribute 'append'
Aborted (core dumped)

I get lots of qt.glx errors but it might be my environment

qt.glx: qglx_findConfig: Failed to finding matching FBConfig for QSurfaceFormat(version 2.0, options QFlags<QSurfaceFormat::FormatOption>(), depthBufferSize -1, redBufferSize 1, greenBufferSize 1, blueBufferSize 1, alphaBufferSize -1, stencilBufferSize -1, samples -1, swapBehavior QSurfaceFormat::SingleBuffer, swapInterval 1, colorSpace QSurfaceFormat::DefaultColorSpace, profile  QSurfaceFormat::NoProfile)
No XVisualInfo for format QSurfaceFormat(version 2.0, options QFlags<QSurfaceFormat::FormatOption>(), depthBufferSize -1, redBufferSize 1, greenBufferSize 1, blueBufferSize 1, alphaBufferSize -1, stencilBufferSize -1, samples -1, swapBehavior QSurfaceFormat::SingleBuffer, swapInterval 1, colorSpace QSurfaceFormat::DefaultColorSpace, profile  QSurfaceFormat::NoProfile)
Falling back to using screens root_visual.

It'd be nice to have a way to edit PVs that have already been added to a collection before saving it, I instead need to delete and add a new PV
Hitting save configuration twice gives this (the first save didn't seem to add it to the tree or make it findable via search). Maybe save configuration should close the edit gui or replace it with the view-only gui?

ERROR:superscore.widgets.views:Failed to set data (False) ->(0, 9): <LivePVHeader.REMOVE: 9>
ERROR:superscore.widgets.views:Failed to set data (False) ->(0, 8): <LivePVHeader.OPEN: 8>
INFO:superscore.widgets.page.collection_builder:Collection saved (10c5933d-79a3-46ff-8bd3-96a1352e3bf1)
qt.glx: qglx_findConfig: Failed to finding matching FBConfig for QSurfaceFormat(version 2.0, options QFlags<QSurfaceFormat::FormatOption>(), depthBufferSize -1, redBufferSize 1, greenBufferSize 1, blueBufferSize 1, alphaBufferSize -1, stencilBufferSize -1, samples -1, swapBehavior QSurfaceFormat::SingleBuffer, swapInterval 1, colorSpace QSurfaceFormat::DefaultColorSpace, profile  QSurfaceFormat::NoProfile)
No XVisualInfo for format QSurfaceFormat(version 2.0, options QFlags<QSurfaceFormat::FormatOption>(), depthBufferSize -1, redBufferSize 1, greenBufferSize 1, blueBufferSize 1, alphaBufferSize -1, stencilBufferSize -1, samples -1, swapBehavior QSurfaceFormat::SingleBuffer, swapInterval 1, colorSpace QSurfaceFormat::DefaultColorSpace, profile  QSurfaceFormat::NoProfile)
Falling back to using screens root_visual.
Traceback (most recent call last):
  File "/cds/home/z/zlentz/github/superscore/superscore/widgets/page/collection_builder.py", line 124, in save_collection
    self.client.save(self.data)
  File "/cds/home/z/zlentz/github/superscore/superscore/client.py", line 174, in save
    self.backend.save_entry(entry)
  File "/cds/home/z/zlentz/github/superscore/superscore/backends/filestore.py", line 271, in save_entry
    raise BackendError("Entry already exists, try updating the entry "
superscore.errors.BackendError: Entry already exists, try updating the entry instead of saving it
Aborted (core dumped)

After saving my config, closing, and re-opening, I now have a validation error on opening the config again

$ python -m superscore ui
Traceback (most recent call last):
  File "/cds/home/z/zlentz/miniconda3/envs/superscore/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/cds/home/z/zlentz/miniconda3/envs/superscore/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/cds/home/z/zlentz/github/superscore/superscore/__main__.py", line 3, in <module>
    main()
  File "/cds/home/z/zlentz/github/superscore/superscore/bin/main.py", line 97, in main
    func(**kwargs)
  File "/cds/home/z/zlentz/github/superscore/superscore/bin/ui.py", line 21, in main
    main_window = Window()
  File "/cds/home/z/zlentz/github/superscore/superscore/widgets/window.py", line 45, in __init__
    self.setup_ui()
  File "/cds/home/z/zlentz/github/superscore/superscore/widgets/window.py", line 56, in setup_ui
    self.tree_model = RootTree(base_entry=self.client.backend.root,
  File "/cds/home/z/zlentz/github/superscore/superscore/backends/filestore.py", line 256, in root
    with self._load_and_store_context():
  File "/cds/home/z/zlentz/miniconda3/envs/superscore/lib/python3.9/contextlib.py", line 119, in __enter__
    return next(self.gen)
  File "/cds/home/z/zlentz/github/superscore/superscore/backends/filestore.py", line 353, in _load_and_store_context
    db = self._load_or_initialize()
  File "/cds/home/z/zlentz/github/superscore/superscore/backends/filestore.py", line 63, in _load_or_initialize
    self._root = self.load()
  File "/cds/home/z/zlentz/github/superscore/superscore/backends/filestore.py", line 251, in load
    return deserialize(Root, serialized)
  File "/cds/home/z/zlentz/miniconda3/envs/superscore/lib/python3.9/site-packages/apischema/deserialization/__init__.py", line 887, in deserialize
    return deserialization_method(
  File "/cds/home/z/zlentz/miniconda3/envs/superscore/lib/python3.9/site-packages/apischema/deserialization/methods.py", line 697, in deserialize
    raise ValidationError(errors or [], field_errors or {})
apischema.validation.errors.ValidationError: ValidationError: [{'loc': ['entries', 4, 'Collection', 'description'], 'err': 'expected type string, found array'}, {'loc': ['entries', 4, 'Collection', 'title'], 'err': 'expected type string, found array'}]

Here's the diff from my db:

+    },
+    {
+      "Collection": {
+        "uuid": "10c5933d-79a3-46ff-8bd3-96a1352e3bf1",
+        "description": [
+          "Defo neater than your collection haha"
+        ],
+        "creation_time": "2024-10-09T23:28:21.315755+00:00",
+        "title": [
+          "My neat collection"
+        ],
+        "children": [
+          {
+            "uuid": "3de82331-0f13-4ed7-a668-0d33eae8cd1e",
+            "description": "",
+            "creation_time": "2024-10-09T23:32:00.149681+00:00",
+            "pv_name": "IM3L0:PPM:MMS:STATE:SET",
+            "abs_tolerance": null,
+            "rel_tolerance": null,
+            "readback": {
+              "uuid": "c092bad8-ada1-4efa-abd8-46306b0b7836",
+              "description": "",
+              "creation_time": "2024-10-09T23:32:00.149661+00:00",
+              "pv_name": "IM3L0:PPM:MMS:STATE:GET_RBV",
+              "abs_tolerance": null,
+              "rel_tolerance": null,
+              "readback": null,
+              "read_only": true
+            },
+            "read_only": false
+          }
+        ],
+        "tags": []
+      }

Note how the description and title are arrays of strings instead of strings

Anyway sorry for only having bugs to report, I think the snapshot gui from your screenshot above looks the most snazzy but I couldn't figure out how to get to it

tangkong · 2024-10-10T16:14:46Z

No, thank you for the bug reports. Helps keep me honest. The editable delegates aren't immediately accessible, since the existing pages don't support editing values by design. I'm currently working on some additional pages that should expose this more clearly (hopefully)

I'll go through the bugs and see what I can fix. 👍

tangkong · 2024-10-10T17:26:40Z

There were stray commas somehow, making strings into tuples. Validation errors and saving title/descriptions into lists should be fixed in ef4bc1f

tangkong · 2024-10-10T19:35:27Z

Tags will remain broken until we actually implement the feature. Part of the problem is that the TagsWidget was expecting the dataclass field to be a list not a set, and the other part of the problem is that the field is a Set[Tag], where Tag is a Flag. We haven't tested serialization with these flags, and if we intend to centrally manage / restrict the tags that can be chosen, this widget will need further modifications.

All this to say I'm not going to work on this further for now. 😢

tangkong · 2024-10-10T19:51:11Z

If you're interested in running a slightly modified collection builder page to see the delegates work, you can run this file:

python /cds/home/r/roberttk/test/superscore/enh_views_test.py

…g stored values

tangkong · 2024-10-10T21:37:05Z

It has become abundantly clear that this was written during the heat wave. The quality is awful. Thank you for being patient. I've got many test cases to write, now that I've caught so many little bugs

ZLLentz · 2024-10-10T21:56:18Z

I think this is probably mature enough to merge with some documentation of things to be fixed later

tangkong · 2024-10-10T22:22:01Z

Thanks for the thorough QA. I'll make some followup issues and add some tests before re-requesting a review+stamp

…nd client

ZLLentz

I think I only had testing nitpicks

superscore/tests/test_views.py

ZLLentz · 2024-10-16T00:22:21Z

superscore/tests/test_views.py

+    yield view
+
+    view.model().stop_polling()
+    qtbot.wait_until(lambda: not view.model()._poll_thread.isRunning())


This test has no assert statements. Is there nothing specific to test for in the basic behavior of getting values?

This is actually a fixture that I use in the subsequent tests. In making it I did reveal to myself some smaller bugs, so in spirit it is also sort of like a test

new rule: no code reviews from me after 5pm

Co-authored-by: Zachary Lentz <[email protected]>

tangkong · 2024-10-16T17:14:18Z

I'll merge this now, and try to help Devan either refactor his table usage or fix it up after he merges his effort.

tangkong · 2024-10-16T17:14:34Z

Thanks everyone for the reviews!

tangkong added 4 commits September 30, 2024 16:30

ENH/WIP: refactor sub pv and nestable tables into self-contained cust…

86f669c

…om QTableViews for simplicity of use and ease of extensibility

ENH: add enum and numeric metadata fields to EpicsData

8451d4b

ENH: add ValueDelegate for Edit widgets based on datatype, enable edi…

32a1a8c

…table flags, improve enum handling

BUG: fix enum handling for is_close highlighting

6a25bed

tangkong force-pushed the enh_pv_nest_views branch from 808d661 to 6a25bed Compare September 30, 2024 23:31

tangkong added 5 commits October 3, 2024 14:29

MNT: fix edge cases while data is fetching

eb8249b

Add poll_period to LivePVTableView kwargs, pass to underlying model. …

ad74dcf

…Add set_editable method to views

MNT: generalize HeaderEnum, BaseTableModel.setData. Setup NestableTab…

996dc40

…leModel to be editable similar to LivePVTableModel

TST: add quick test_coll_builder_edit to verify edit functionality

2bf3f35

DOC: pre-release notes

3ff5903

tangkong requested a review from ZLLentz October 3, 2024 23:06

tangkong marked this pull request as ready for review October 3, 2024 23:06

tangkong changed the title ~~ENH/REF/WIP: refactor shared table views~~ ENH/REF: refactor shared table views Oct 3, 2024

ZLLentz reviewed Oct 4, 2024

View reviewed changes

ZLLentz reviewed Oct 7, 2024

View reviewed changes

superscore/widgets/views.py Show resolved Hide resolved

tangkong added 2 commits October 7, 2024 16:46

BUG: allow BaseTableEntryModel to not specify _editable_cols

72d4d48

BUG: actually fix the bug

891849f

tangkong added 3 commits October 9, 2024 16:37

MNT: fix uuid filling in BaseDataTableView.gather_sub_entries, add _b…

015f737

…utton_cols to keep track of cols to skip setData on

MNT: make ResultsModel properly implement BaseTableEntryModel

863cf45

MNT: pass client to RootTree so it can fill entries

330939b

BUG: remove commas that made text fields into tuples

ef4bc1f

MNT: let PV_NAME be editable in collection builder

9a7ec2e

tangkong added 2 commits October 10, 2024 14:10

BUG: let (0,0) limits mean no limits, be more precise about enum check

833f3aa

BUG/MNT: fix status/severity handling when setting data and displayin…

f8f4290

…g stored values

tangkong mentioned this pull request Oct 10, 2024

GUI: handle tags in NameDescTagsWidget #92

Open

tangkong added 2 commits October 15, 2024 15:46

MNT: make BaseDataTableView less strict about order of setting data a…

17dbe29

…nd client

TST: add and expand tessts for shared views

cc7a400

tangkong requested review from ZLLentz and shilorigins October 15, 2024 23:01

TST: actually round-trip serialize/deserialize, remove unused qtbot

8fbac15

ZLLentz previously approved these changes Oct 16, 2024

View reviewed changes

tangkong dismissed ZLLentz’s stale review via 8df75f5 October 16, 2024 00:39

TST: sort limits from low to high

8df75f5

Co-authored-by: Zachary Lentz <[email protected]>

ZLLentz approved these changes Oct 16, 2024

View reviewed changes

shilorigins approved these changes Oct 16, 2024

View reviewed changes

tangkong merged commit 6ef255c into pcdshub:master Oct 16, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH/REF: refactor shared table views #90

ENH/REF: refactor shared table views #90

tangkong commented Sep 27, 2024 •

edited

Loading

tangkong commented Oct 3, 2024

ZLLentz left a comment

ZLLentz Oct 3, 2024

ZLLentz commented Oct 4, 2024

ZLLentz commented Oct 7, 2024 •

edited

Loading

tangkong commented Oct 7, 2024

ZLLentz commented Oct 7, 2024

ZLLentz commented Oct 8, 2024

tangkong commented Oct 8, 2024

ZLLentz commented Oct 8, 2024

tangkong commented Oct 8, 2024 •

edited

Loading

ZLLentz commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

ZLLentz commented Oct 10, 2024

tangkong commented Oct 10, 2024 •

edited

Loading

ZLLentz left a comment

ZLLentz Oct 16, 2024

tangkong Oct 16, 2024

ZLLentz Oct 16, 2024

tangkong commented Oct 16, 2024

tangkong commented Oct 16, 2024

		value_time = await caget(address, format=dbr.FORMAT_TIME)
		value_ctrl = await caget(address, format=dbr.FORMAT_CTRL)

-            value_time = await caget(address, format=dbr.FORMAT_TIME)
-            value_ctrl = await caget(address, format=dbr.FORMAT_CTRL)
+            value_time, value_ctrl = await asyncio.gather(
+                caget(address, format=dbr.FORMAT_TIME),
+                caget(address, format=dbr.FORMAT_CTRL),
+            )

ENH/REF: refactor shared table views #90

ENH/REF: refactor shared table views #90

Conversation

tangkong commented Sep 27, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Pre-merge checklist

tangkong commented Oct 3, 2024

ZLLentz left a comment

Choose a reason for hiding this comment

ZLLentz Oct 3, 2024

Choose a reason for hiding this comment

ZLLentz commented Oct 4, 2024

ZLLentz commented Oct 7, 2024 • edited Loading

tangkong commented Oct 7, 2024

ZLLentz commented Oct 7, 2024

ZLLentz commented Oct 8, 2024

tangkong commented Oct 8, 2024

ZLLentz commented Oct 8, 2024

tangkong commented Oct 8, 2024 • edited Loading

ZLLentz commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

tangkong commented Oct 10, 2024

ZLLentz commented Oct 10, 2024

tangkong commented Oct 10, 2024 • edited Loading

ZLLentz left a comment

Choose a reason for hiding this comment

ZLLentz Oct 16, 2024

Choose a reason for hiding this comment

tangkong Oct 16, 2024

Choose a reason for hiding this comment

ZLLentz Oct 16, 2024

Choose a reason for hiding this comment

tangkong commented Oct 16, 2024

tangkong commented Oct 16, 2024

tangkong commented Sep 27, 2024 •

edited

Loading

ZLLentz commented Oct 7, 2024 •

edited

Loading

tangkong commented Oct 8, 2024 •

edited

Loading

tangkong commented Oct 10, 2024 •

edited

Loading