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: positioner row widget sizing usability fixes and tweaks #611

Merged
merged 27 commits into from
Jun 18, 2024

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Jun 6, 2024

Description

Rework the design, sizing, and font scaling of the positioner row widget to address concerns about readability in RIX for devices with large position numbers, and to address issues with poorly used space in state devices.

Full list of changes:

dynamic_font.py:

  • Allow dynamic font scaling to be applied to comboboxes
  • Allow dynamic font resizing widgets to optionally define a minimum and maximum font size.
  • Let font size change by less than the delta if it is to snap to the minimum or maximum, making sizing more consistent at the edges.
  • Minor caching optimizations related to dynamic font scaling (there could be more improvements here)
  • setText-based widgets with dynamic font scaling now also adjust their font size when their text contents change
  • Remove sizing overrides for dynamic patched widgets because they are no longer needed now that we're not using paint events and they get in the way of implementing proper sizing elsewhere
  • Set the font size prior to the first resize in case the widget is never resized.

panel.py

  • Fix all known sizing issues related to recursive composite signal panels

positioner.py

  • Remove reload_widget_stylesheet call from clear_error to avoid an issue where clicking clear_error would undo all dynamic font scaling and set it back to the default size.
  • Include the possibility for the limit indicators to completely disappear to allow other widgets to use their space
  • Remove references to fixed sizing to allow for the possibility of widget scaling
  • Remove unused layout modifications now that the layouts are different
  • Apply dynamic font sizing to every single widget with text in the positioner row widget
  • Add the low/high limit position settings to the expandable panel below the row widget
  • Allow the setpoint widget to expand into the space left by the tweak widget for the combobox case
  • Rework/refactor the expandable signal panel to be a pop-out window (the new RowDetails widget) on top of the screen instead of inside of the main layout, because adding a huge widget to the layout causes serious issues with resizing.
  • Remove references to static "Error" text that is removed in the row widget ui file

test_dynamic_font.py

  • Make sure combobox font patching works

PositionerBase.embedded.ui

  • Set minimum size, set size policy to be expanding so that the row is allowed to expand

positioner.ui

  • Remove all instances of fixed font-size that interfere with dynamic text resizing
  • Remove all alarm-sensitive borders because I had to disable a widget style reload earlier

positioner_row.ui

  • Completely redesign this to be aesthetically scalable in all directions
  • Small note: made the clear_error button smaller to save vertical space
  • Small note: switched squares to circles because the square widgets resize terribly

Motivation and Context

How Has This Been Tested?

  • Locally
  • Added a little bit of testing
  • In person test at RIX

Where Has This Been Documented?

Need to revise the pre-release notes one more time

Screenshots:

Before small:
image

Before wide:
image

After small:
image

After wide:
image

After default size with default size/location expansion pop-outs:
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

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 7, 2024

I'm going to finish up the testing and documentation todo stuff and then I'm going to adjust how I implemented the dynamic sizing. I've realized some basic tricks for making these sorts of composite widgets cleanly rescaleable in all directions while maintaining proportions and while only needing specification in the ui file (rather than my current code here which does a lot of extra stuff in the py files that it shouldn't need to if the ui is set up correctly). In essence: the only sizing rule that we'll need to keep track of in code is the dynamic font resizing, which I plan to attach to all of the text widgets instead of just some of them.

I might also take a stab at fixing the resizing for the square widget while I'm here if I'm feeling motivated.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

I like where this is at now

I didn't quite get to implementing the splitters idea, because the way the basic and composite signal panels are implemented (as a QWidget that is also itself a QGridLayout) makes this somewhat annoying to implement. There will need to be a sizable refactor to achieve this. (Now: add rows to a grid layout that is actually a typhos panel, later: add rows to a splitter area? And maintain the grid sizing? And only have splitters between subdevices, not between signals?)

The splitters idea will become an issue for implementing later since I think it would be helpful in a lot of cases.

I am going to clean up the description for documentation purposes but I don't plan on modifying the code again until next PR unless there are code quality issues or bugs. I think after this PR I'll switch to going through the most recent typhos issues.

@ZLLentz ZLLentz changed the title ENH: sizing usability fixes and tweaks ENH: positioner row widget sizing usability fixes and tweaks Jun 12, 2024
@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

This is now reviewable so I'm going to mark as ready for review. I still will revise the pre-release notes one more time.

@ZLLentz ZLLentz marked this pull request as ready for review June 12, 2024 17:19
@ZLLentz ZLLentz requested review from tangkong and patoppermann June 12, 2024 17:19
@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

I also want to make a companion PR on pcdsdevices, I'll try not to forget. This works by itself but there's a couple more things to tweak (such as the ??? rendering of the disabled signal)

@tangkong
Copy link
Contributor

wowee this is a biggun

A few notes from interactive testing:

  • Largely everything rendered fine on my end. I'm not entirely sure how to mock high-dpi displays, maybe I can petition for a nice big monitor 🤔
  • Some strange clipping in non-positioner widgets. On master this happens bug can be fixed by making the window taller, but with the changes in this PR I can't seem to reveal the rest of the readouts.
    • image
  • (Unrelated to this PR) Generally these windows rendered too tall for my monitor, with no ability to scroll to the bottom. On mac at least the only resize slider is at the bottom right, making it impossible to resize. Even when I can resize it, the minimum widget size often doesn't let me. I wonder if it's a scroll area angle, where we limit the size of the initial window to the monitor size
  • I like the popouts. I do wonder if we'll run into a problem of these windows getting left behind when the user clicks on the main window, bringing it into focus. If you open one of these then close the parent typhos widget, it does stay alive (and produces a segfault). One way to fix this is by putting this widget in a QMenuAction, which we could set to close when it loses focus. Otherwise I suspect properly parenting this widget might be the solution
  • (immensely minor nitpick) It might just be me, but I feel like a thin horizontal line between the positioner widgets would be helpful. As is the top and bottom of the widget-group doesn't really have a clear line, so it can be ambiguous whether the "clear error" and "status" buttons might belong to the group either above or below it.

I'll give the code a read through now

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Hard to argue with the results here. I pointed out a few things I saw

typhos/dynamic_font.py Outdated Show resolved Hide resolved
typhos/positioner.py Outdated Show resolved Hide resolved
@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

I'm not entirely sure how to mock high-dpi displays

I've tried using a VM for this in the past with mixed results. We have a new "like a hutch" test area coming online soon thankfully. I should double-check the final version of this in RIX before merging.

Some strange clipping in non-positioner widgets

I need to investigate and resolve this prior to merging, this is a blocker.

windows rendered too tall for my monitor, with no ability to scroll to the bottom

This is related to #599 and I'm planning to fix it before the tag but not during this PR. There's a hotfix for it in dev_conda.

windows getting left behind

This is a valid concern and should be addressed before merging. Parenting the widgets properly actually made them disappear... which confused me.

a thin horizontal line between the positioner widgets would be helpful

Vincent also suggested this at some point and I agree. I had been intending to simply use the splitters but since I punted on it maybe I need to address that now.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

One more dumb bug I need to resolve: the text snaps back to minimum size before and after a move
image

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

Another dumb "only while moving real things" bug: the combobox text gets repeatedly bigger and bigger and each time you click on it... bigger once the first time you click on it
image

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

Text getting small is because pydm is manually resetting the style on certain metadata events.
It seems like the only way around this is to also set the stylesheet to have the font specifier in it, so I'll try that.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

Setting the stylesheet fixed both the big big font and the tiny tiny font issues

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

The only remaining fix I'm going to do here is to address:

Some strange clipping in non-positioner widgets

After that, I'll round up the remaining items into tickets:

  • thin lines between positioner rows
  • splitters between devices
  • extra space underneath the title widget

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 12, 2024

side-by-side of old and new which shows off the sizing updates and shows how much is being clipped with the bug
image

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 13, 2024

Left: fixed clipping in default view
Right: expanding to +config view shows more and dynamically resizes
Changing the views to remove items also shrinks the widgets to remove the whitespace
image

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 13, 2024

There's only one more situation where the sizing is wrong- when the signal panel has no entries in it at all. Maybe this becomes a ticket to fix later, too. (I'll try a bit before today ends)

@ZLLentz
Copy link
Member Author

ZLLentz commented Jun 13, 2024

Remaining issue is that subdevices of subdevices don't collapse properly when vanished... my first try at that one caused more issues so this might be where I leave it for now.

Copy link
Contributor

@tangkong tangkong 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 looks good on the whole. We have some followups, but this is a clear improvement

@ZLLentz ZLLentz merged commit 86ef472 into pcdshub:master Jun 18, 2024
11 checks passed
@ZLLentz ZLLentz deleted the fix_failed_resizes branch June 18, 2024 20:40
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