Pivot
  1. Pivot
  2. PIVOT-548

Fire selection change events when selection changes indirectly

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0
    • Component/s: wtk
    • Labels:
      None

      Description

      Currently, selection change events are fired only when an explicit call has been made that affects the selection. For example, in ListView, calling either setSelectedRanges() or clearSelection() will fire this event. However, an operation that indirectly changes the selection state (such as adding or removing an item from the ListView's model data) does not trigger an event. This was originally done by design - selectedRangesChanged() includes the previous selection as an argument, and we didn't want to have to manually re-construct that every time the selection changed as a side effect of a model change:

      public void selectedRangesChanged(ListView listView, Sequence<Span> previousSelectedRanges);

      However, in practice, working within this model can be challenging. More than once I have registered a selection change listener expecting to receive notification of all selection changes, forgetting that it is not designed that way. I'm guessing that other developers may be confused by this as well.

      So, I am proposing that components that maintain a selection state also fire selection change events when the selection changes indirectly. In this case, a null value would be passed for the previous selection. This will save the effort of re-constructing the previous selection info and will give the listener additional information about the nature of the change (i.e. null == indirect state change).

      This change should also be propagated to TextInput, which has a similar issue with character change events. Currently, TextInput fires character change events via TextInputCharacterListener and text change events via TextInputTextListener. The textChanged() event does not pass the previous text value, which is inconsistent with other change events. textChanged() should be incorporated into TextInputCharacterListener and should pass the previous text value when it is changed via an explicit call to setText(); otherwise, it should pass null.

      The updated version of TextArea should probably follow the same approach.

        Activity

        Hide
        Greg Brown added a comment -

        TextInput has been updated to fire events as described above.

        Show
        Greg Brown added a comment - TextInput has been updated to fire events as described above.
        Hide
        Greg Brown added a comment -

        ListView and TableView have been updated.

        Show
        Greg Brown added a comment - ListView and TableView have been updated.
        Hide
        Greg Brown added a comment -

        ListButton and Spinner have been updated. Assigning to Todd for TreeView updates. Like ListView and TableView, TreeView should be updated to fire selectedPathsChanged() when the model data changes or a selected path is added/removed. In the first case (model data insert/remove), selectedPathsChanged() should be called with the value of getSelectedPaths() as the previousSelectedPaths argument. In the second case (selected path add/remove), the previousSelectedPaths argument should be null.

        Show
        Greg Brown added a comment - ListButton and Spinner have been updated. Assigning to Todd for TreeView updates. Like ListView and TableView, TreeView should be updated to fire selectedPathsChanged() when the model data changes or a selected path is added/removed. In the first case (model data insert/remove), selectedPathsChanged() should be called with the value of getSelectedPaths() as the previousSelectedPaths argument. In the second case (selected path add/remove), the previousSelectedPaths argument should be null.
        Hide
        Greg Brown added a comment -

        Need to update CardPane, Accordion, and TabPane as well.

        Show
        Greg Brown added a comment - Need to update CardPane, Accordion, and TabPane as well.
        Hide
        Greg Brown added a comment -

        Accordion, CardPane, TabPane, and SuggestionPopup have been updated.

        Show
        Greg Brown added a comment - Accordion, CardPane, TabPane, and SuggestionPopup have been updated.
        Hide
        Greg Brown added a comment -

        Resolved for TreeView.

        Show
        Greg Brown added a comment - Resolved for TreeView.

          People

          • Assignee:
            Todd Volkert
            Reporter:
            Greg Brown
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development