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 }})

irgolic

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

@irgolic irgolic changed the titlegui: Set maximum autocommit btn height gui: Keep Macstyle autocommit, allow boxes to expand

Feb 5, 2021

@irgolic irgolic changed the titlegui: Keep Macstyle autocommit, allow boxes to expand Widget sizeHint, button/box sizePolicy fixes, rubber

Feb 8, 2021

@irgolic irgolic changed the titleWidget sizeHint, button/box sizePolicy fixes, rubber Widget sizeHint, button/box sizePolicy fixes

Feb 8, 2021

@ajdapretnar

I don't think Time Slice has this option anymore.

irgolic

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)

markotoplak

markotoplak

markotoplak

markotoplak

@markotoplak

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.

@markotoplak

I can merge this PR if:

ales-erjavec

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

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

ales-erjavec

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.

@irgolic

@irgolic

@irgolic

@irgolic

@irgolic

@irgolic

@irgolic

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.

@irgolic

Also fixup some spacing/margins

@irgolic

This was referenced

Feb 18, 2021