Details

      Description

      Literally moving the classes from com.sun.javafx.scene.control.skin to javafx.scene.control.skin is trivial. The time consuming aspects are:

      1) Reviewing each class and making only the minimal necessary API public. Guestimate: two weeks.
      2) Performing API review - requiring time from Kevin, but also someone else such as Leif. Guestimate: two weeks.
      3) There are some classes that are not quite skins, but not quite controls. We need to decide whether these remain private for now, or whether they graduate to fully-fledged controls. Guestimate: one week.
      4) Updating 'the world (i.e. Scene Builder, etc)' to use the new APIs: one week.

      Total guestimate (with no padding): six weeks.

        Issue Links

          Activity

          Show
          jgiles Jonathan Giles added a comment - Changeset URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/5023330f0526
          Hide
          tmikula Tomas Mikula added a comment -
          TextPosInfo needs to be changed to take character clusters into account, as noted in JDK-8092327, point 2.
          As it currently stands, TextPosInfo#getInsertionIndex() is broken: it may return an index inside a character cluster. Indeed, the skins themselves don't use TextPosInfo#getInsertionIndex(), but rather Utils#getHitInsertionIndex(TextPosInfo hit, String text), which is not available to the users, since Utils is in com.sun.javafx.scene.control.skin. Also note that Utils#getHitInsertionIndex in order to work around the limitation of TextPosInfo, needs to see the text again. I think the right thing to do is to make sure the returned TextPosInfo is correct, otherwise it is useless for anything but private implementation again.

          The most reasonable place to fix this seems to be to fix the hit test implementation on Text, rather than keep it broken and then work around it in text input controls.
          Show
          tmikula Tomas Mikula added a comment - TextPosInfo needs to be changed to take character clusters into account, as noted in JDK-8092327 , point 2. As it currently stands, TextPosInfo#getInsertionIndex() is broken: it may return an index inside a character cluster. Indeed, the skins themselves don't use TextPosInfo#getInsertionIndex(), but rather Utils#getHitInsertionIndex(TextPosInfo hit, String text), which is not available to the users, since Utils is in com.sun.javafx.scene.control.skin. Also note that Utils#getHitInsertionIndex in order to work around the limitation of TextPosInfo, needs to see the text again. I think the right thing to do is to make sure the returned TextPosInfo is correct, otherwise it is useless for anything but private implementation again. The most reasonable place to fix this seems to be to fix the hit test implementation on Text, rather than keep it broken and then work around it in text input controls.
          Hide
          tmikula Tomas Mikula added a comment -
          While at it, TextPosInfo is good for positioning caret, but not for getting the character under the cursor (JDK-8091012), which is useful e.g. for displaying a tooltip based on the word under the cursor. As I suggested in JDK-8136350, I think a reasonable API for TextPosInfo would be

              public OptionalInt getCharacterIndex();
              public int getInsertionIndex();

          Note the OptionalInt, to determine whether any character was hit at all.
          Show
          tmikula Tomas Mikula added a comment - While at it, TextPosInfo is good for positioning caret, but not for getting the character under the cursor ( JDK-8091012 ), which is useful e.g. for displaying a tooltip based on the word under the cursor. As I suggested in JDK-8136350 , I think a reasonable API for TextPosInfo would be     public OptionalInt getCharacterIndex();     public int getInsertionIndex(); Note the OptionalInt, to determine whether any character was hit at all.
          Hide
          kcr Kevin Rushforth added a comment -
          Resolving as fixed now that the changesets have been pushed (we can file follow-on issues for further work).
          Show
          kcr Kevin Rushforth added a comment - Resolving as fixed now that the changesets have been pushed (we can file follow-on issues for further work).

            People

            • Assignee:
              jgiles Jonathan Giles
              Reporter:
              jgiles Jonathan Giles
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Due:
                Created:
                Updated:
                Resolved: