pcdshub / lightpath

LCLS Lightpath Module
https://pcdshub.github.io/lightpath
Other
4 stars 9 forks source link

UI: Row Change #71

Closed teddyrendahl closed 6 years ago

teddyrendahl commented 6 years ago

Description

Load each row from a preconfigured .ui file instead of creating the entire widget in code. This leads to a better separation of "ui" code and actual "lightpath" logic. The overall PR actually adds lines of code, but if you look by making the UI file I was able to delete quite a few lines of actual Python code. The new UI file also makes the following changes.

This was going to part of a large PR that included Typhon screens but I've gone back and forth too many times on that implementation and chose just to push this snippet as its own entity.

Screenshots (if appropriate):

screen shot 2018-07-16 at 6 33 04 pm
codecov-io commented 6 years ago

Codecov Report

Merging #71 into master will decrease coverage by 1.1%. The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
- Coverage   88.47%   87.36%   -1.11%     
==========================================
  Files           7        7              
  Lines         425      380      -45     
==========================================
- Hits          376      332      -44     
+ Misses         49       48       -1
Impacted Files Coverage Δ
lightpath/ui/gui.py 94.5% <100%> (+2.13%) :arrow_up:
lightpath/ui/widgets.py 86.27% <84.61%> (-7.93%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 07e9782...8805a21. Read the comment docs.

ZLLentz commented 6 years ago

I'm a fan of the reorganization to make and edit the rows as .ui files. I'm still not fond of making the action buttons laid out vertically because it makes the application less intuitive to use from a muscle memory standpoint. It's much better to have an insert column and a remove column because the scrolling is vertical.

As laid out here, the insert and remove buttons are actually changing places as we scroll down the screen.

teddyrendahl commented 6 years ago
screen shot 2018-07-17 at 3 44 06 pm

Changes

ZLLentz commented 6 years ago

Looks better, I like the enable/disable solution. Not crazy about the wasted space, I wonder if it's worth having a compact mode... I'd like to be able to see a large swath of the beamline at once to scan for the blocking device. Maybe a separate issue.

How does this fit in with setting an attenuation on the screen? Or is that all going to be through typhon?