Pivot
  1. Pivot
  2. PIVOT-751

TabPaneSelectionListener#selectedIndexChanged called twice when first tab is inserted

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.0.1
    • Fix Version/s: 2.0.2
    • Component/s: wtk
    • Labels:
      None

      Description

      If you add a tab to an empty tabpane, two selectedIndexChanged events are called. I.e, when you add a TabPaneSelectionListener to a tabpane and call:

      tabpane.getTabs().add(component);

      .. the listener fires twice, with previousSelectedIndex set to -1 and then 0.

      The two events are fired by line 68 and 72 in TabPane.java:

      68 tabPaneListeners.tabInserted(TabPane.this, index);
      69
      70 // Fire selection change event, if necessary
      71 if (selectedIndex != previousSelectedIndex)

      { 72 tabPaneSelectionListeners.selectedIndexChanged(TabPane.this, selectedIndex); 73 }

      This could be fixed by taking into account that previousSelectedIndex might have been -1 when invoked, like this:

      if (selectedIndex != previousSelectedIndex && previousSelectedIndex > -1) {

      See this thread for more info: http://apache-pivot-users.399431.n3.nabble.com/TabPaneSelectionListener-selectedIndexChanged-called-twice-when-first-tab-is-inserted-td3022515.html

      PS: According to Chris, CardPane and Accordion behave in the same way, so any changes to TabPane should also be made to these for consistency.

      1. PIVOT751.java
        2 kB
        Chris Bartlett
      2. PIVOT751.txt
        1 kB
        Chris Bartlett

        Activity

        Edvin Syse created issue -
        Sandro Martini made changes -
        Field Original Value New Value
        Fix Version/s 2.0.1 [ 12315951 ]
        Hide
        Chris Bartlett added a comment -

        Does anyone have any comments on this?

        According to the javadocs, the previousSelectedIndex value of the selectedIndexChanged event will represent the previously selected index or the currently selected index depending on whether the change was direct/indirect.
        http://pivot.apache.org/2.0/docs/api/org/apache/pivot/wtk/TabPaneSelectionListener.html#selectedIndexChanged(org.apache.pivot.wtk.TabPane, int)

        I feel that a single event should be fired as there is only actually one change of the selected index value, but am not sure which of the two it should be. My initial thought was that it should be the first event, but after reading the javadoc, think it should be the second one.

        The first event that is currently fired has a previousSelectedIndex value of -1. This is the actual previous selection index value, and according to the javadocs, should arise from a direct/explicit selection change.

        The second event that is currently fired has a previousSelectedIndex value of 0. This is the current selection index value, and according to the javadocs, should arise from an indirect selection change. That certainly seems to describe this scenario as no explicit selection change was made, rather a new tab was added.

        So perhaps it should actually be the first event that is suppressed rather than the second one?

        Show
        Chris Bartlett added a comment - Does anyone have any comments on this? According to the javadocs, the previousSelectedIndex value of the selectedIndexChanged event will represent the previously selected index or the currently selected index depending on whether the change was direct/indirect. http://pivot.apache.org/2.0/docs/api/org/apache/pivot/wtk/TabPaneSelectionListener.html#selectedIndexChanged(org.apache.pivot.wtk.TabPane , int) I feel that a single event should be fired as there is only actually one change of the selected index value, but am not sure which of the two it should be. My initial thought was that it should be the first event, but after reading the javadoc, think it should be the second one. The first event that is currently fired has a previousSelectedIndex value of -1. This is the actual previous selection index value, and according to the javadocs, should arise from a direct/explicit selection change. The second event that is currently fired has a previousSelectedIndex value of 0. This is the current selection index value, and according to the javadocs, should arise from an indirect selection change. That certainly seems to describe this scenario as no explicit selection change was made, rather a new tab was added. So perhaps it should actually be the first event that is suppressed rather than the second one?
        Hide
        Chris Bartlett added a comment -

        It might also be worth changing the name of the 'previousSelectedIndex' parameter to something else to that better reflects that its value might not actually be the previously selected index, but the current selected index instead.

        Any such change should be made to other XXXSelectionListener interfaces for consistency.

        Show
        Chris Bartlett added a comment - It might also be worth changing the name of the 'previousSelectedIndex' parameter to something else to that better reflects that its value might not actually be the previously selected index, but the current selected index instead. Any such change should be made to other XXXSelectionListener interfaces for consistency.
        Hide
        Chris Bartlett added a comment -

        Simple test app and its generated output

        Show
        Chris Bartlett added a comment - Simple test app and its generated output
        Chris Bartlett made changes -
        Attachment PIVOT751.txt [ 12486566 ]
        Attachment PIVOT751.java [ 12486567 ]
        Hide
        Chris Bartlett added a comment -

        One other thing to note...

        When a tab is added to a previously empty TabPane, the tab will be selected as it is the only tab.
        However if a TabPane has 2 tabs, and the selected one is removed, the only remaining tab is not selected.
        Any time that a selected tab is removed, it is up to the application developer to choose which tab (if any) should be selected. If this is not done, no tabs will be selected.

        Show
        Chris Bartlett added a comment - One other thing to note... When a tab is added to a previously empty TabPane, the tab will be selected as it is the only tab. However if a TabPane has 2 tabs, and the selected one is removed, the only remaining tab is not selected. Any time that a selected tab is removed, it is up to the application developer to choose which tab (if any) should be selected. If this is not done, no tabs will be selected.
        Hide
        Edvin Syse added a comment -

        Yes, I was also quite surprised that there were no default tabclosing-policy. It's easy to deal with, but kind of strange that there is no default behavior. Every other GUI toolkit I know of defaults to showing another tab, and others even provide different tabclosing-policies to choose from.

        Show
        Edvin Syse added a comment - Yes, I was also quite surprised that there were no default tabclosing-policy. It's easy to deal with, but kind of strange that there is no default behavior. Every other GUI toolkit I know of defaults to showing another tab, and others even provide different tabclosing-policies to choose from.
        Hide
        Chris Bartlett added a comment - - edited

        I did have quick look into it a while ago, but couldn't think of a clean way to implement it without too many changes.
        This was due to handling of disabled tabs, and enabled tabs that might be prevented from being selected by veto logic in the 'previewSelectedIndexChange' event.
        It seemed simpler to just allow the developer to consolidate their logic in one place and then reference it in the 'previewSelectedIndexChange' events and 'tabsRemoved' events as required.

        Feel free to have another shot at it though!

        Show
        Chris Bartlett added a comment - - edited I did have quick look into it a while ago, but couldn't think of a clean way to implement it without too many changes. This was due to handling of disabled tabs, and enabled tabs that might be prevented from being selected by veto logic in the 'previewSelectedIndexChange' event. It seemed simpler to just allow the developer to consolidate their logic in one place and then reference it in the 'previewSelectedIndexChange' events and 'tabsRemoved' events as required. Feel free to have another shot at it though!
        Hide
        Greg Brown added a comment -

        As noted, the bogus event is fired because TerraTabPaneSkin sets the selected index in response to the initial tab insert event, and then TabPane fires an additional event based on incorrect information.

        This represents an interesting design challenge. It is similar to the challenge of keeping a ListView's model and selection state in sync. In Swing, this is handled by the UI delegate (i.e. the skin). When the model fires events, the skin is responsible for updating the component's selection state.

        We intentionally chose not to do it this way since it seemed like the component should be responsible for maintaining its own state internally. However, after some further thought I'm wondering if the Swing approach might be better. We had initially viewed the skin as the "view" and the component as the "controller" - however, in retrospect, I think the component is actually the "view-model", and the skin is the "view-controller". The component is part view because it is the object that actually receives user input and paint calls (though it delegates them to the skin). It is part model since it generally hosts the model (either as an intrinsic property like "buttonData" or as an attached model like "listData"). The skin is part view because it actually handles the paint calls, and part controller since it handles user input and other events and updates the component's (i.e. model's) state in response to them.

        When viewed as a controller, it seems valid for the skin to be responsible for keeping the component's various models in sync. Since there will never be a practical case where a component does not have a skin installed, this seems OK to me. It means that, in this case, TabPane wouldn't fire an indirect selection change event when a tab is inserted - it would be up to the skin to respond to that event appropriately (which it already does). The skin should also do something similar when a tab is removed.

        What do you think? FYI, this would be a big change, as it affects the design of many components. However, I think it could probably be restricted to TabPane, Accordion, and CardPane for now and propagated to ListView, TableView, etc. later.

        Show
        Greg Brown added a comment - As noted, the bogus event is fired because TerraTabPaneSkin sets the selected index in response to the initial tab insert event, and then TabPane fires an additional event based on incorrect information. This represents an interesting design challenge. It is similar to the challenge of keeping a ListView's model and selection state in sync. In Swing, this is handled by the UI delegate (i.e. the skin). When the model fires events, the skin is responsible for updating the component's selection state. We intentionally chose not to do it this way since it seemed like the component should be responsible for maintaining its own state internally. However, after some further thought I'm wondering if the Swing approach might be better. We had initially viewed the skin as the "view" and the component as the "controller" - however, in retrospect, I think the component is actually the "view-model", and the skin is the "view-controller". The component is part view because it is the object that actually receives user input and paint calls (though it delegates them to the skin). It is part model since it generally hosts the model (either as an intrinsic property like "buttonData" or as an attached model like "listData"). The skin is part view because it actually handles the paint calls, and part controller since it handles user input and other events and updates the component's (i.e. model's) state in response to them. When viewed as a controller, it seems valid for the skin to be responsible for keeping the component's various models in sync. Since there will never be a practical case where a component does not have a skin installed, this seems OK to me. It means that, in this case, TabPane wouldn't fire an indirect selection change event when a tab is inserted - it would be up to the skin to respond to that event appropriately (which it already does). The skin should also do something similar when a tab is removed. What do you think? FYI, this would be a big change, as it affects the design of many components. However, I think it could probably be restricted to TabPane, Accordion, and CardPane for now and propagated to ListView, TableView, etc. later.
        Hide
        Chris Bartlett added a comment -

        > We had initially viewed the skin as the "view" and the component as the "controller" - however, in retrospect, I think the component is actually the "view-model", and the skin is the "view-controller"
        That is exactly how I had ended up seeing their relationships. And for pretty much the same reasoning as you too.

        Show
        Chris Bartlett added a comment - > We had initially viewed the skin as the "view" and the component as the "controller" - however, in retrospect, I think the component is actually the "view-model", and the skin is the "view-controller" That is exactly how I had ended up seeing their relationships. And for pretty much the same reasoning as you too.
        Hide
        Sandro Martini added a comment -

        This requires more tests, so move to 2.0.2

        Show
        Sandro Martini added a comment - This requires more tests, so move to 2.0.2
        Sandro Martini made changes -
        Fix Version/s 2.0.2 [ 12317574 ]
        Fix Version/s 2.0.1 [ 12315951 ]
        Affects Version/s 2.0.1 [ 12315951 ]
        Sandro Martini made changes -
        Assignee Sandro Martini [ smartini ]
        Hide
        Sandro Martini added a comment -

        Resolved, for insert and remove operations, on: TabPane, CardPane , Accordion.

        Show
        Sandro Martini added a comment - Resolved, for insert and remove operations, on: TabPane, CardPane , Accordion.
        Sandro Martini made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Sandro Martini
            Reporter:
            Edvin Syse
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development