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

Hotfix: remove all special resize handling #15

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Nov 8, 2024

Original PR Text

Related to https://jira.slac.stanford.edu/browse/ECS-6595

Removing the special resize handling has the following effects:

  • Some of the crash on resize problems go away
  • Sometimes the gui looks weird and needs to be resized again

I suspect we don't actually want to remove this entirely, but it will take some back and forth to figure out which parts to cut out.

I'll deploy this hotfix as a dev version with the intention of making a cleaner tag after giving this some more attention.

Update After Testing

This worked well in testing and in XCS.
Comparing an old window with the custom resize logic vs the new window without it side-by-side, I noticed the following differences:

  • With the old special resize handling, I noticed much more "magic" extra ajustments after actions like disabling/enabling projections
  • In some cases this actually bounces and resizes multiple times before settling down
  • Certain basic things like changing cameras are measurably faster after the patch (since fewer things are happening behind the scenes now)
  • The application sizing seems much more stable without this
  • The application resize window controls no longer get "stuck" after my patch

Notes from analyzing the old code

The following things aside from resizeEvent internals were encapsulated inside start/finish resize blocks:

  • All initial setup during init
  • Showing or hiding the projections
  • Showing or hiding the markers
  • Showing or hiding the config
  • Switching cameras, including all the PV connections
  • Whenever the image changes size

The start/finish resize blocks presumably had the following effects:

  • start: if count is zero, set the top-level layout's size constraint to fixed size, set resizing to true
    regardless of count, increment count
    effectively, this blocks you from resizing the window at all during one of these time spans
  • finish: decrement count
    if count is zero, call adjustSize

adjustSize: "Adjusts the size of the widget to fit its contents."

Then presumably this always enters resizeEvent, which could also be entered by the user resizing the window.

There's no real protection against the count getting in a bad state, against two threads doing finish resize at the same time and both running adjust size, or multiple resizeEvents getting layered on top of each other, but maybe these always happen in sequence so it's ok

Anecdotally, sometimes the screen resize controls lock up, so something goes wrong in some cases.

resizeEvent -> completeResize
(Note: this drops the normal return value on the floor)

  • Undo fixed size constraint, back to default constraint
  • If we're not inside a start/finish resize block, this does a really basic resize of the image
  • Otherwise it does some strange adjustments, not fully explained
  • Either way it calls some of the custom c code in here
  • It seems like it would be possible for this to loop infinitely

Looking at the first crash now:

  • One is inside ui impl's "setImageSize", which is called during "onSizeUpdate", "connectCamera", and "changeSize"
  • "changeSize" is the only one that gets called inside the resizeEvent

Second crash:

  • Is inside "dumpConfig", which is called from many places but also from "changeSize"
  • I wonder if this or the other one can get layered and be called twice on top of itself

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 20, 2024

@tangkong @janeliu-slac
The diff here is pretty "straightforward" in that I'm just removing code.

I did a best-effort look at understanding what the old (crashing) code was doing and included my efforts in the PR description.

I'd appreciate any sanity checks that:

  • My understanding of what the old code is doing isn't completely wrong
  • The old code is also confusing to you (and if not, I'd love help understanding it)

@ZLLentz ZLLentz marked this pull request as ready for review November 20, 2024 00:07
@tangkong
Copy link

500 line init methods should be illegal.

On a higher level, I think this is what happens when you try to do the resizing yourself instead of setting size policies and letting Qt do it for you. I think the intent of the old code is clear, but the pitfalls are numerous and as you describe.

@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 20, 2024

I have to wonder if some of this is solving issues from the qt4 era that no longer apply....

@janeliu-slac
Copy link

I haven't worked with the camViewer or Qt, so I don't feel qualified to comment on the code at all. Going to pass on this one but will read and try to understand what camviewer_ui_impl.py is doing.

Copy link

@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.

This is a place where my convention of only allowing CamelCase for qt override methods is missed: It's hard to track exactly where we're invoking qt api vs creating our own methods.

I think disabling this is probably the prudent decision, in the future maybe a better approach is a better size hints / spacers / scroll areas?

I realize we do have a ui file, but the window seems rather intolerant to resizing

if self.rcnt == 0:
self.adjustSize()

def resizeEvent(self, ev):

Choose a reason for hiding this comment

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

This is an event handler that is called every time the widget is resized https://doc.qt.io/qt-6/qwidget.html#resizeEvent

This is probably also being called when programmatic resizes are happening, further adding {start. finish}Resize calls to the stack.


def completeResize(self):
self.layout().setSizeConstraint(QLayout.SetDefaultConstraint)
self.setMaximumSize(QSize(16777215, 16777215))

Choose a reason for hiding this comment

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

Note that here we're setting sizes and not SizeHints, another place where I imagine there's cross talk / competing instructions

def delayedRetry(self):
# Try the resize again...
self.startResize()
self.layout().invalidate()

Choose a reason for hiding this comment

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

I've seen guidance by qt to .activate() after .invalidate(), but also to usually avoid manually invalidating your layouts. (ui updates should happen automatically).

https://doc.qt.io/qt-6/qlayout.html#activate

I lost the tab that had this guidance, so here's a SO post along those lines https://stackoverflow.com/questions/2427103/qt-how-to-force-a-hidden-widget-to-calculate-its-layout

Choose a reason for hiding this comment

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

This application never calls .activate

Copy link

@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'll go on the record as approving, since I think the hotfix is valid since it works int he field. Most of my comments here will be musings/annoyances with Qt

@ZLLentz ZLLentz merged commit bee2eea into pcdshub:master Nov 20, 2024
@ZLLentz
Copy link
Member Author

ZLLentz commented Nov 20, 2024

Ok, thank you! This is easy enough to bring back if needed.
I'm hoping to get user feedback prior to tag but likely instead I'll just make a tag and get feedback later 🤷

@ZLLentz ZLLentz deleted the hotfix_no_special_resizing branch November 20, 2024 01:29
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