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 support for {enabled/visible}_when for Tabbed and VFold #1705

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

aaronayres35
Copy link
Contributor

@aaronayres35 aaronayres35 commented Jun 28, 2021

This will close #758

Note this is effectively a new feature (so clearly a change in behavior).
For example, running the example given in the issue replacing visible_when with enabled_when, on master you see:
enabled_master
whereas on this branch you see:
enabled_branch

I'm a very slightly wary of this change... Although I believe the new behavior is "correct", users may be surprised by this...

Similarly, on this branch when running the following slightly modified version of the example from the issue:

Modified Example
from traits.api import Int, Float, HasTraits
from traitsui.api import (Group, HGroup, Item, Tabbed, VGroup, View)


class Foo(HasTraits):

    a = Int
    b = Float
    
    view = View(
        Tabbed(
            VGroup(
                HGroup(
                    Item('a')
                ),
                label='Tab #1',
                visible_when='object.b != 0.0',  # THIS IS THE CHANGED LINE
                id="first_tab"
            ),
            VGroup(
                HGroup(
                    Item('b')
                ),
                label='Tab #2',
                visible_when='True',
                id="second_tab"
            ),
        )
    )

if (__name__ == '__main__'):
    foo = Foo()
    foo.configure_traits()

This is what I now see:

Screen Recording
visible_when.mov

As you can see in the video there is still some weirdness (looking at the label for the text field for B, it looks like an A gets written over it)... Not sure what is going on here. But nonetheless this works as we want it to. (also note that goes away when you switch tabs and back)

As far as implementation goes, I took a very similar approach to this PR: #1544
which in turn was simply following what is done in the standard enabled/visible_when machinery from in the traitsui.ui.UI class. The repetition is a little ugly, but each case is unique and sort of needs to be handled on their own. The general approach of keeping a list of things that we want to be enabled/disabled or visible/invisible, and then setting up an any trait change handler to call our particular call back that toggles visibility/enabledness I believe is a good approach (its at least worked elsewhere as mentioned).

Checklist

  • Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

self.container, method_to_call_name
)
# check that the tab for this widget is already showing
if self.container.indexOf(widget) != -1:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

theoretically, if the widget is visible, idx == self.container.indexOf(widget). However, perhaps the tab could change its index during the lifetime of the tab widget, in which case we should call method_to_call(self.container.indexOf(widget)) instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this comment raises a significant issue that needs to be considered: the index of the widget in the Tabbed group doesn't necessarily match the index of the Qt tab, because visibility is controlled by adding/removing the widget from the container.

I think things will go wrong in situations like the following:

  • have tabs T0, T1, T2, T3, all initially visible
  • T1 is hidden, so we get T0, T2, T3, but now the indices of T2 and T3 at the Qt level are 1 and 2, but at the TraitsUI level are 2 and 3.
  • If we hide T2 in this state, the current code will actually hide T3 (and hiding T3 will result in an error?)
  • After hiding T2 we have T0 and T2 at the Qt level. If we un-hide T2, then it will try to insert it at Qt index 2 with the current code, and so we would get no change as it is already showing

I think there are situations where you can get things showing up in pretty much arbitrary order (eg. I think hide T2, hide T1, show T2 ends up with T0, T3, T2 as the layout).

I could be missing something about indices are being computed, but it looks like you can't assume that TraitsUI and Qt-level indices match, and computing the right Qt level index for re-insertion needs some thought (I think we need to work out how many TraitsUI tabs before the tab of interest are visible).

@aaronayres35 aaronayres35 changed the title [WIP] add support for {enabled/visible}_when for Tabbed Add support for {enabled/visible}_when for Tabbed and VFold Jun 28, 2021
Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

I think that the logic around the correspondence between Qt and TraitsUI indices can go wrong in complex scenarios of showing/hiding tabs

self.container, method_to_call_name
)
# check that the tab for this widget is already showing
if self.container.indexOf(widget) != -1:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this comment raises a significant issue that needs to be considered: the index of the widget in the Tabbed group doesn't necessarily match the index of the Qt tab, because visibility is controlled by adding/removing the widget from the container.

I think things will go wrong in situations like the following:

  • have tabs T0, T1, T2, T3, all initially visible
  • T1 is hidden, so we get T0, T2, T3, but now the indices of T2 and T3 at the Qt level are 1 and 2, but at the TraitsUI level are 2 and 3.
  • If we hide T2 in this state, the current code will actually hide T3 (and hiding T3 will result in an error?)
  • After hiding T2 we have T0 and T2 at the Qt level. If we un-hide T2, then it will try to insert it at Qt index 2 with the current code, and so we would get no change as it is already showing

I think there are situations where you can get things showing up in pretty much arbitrary order (eg. I think hide T2, hide T1, show T2 ends up with T0, T3, T2 as the layout).

I could be missing something about indices are being computed, but it looks like you can't assume that TraitsUI and Qt-level indices match, and computing the right Qt level index for re-insertion needs some thought (I think we need to work out how many TraitsUI tabs before the tab of interest are visible).

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.

visible_when=False in Vgroup Tabbed still visible
2 participants