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

Add LayoutWidget class and use it for newer widget classes #1016

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

corranwebster
Copy link
Contributor

This adds infrastructure below IWidget for widgets that have opinions about sizing; in particular should they be allowed to expand, what weight should they have compared to other widgets when expanding, and are there bounds on how big they can get? This is useful for widgets which are embedded in layouts and containers.

The PR provides an interface for this, an implementation for both toolkits, and makes this a superclass for IField and IDataViewWidget for concreteness. Other widgets will be added tot he system in a separate PR.

This PR is working towards #738 (in that LayoutWidget instances are the contents of IContainers) and a general layout system for Pyface. It also is a requirement for being able to directly use Pyface widgets in TraitsUI editors without needing any toolkit-specific code.

@rahulporuri rahulporuri self-requested a review August 16, 2021 09:30
@rahulporuri
Copy link
Contributor

@corranwebster CI is failing on this PR. Can you take a look at the test failures?

@corranwebster
Copy link
Contributor Author

corranwebster commented Aug 16, 2021

Arg. Didn't test with PySide2. I know what the issue likely is (it's to do with me trying to be clever and use Python enum.Enum for constants).

@corranwebster
Copy link
Contributor Author

Had a deeper look yesterday, and this looks like a more fundamental issue around PySide2 - for some reason setting the size policy is failing to actually change the size policy in many instances.

@corranwebster
Copy link
Contributor Author

Looks like it was an issue with enums: while PyQt5 will accept arguments which are integer subclasses that match the constant value (like you get from IntEnum), PySide2 appears to require actual constant values. This is resolved by using an Enum and asking for the .value.

Hopefully CI should now pass.

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Mostly LGTM with a number of suggestions and a few questions.

self.control = self._create_control(self.parent)
self._initialize_control()
self._add_event_listeners()
super()._create()
Copy link
Contributor

Choose a reason for hiding this comment

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

does this PR depend on #1013 ? It looks like this PR makes some of the changes that are included in #1013. Is that intentional? Do the changes in this PR depend on #1013 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't depend on #1013 because it adds a new class between these classes and the base Widget class which doesn't call super.

Now that #1013 is merged we will want to add that super call, but it can safely be in another PR if needs be (or if I make any modifications to this I will add it in to this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've merged in main and resolved all of this.

pyface/i_layout_widget.py Outdated Show resolved Hide resolved
pyface/testing/widget_mixin.py Outdated Show resolved Hide resolved
from pyface.ui.qt4.widget import Widget


#: Maximum widget size (some versions of PyQt don't export it)
Copy link
Contributor

Choose a reason for hiding this comment

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

PyQt or PyQt5? We don't need to add new code with support for PyQt as we've dropped support for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's some but not all versions of PyQt5 - I don't remember which.

class LayoutWidget(MLayoutWidget, Widget):
""" A widget which can participate as part of a layout. """

def _set_control_minimum_size(self, size):
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not clear from the MLayoutWidget documentation that size needs to be a tuple where the first and second elements are width and height - although that might be the default behavior for most/all toolkits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not too concerned about that as this API is only used by toolkit implementers. End-users will use the minimum_size trait which is set to the equivalent to Tuple(Int, Int).

Comment on lines -42 to -46
def _initialize_control(self):
""" Perform any toolkit-specific initialization for the control. """
self.control.SetToolTip(self.tooltip)
self.control.Enable(self.enabled)
self.control.Show(self.visible)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not clear to me why Field.initialize_control can be removed here nor do i understand why Field._create and Field.destroy can be removed either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are now handled without any toolkit-specific code in the base MField class (via _set_control_tooltip, etc.)

The _create and _destroy aren't needed because their only purpose was to call _add_event_listeners and _remove_event_listeners which is now done in the _create and _destroy methods of MLayoutWidget.

return self.size_policy


def _size_to_wx_size(size: t.Tuple[int, int]) -> wx.Size:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason why you made use of typing here? We haven't used it in other parts of the codebase or in other ETS libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an experiment on my part to see if it adds anything to use explicitly typed code and to play with patterns for doing so. This seemed like a fairly harmless corner to try it in, and one where the constraints are fairly clear.

Typing seems to be catching on, so I think we may well end up with our codebases being typed in the 2-3 year timeframe, so experiments are valuable.

If you feel strongly it should go, I will remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it. At some point I may try with a more self-contained library.

@corranwebster corranwebster merged commit 5c50f3a into main Sep 8, 2021
@corranwebster corranwebster deleted the enh/layout-widget-basics branch September 8, 2021 13:09
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