Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: Common table views, Primarily LivePVTableModel, minor skeleton work on NestableTableModel #72

Merged
merged 10 commits into from
Sep 6, 2024

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Aug 1, 2024

Description

Build a couple table models that can hopefully be reused elsewhere. Includes:

  • LivePVTableModel: a model for displaying {Parameter, Readback, Setpoint} data alongside the current values
  • NestableTableModel: a model for {Collection, Snapshot} entries, which shows title / description primarily
  • BaseTableModel that holds their own entries

Also:

  • refactors a bit of the existing table models
  • Adds a _PVPollThread for grabbing data outside the GUI thread

Note: I renamed superscore.widgets.tree -> superscore.widgets.views, but the diff viewer doesn't pick that up. I did not change the tree model at all, so the reviews can focus on the table models

To-do (in a later PR at this point):

Motivation and Context

Building screens, I fell down this rabbit hole and the diff warranted a separate PR

How Has This Been Tested?

Interactively so far, a couple tests added

Where Has This Been Documented?

This PR

image

image

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

@tangkong
Copy link
Contributor Author

tangkong commented Aug 2, 2024

I may work on #71 first... these tables want those data types, and I'll need to rebase this branch anyway

@tangkong tangkong changed the title WIP/MNT: Common table views MNT: Common table views Aug 28, 2024
@tangkong tangkong changed the title MNT: Common table views MNT: Common table views, Primarily LivePVTableModel, minor skeleton work on NestableTableModel Aug 28, 2024
@tangkong tangkong marked this pull request as ready for review August 28, 2024 22:01
@tangkong
Copy link
Contributor Author

This got too long, so I'm cutting it off at the LivePVTableModel and leaving the NestableTableModel for another PR. There's also still some improvements to be made for this table model but maybe in the spirit of an MVP we put that off

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was all nitpicks and questions, there isn't anything bad here
Feel free to discard the ones you disagree with

superscore/tests/test_views.py Outdated Show resolved Hide resolved
_bridge_cache: ClassVar[
WeakValueDictionary[int, QDataclassBridge]
] = WeakValueDictionary()
bridge: QDataclassBridge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the data class bridge used for with EntryItem? I remember its usage in atef but I don't see any other references to bridge here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't used this in superscore yet, but its usage is intended to be similar to atef. At some point we'll probably update an entry and want to make sure that updates in the tree view.

superscore/widgets/views.py Outdated Show resolved Hide resolved
superscore/widgets/views.py Outdated Show resolved Hide resolved
self.client = client
self.open_page_slot = open_page_slot
self.poll_rate = poll_rate
self._data_cache = {e.pv_name: None for e in self.entries} # noqa: F821
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is None preferable here to some sort of disconnected/not connected yet EpicsData value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't entirely intentional, None was just the easiest for me to initialize at the time. It might helpful to be able to differentiate between the not-yet-attempted state and an attempted-but-failed state, but I'm not wedded to it.

I suppose it might be better to keep the types consistent though

superscore/widgets/views.py Outdated Show resolved Hide resolved
superscore/widgets/views.py Outdated Show resolved Hide resolved
superscore/widgets/views.py Outdated Show resolved Hide resolved
superscore/widgets/views.py Outdated Show resolved Hide resolved
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tangkong tangkong merged commit b5763e1 into pcdshub:master Sep 6, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants