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

ENH/REF: Modify FillTemplatePage to have staging area #239

Merged
merged 13 commits into from
Jun 3, 2024

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented May 30, 2024

Description

  • Adds a staging area to FillTemplatePage, to be more clear about which changes are being queued for application
  • Refactors various helpers and widgets to support this
  • Adds tree-view that highlights closest tree-item when a row detail is examined

Motivation and Context

In looking into reusing FillTemplatePage for templated checkouts, it became clear that the page needed a refactor

  • If we were to serialize actions, we would need some way to display those queued actions.
    • Previously, FillTemplatePage would maintain an internal "all actions" list and a user could apply them gradually. This made the current state of the loaded checkout ambiguous (had we applied changes yet?)
  • Edit actions (FindReplaceAction) saved in a templated checkout would need to be loaded without knowledge of the search query that produced it.
    • Prior to this FillTemplatePage would gather all actions in any populated edit when asked to apply all. The user would either have to know that these were the right actions or click through them all to verify
    • Now the staging area makes clear what will be applied when we save the file, and edits are not applied until the new file is saved

How Has This Been Tested?

Entirely interactively so far. But the test suite passes

Where Has This Been Documented?

This PR.

image

image

Compare to before:

Details

image

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Code has been checked for threading issues (no blocking tasks in GUI thread)
  • 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 tangkong requested review from ZLLentz and janeliu-slac May 30, 2024 21:58
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 always find it really hard to review atef
I had a comment and a docs nitpick
I'll try to run this myself to play qa bugfinder

atef/widgets/config/run_base.py Show resolved Hide resolved
@@ -56,7 +58,7 @@ def verify_file_and_notify(
bool
the verification success
"""
verified, msg = file.verify()
verified, msg = file.validate()
Copy link
Member

Choose a reason for hiding this comment

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

Was this unknowingly broken for a few days?

Copy link
Contributor Author

@tangkong tangkong May 31, 2024

Choose a reason for hiding this comment

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

Yes 😢 . My ctrl+f did not ctrl+f enough it seems

@tangkong
Copy link
Contributor Author

I always find it really hard to review atef

I'm not sure how to make this better. atef is complicated but I think the web of signals and slots tends to make things opaque.

I've often wondered what it would take to make the atef development process more clear...

@ZLLentz
Copy link
Member

ZLLentz commented May 31, 2024

Design thoughts and nitpicks:

  • The "select file" button is surprisingly hard to find from a blank template fill because it's at the bottom of a very long screen and selecting a file ends up populating the very top portion of the screen. I wonder if there should be another way to open a file, some clicky click magic in one of the top areas, for example.
  • What is the intent of the "signals" column in the top-right view? It certainly isn't showing device signals, is it just for PV configs? If it's for PV configs I suggest marking the column as e.g. PVs instead of as Signals to evoke the same terminology as used in the rest of atef.
  • I'm glad you have the movable dividers because qt default resizing is pain as always. One of these is slightly awkward though because it's so so close to the "Stage All" button, it's easy to click the button by mistake while aiming for the divider.
  • I didn't see any bugs
  • The "edits" table header is the only text element on the display that isn't capitalized (should probably be "Edits")

@tangkong
Copy link
Contributor Author

"no obvious bugs" has to be an all-time high for me 😆

@tangkong tangkong requested a review from ZLLentz June 3, 2024 16:10
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'm 👍 on this design update

@tangkong
Copy link
Contributor Author

tangkong commented Jun 3, 2024

Great, thanks! I'm going to merge this and forge ahead with the GUI elements that fold this into the main checkouts.

@tangkong tangkong merged commit 1ed08e5 into pcdshub:master Jun 3, 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.

2 participants