Uploaded image for project: 'Pivot'
  1. Pivot
  2. PIVOT-948

Allow selected key bindings to work with unselected items

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.0.4
    • Fix Version/s: 2.1, 2.0.5
    • Component/s: wtk
    • Labels:
    • Environment:
      All

      Description

      If you set a selected key binding (on a ListButton, ListView, TableView or Spinner), and invoke the "store" method when no item is selected, you can get an error about not being able to assign "null" to an "int" field (in my case).
      The problem is that "store" tests for nothing being selected and sets the item to null without even calling the binding method. In my case, I am using the binding to just store the selected index as an integer, so the item will not end up being null, except that the binding method is never called.
      So, the item value to be stored should be left up to the binding method, even in the "unselected" case in order to be consistent (and to allow my selected index bind mapping to work correctly, even in the unselected case).
      It will be more clear what the problem is when you see the patch file.

      1. binding.patch
        5 kB
        Roger Whitcomb

        Activity

        Hide
        rwhitcomb Roger Whitcomb added a comment -

        The logic is changed as follows:
        if (selectedIndex == -1)
        item = null;
        else

        { if (selectedKeyBinding == null) item = data.get(selectedIndex); else item = selectedKeyBinding.get(data, selectedIndex); }

        To:
        if (selectedKeyBinding == null)

        { if (selectedIndex == -1) item = null; else item = data.get(selectedIndex); }

        else

        { item = selectedKeyBinding.get(data, selectedIndex); }

        Which allows the binding method to completely decide for itself what to do with the -1 (unselected) index, instead of arbitrarily deciding that the value should be null.

        Show
        rwhitcomb Roger Whitcomb added a comment - The logic is changed as follows: if (selectedIndex == -1) item = null; else { if (selectedKeyBinding == null) item = data.get(selectedIndex); else item = selectedKeyBinding.get(data, selectedIndex); } To: if (selectedKeyBinding == null) { if (selectedIndex == -1) item = null; else item = data.get(selectedIndex); } else { item = selectedKeyBinding.get(data, selectedIndex); } Which allows the binding method to completely decide for itself what to do with the -1 (unselected) index, instead of arbitrarily deciding that the value should be null.
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Committed the patch to trunk:
        Sending wtk/src/org/apache/pivot/wtk/ListButton.java
        Sending wtk/src/org/apache/pivot/wtk/ListView.java
        Sending wtk/src/org/apache/pivot/wtk/Spinner.java
        Sending wtk/src/org/apache/pivot/wtk/TableView.java
        Transmitting file data ....
        Committed revision 1593659.

        Show
        rwhitcomb Roger Whitcomb added a comment - Committed the patch to trunk: Sending wtk/src/org/apache/pivot/wtk/ListButton.java Sending wtk/src/org/apache/pivot/wtk/ListView.java Sending wtk/src/org/apache/pivot/wtk/Spinner.java Sending wtk/src/org/apache/pivot/wtk/TableView.java Transmitting file data .... Committed revision 1593659.
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        This could be put into 2.0.4, but we already cut the tag for it ....

        Sandro, what do you think?

        Show
        rwhitcomb Roger Whitcomb added a comment - This could be put into 2.0.4, but we already cut the tag for it .... Sandro, what do you think?
        Hide
        smartini Sandro Martini added a comment -

        Hi Roger, after a quick look at the patch I think it's good. My only concern is on:
        > The logic is changed as follows:
        I'm sure now it's better, but are we sure it's good even this small change for 2.0.x ?

        Anyway, for including it in 2.0.4 I have no problem, but we have to:

        • merge to branches/2.0.x, and make some test
        • stop (cancel) the current vote
        • delete the 2.0.4 tag
        • regenerate the release candicate (with a new tag) and redeploy artifacts for the vote
        • start a new vote.

        Note only that before next week I won't have so much free time (approx. 3 / 4 hours for all steps), sorry ... or we could fix it for 2.0.5 . Tell me what you think. Bye

        Show
        smartini Sandro Martini added a comment - Hi Roger, after a quick look at the patch I think it's good. My only concern is on: > The logic is changed as follows: I'm sure now it's better, but are we sure it's good even this small change for 2.0.x ? Anyway, for including it in 2.0.4 I have no problem, but we have to: merge to branches/2.0.x, and make some test stop (cancel) the current vote delete the 2.0.4 tag regenerate the release candicate (with a new tag) and redeploy artifacts for the vote start a new vote. Note only that before next week I won't have so much free time (approx. 3 / 4 hours for all steps), sorry ... or we could fix it for 2.0.5 . Tell me what you think. Bye
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        I don't think it's worth it for 2.0.4 to do all that work over. If no one else has noticed the problem until now, it's not a show-stopper for this release. 2.0.5 is fine. Thanks.

        Show
        rwhitcomb Roger Whitcomb added a comment - I don't think it's worth it for 2.0.4 to do all that work over. If no one else has noticed the problem until now, it's not a show-stopper for this release. 2.0.5 is fine. Thanks.
        Hide
        smartini Sandro Martini added a comment -

        Ok, so I'll do the merge in next days and put in release notes for 2.0.5 ... Bye

        Show
        smartini Sandro Martini added a comment - Ok, so I'll do the merge in next days and put in release notes for 2.0.5 ... Bye
        Hide
        smartini Sandro Martini added a comment -

        Just do the merge in 2.0.x: Committed revision 1602867.

        Roger, how can I test the right behaviour in 2.0.x ? Or if you have some time, could you do some check and tell me ?
        So I could mark this as resolved even for 2.0.x.

        Show
        smartini Sandro Martini added a comment - Just do the merge in 2.0.x: Committed revision 1602867. Roger, how can I test the right behaviour in 2.0.x ? Or if you have some time, could you do some check and tell me ? So I could mark this as resolved even for 2.0.x.
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Thank you, Sandro, for the commit.

        I will provide a test case and check that in to both "trunk" and "branches/2.0.x".

        Show
        rwhitcomb Roger Whitcomb added a comment - Thank you, Sandro, for the commit. I will provide a test case and check that in to both "trunk" and "branches/2.0.x".
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        Checked in a test case program in branches/2.0.x:
        Adding tests\src\org\apache\pivot\tests\issues\pivot948
        Adding tests\src\org\apache\pivot\tests\issues\pivot948\Pivot948.java
        Adding tests\src\org\apache\pivot\tests\issues\pivot948\pivot_948.bxml
        Transmitting file data ..
        Committed revision 1756437.

        Merged to trunk:
        Sending .
        Adding tests\src\org\apache\pivot\tests\issues\pivot948

        Show
        rwhitcomb Roger Whitcomb added a comment - Checked in a test case program in branches/2.0.x: Adding tests\src\org\apache\pivot\tests\issues\pivot948 Adding tests\src\org\apache\pivot\tests\issues\pivot948\Pivot948.java Adding tests\src\org\apache\pivot\tests\issues\pivot948\pivot_948.bxml Transmitting file data .. Committed revision 1756437. Merged to trunk: Sending . Adding tests\src\org\apache\pivot\tests\issues\pivot948
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        After committing the test program, I consider this issue fixed.

        Show
        rwhitcomb Roger Whitcomb added a comment - After committing the test program, I consider this issue fixed.

          People

          • Assignee:
            rwhitcomb Roger Whitcomb
            Reporter:
            rwhitcomb Roger Whitcomb
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development