Widget sizeHint, button/box sizePolicy fixes by irgolic · Pull Request #130 · biolab/orange-widget-base (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits21 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Issue
Widgets aren't visually consistent. It'll take a little bit of work in Orange3 and add-ons, but once we're there, everything will be nice. Most of the changes I made in the Orange3 PR simplify the code, I hope that's the case for addons too.
Description of changes
See commit history.
This was written on macOS 10.15.7, working with both QMacStyle and Plastic. Hoping Windows and Linux look ok.
Includes
- Code changes
- Tests
- Documentation
gui: Set maximum autocommit btn height gui: Keep Macstyle autocommit, allow boxes to expand
gui: Keep Macstyle autocommit, allow boxes to expand Widget sizeHint, button/box sizePolicy fixes, rubber
Widget sizeHint, button/box sizePolicy fixes, rubber Widget sizeHint, button/box sizePolicy fixes
I don't think Time Slice has this option anymore.
| else: |
|---|
| separator(widget) |
| warnings.warn("'addSpace' has been deprecated. Use gui.separator instead.", |
| DeprecationWarning, stacklevel=3) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better deprecate (someone test this pls I might be doing it wrong)
While I do agree that this branch combined with the PR in Orange3 makes Orange look nicer, I can not merge this. For me, there is too much new additional magic in miscellanea (allowing box_ and parent_ settings) and complicated and non-avoidable magical widget sizeHint.
Mac-specific changes also bother me somewhat, but if that is really needed to fix strange alignments, well, then fine. These I could actually merge...
@ales-erjavec, do you have any time to review this? If you say these code patterns are fine, then I'll accept them.
I can merge this PR if:
- There is an option to disable sizeHint.
- The new
box_,parent_, multiple_magic gets removed from miscelanea. Yes, sometimes we will have to use multiple lines of code instead of additional parameters... But if we could survive without these before, they do not seem to be needed often.
Comment on lines 1127 to 1119
| if button.style().metaObject().className() == 'QMacStyle': |
|---|
| btnpaddingbox = vBox(widget, margin=0, spacing=0) |
| separator(btnpaddingbox, 0, 4) # lines up with a WA_LayoutUsesWidgetRect checkbox |
| button.outer_box = btnpaddingbox |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you assume anything related to checkbox here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autocommit is a button next to a checkbox. Generally you want all buttons in a row (QHBoxLayout) at the same height, so it's set globally.
Without this, still keeping Qt.WA_LayoutUsesWidgetRect will misalign the checkbox and button.
@ales-erjavec, do you have any time to review this? If you say these code patterns are fine, then I'll accept them.
I don't like it.
I already cannot reason about what the existing code does. This would make it even worse.
But then again I advise against using gui at all and if this fixes existing uses (for Orange only?) then go ahead.
If you do remove addSpace (good) why then add addSpaceBefore (bad)?
I would keep some changes that fix layouts locally, I.e. the one that removes near global WA_LayoutUsesWidgetRect and moves it to the function where it is required, margins/spacings in created widgets/layouts, ...
Comment on lines 263 to 264
| if box and parent and box is not parent and parent.layout() and parent.layout().spacing() == -1: |
|---|
| parent.layout().setSpacing(8) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this. If you keep it add addToLayout to the if condition. But really remove this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MacStyle is weird. It doesn't use constant spacing, it uses this weird matrix to calculate spacing between two controls, depending on which one precedes the other. https://doc.qt.io/archives/qq/qq23-layouts.html
This works great if you only use control elements. But as soon as you nest controls in boxes (e.g., label + combo), the margins and spacing get really big. Our solution for this is to set the spacing on the box (if no spacing has yet been set), if you add a nested control. For this to work right, Qt.WA_LayoutUsesWidgetRect should be set on all the controls in the parent box.
... That was our rationale, but I've removed it, and I think we'll live without it.
Comment on lines 405 to 397
| if widget and widget.layout() and \ |
|---|
| isinstance(widget.layout(), QtWidgets.QVBoxLayout) and \ |
| not widget.layout().isEmpty(): |
| misc.setdefault('addSpaceBefore', True) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not add back another automagic to replace the addSpace one.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addSpaceBefore replaced addSpace, it's there to put separators between boxes, but not before the first box. I could make it private to the gui package, then you could disable it with addToLayout=False.
Initially this helped align checkboxes with checkboxes in their nested boxes (e.g. with a spinbox). But, this messes with MacStyle spacing, and makes everything a bit too spaced out. If this is necessary, put all controls in their own box, or set the attribute manually.
Also fixup some spacing/margins
This was referenced
Feb 18, 2021