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

TerraTextInputSkin may process the same key press twice

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4
    • Fix Version/s: 2.1, 2.0.5
    • Component/s: wtk-terra
    • Labels:
      None

      Description

      TerraTextInputSkin's keyPress method may call super.keyPress twice (once as its first instruction, and then again in the "else" branch).
      I suggest to remove the first ancestor call.

        Activity

        Hide
        alessandro.pira Alessandro Pira added a comment -

        Sorry, the exact name of the method is "keyPressed"

        Show
        alessandro.pira Alessandro Pira added a comment - Sorry, the exact name of the method is "keyPressed"
        Hide
        smartini Sandro Martini added a comment -

        Hi Alessandro, thanks for the info ... only one thing: to ensure that this small change doesn't introduce side effects, to see/test it you do something particular ?

        Roger, what do you think ?

        Show
        smartini Sandro Martini added a comment - Hi Alessandro, thanks for the info ... only one thing: to ensure that this small change doesn't introduce side effects, to see/test it you do something particular ? Roger, what do you think ?
        Hide
        alessandro.pira Alessandro Pira added a comment -

        What I am doing is:

        • attach a FocusTraversalPolicy to a TextInput that (under some circumstances) pops up a message and leaves the focus on the same text input
        • run the application, give focus to the TextInput and press Tab
        • the popup message is shown twice
        Show
        alessandro.pira Alessandro Pira added a comment - What I am doing is: attach a FocusTraversalPolicy to a TextInput that (under some circumstances) pops up a message and leaves the focus on the same text input run the application, give focus to the TextInput and press Tab the popup message is shown twice
        Hide
        rwhitcomb Roger Whitcomb added a comment -

        In looking through the code, it seems like TerraTextInputSkin is the only class that does this (call "super.keyPressed" twice). The "consumed" value from the first call is never used, and in fact, is set by each of the "if" cases, and "else" sets it by calling "super.keyPressed" again. So, I agree with Alessandro's analysis, and believe that we should take out the first call.

        If there would be problems it would be with the keys that are processed in this skin method itself (which would be DELETE, BACKSPACE, HOME, END, arrow keys, etc.). The only problem I could envision would be with TAB (which should traverse fields in a container) but which is never processed by TextInput. So, I believe this fix would be completely safe.

        Show
        rwhitcomb Roger Whitcomb added a comment - In looking through the code, it seems like TerraTextInputSkin is the only class that does this (call "super.keyPressed" twice). The "consumed" value from the first call is never used, and in fact, is set by each of the "if" cases, and "else" sets it by calling "super.keyPressed" again. So, I agree with Alessandro's analysis, and believe that we should take out the first call. If there would be problems it would be with the keys that are processed in this skin method itself (which would be DELETE, BACKSPACE, HOME, END, arrow keys, etc.). The only problem I could envision would be with TAB (which should traverse fields in a container) but which is never processed by TextInput. So, I believe this fix would be completely safe.
        Hide
        smartini Sandro Martini added a comment - - edited

        For 2.0.x: Committed revision 1631366.
        For Trunk (2.1.0): Committed revision 1631371.

        I'll mark this as resolved in a few days ... anyway be free to reopen if needed.
        Alessandro and Roger, thank you very much for catching and helping me on this.

        Show
        smartini Sandro Martini added a comment - - edited For 2.0.x: Committed revision 1631366. For Trunk (2.1.0): Committed revision 1631371. I'll mark this as resolved in a few days ... anyway be free to reopen if needed. Alessandro and Roger, thank you very much for catching and helping me on this.
        Hide
        smartini Sandro Martini added a comment -

        Resolved.

        Show
        smartini Sandro Martini added a comment - Resolved.

          People

          • Assignee:
            smartini Sandro Martini
            Reporter:
            alessandro.pira Alessandro Pira
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development