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

Label {visible/enabled}_when #1544

Merged
merged 8 commits into from
Apr 23, 2021
Merged

Label {visible/enabled}_when #1544

merged 8 commits into from
Apr 23, 2021

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Mar 19, 2021

closes #298

This PR is miles away from being ready for a review. At this point it is basically chicken scratch to try and get something to work. I would like to have a discussion about possible approaches for how to best solve the issue. The current start of an implementation here I don't image will be how we wish to proceed long term.

Nonetheless, the solution here for qt only seems to work as expected, at least for the simple examples I have tried, eg. the ones described on the issue. I effectively tried to recreate the approach used to handle {visible/enabled}_when in traitsui.ui.py only to match the slightly different scenario.

This PR provides a solution for having Labels respect their enabled_when and visible_when traits. The solution on qt works exactly as expected.

I had a hiccup with wx:
basically with wx if you try to do the self._label_when() call immediately, if one of the labels has a visible_when expression evaluating to False initially, it will never be able to toggle back on. see comments on the code changes for more details. Thus, currently the enabled_when/visible_when could start incorrect but as soon as any trait in the uis context changes they are then correct/will remain in sync.

There is no unittest here (I really struggled trying to write one for this code). I can open an issue to add one

@rahulporuri
Copy link
Contributor

orthogonal to this change, i'm really interested in refactoring the _add_items method to make it a lot easier to read and understand, potentially by creating other private methods which get used by _add_items e.g. _add_labels which handles the case where an item is a Label.

this might make for an interesting pair programming session.

@corranwebster
Copy link
Contributor

The layout generation code is a continuing source of problems - it is opaque, custom for each backend, and not dynamic. The right way to fix this is to add a cross-toolkit abstraction of layout, tabs, splitters, etc. and have them controlling the layout dynamically and accessible from the main UI object.

In the meantime, do the best you can with what we have :/

@aaronayres35
Copy link
Contributor Author

I am finding it quite difficult to write a regression test for this...
Labels are not Editors (hence the solution implemented in this PR!), but that means we cant access them easily via UITester in testing. Even further, we can manually access the Item objects for the labels, but those don't actually carry any reference to or info about the underlying widget. The widget get created inside traitsui.qt4.ui_panel._GroupPanel._add_label_item and is held in the temporary label variable there. This then gets added to the _GroupPanel layout by the call self._add_widget(inner, label, row, col, show_labels).
So, to access this widget in the test, we would first need to access the specific _GoupPanel object, and then iterate through the widgets in its layout to find the one we want to test.

I am unable to even get my hands on the correct _GoupPanel object. I can get a traitsui.qt4.ui_live._LiveWindow, traitsui.qt4.ui_base._StickyDialog, PyQt5.QtWidgets.QMainWindow, etc. It is extremely convoluted trying to follow the logic of what all is happening in traitsUI/Qt / to find the object I am looking for in such nested class structures.

I am hopeful I'm missing something trivial and this can be done in a much simpler way. For right now I think I am going to stop looking into this as I feel like I am making no progress on something which feels like it should be so simple.
If I test manually the solution very clearly works (I can see the label enable/disable and/or toggle its visibility)

@@ -1033,6 +1069,10 @@ def add_items(self, content, panel, sizer):
# Save the reference to the label control (if any) in the editor:
editor.label_control = label

if (len(self._label_enabled_whens) + len(self._label_visible_whens)) > 0:
for object in self.ui.context.values():
object.on_trait_change(lambda: self._label_when(), dispatch="ui")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally we would call self._label_when() after this for loop, just like we do in the qt version. The problem is, if we do this, if you run the following example:

from traits.api import HasTraits, Int, Str
from traitsui.api import View, Item, Label, HGroup

class A(HasTraits):
    x = Int(1)

    view = View(
        Label("Visible", visible_when="x != 0"),
        Label("Enabled", enabled_when="x != 0"),
        Item('x'),
        resizable=True
    )

if __name__ == '__main__':
    A().configure_traits()

you will never be able to see a "Visible" label, even if you change x to not be 0. (enabled works perfectly fine). I think the reason is that, if we set the label to invisible right off the bat, it doesn't get included in the layout or something like this. Then everytime in the future, showing/hiding it does nothing.

If we don't have self._label_when() (as is currently the case), then by default both the "Visible" and "Enabled" labels are shown at the start, (even though we shouldn't see "Visible" and "Enabled" should be greyed out). From then on whenever anything is changed on the UI they will get synced up with their enabled_when/visible_when expressions and work as desired.

This is not ideal, but better than just not working at all. I could not find a way to have the visible label be invisible to start, but then show up later if the visible_when condition ever got switched to True.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's convert this into an issue

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 opened #1593

@aaronayres35 aaronayres35 changed the title [WIP] Label {visible/enabled}_when Label {visible/enabled}_when Apr 21, 2021
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.

LGTM. I tested this on qt with the MRE in the issue and this solution works as expected.

Tangentially, it looks like we can continue refactoring the toolkit-specific ui_panel classes.

traitsui/qt4/ui_panel.py Show resolved Hide resolved
traitsui/wx/ui_panel.py Show resolved Hide resolved
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.

Label ignores visible_when
3 participants