Commit on focus loss in ListView, TreeView, TableView, and TreeTableView

[Jonathan Giles]

Hi all,

In the next week I have two topics to discuss, both with similar names 
but related to quite different functionality. The topic for today is 
adding the necessary API to the JavaFX UI control Cell class to enable 
support for 'commit on focus loss', that is, the ability to 
automatically commit a user-input value (into, say, a TextField inside a 
Cell) back into the backing data structure when the cell (or its 
content) looses focus. The topic for _next_ week relates to a new focus 
traversal API I plan to propose, related to focus traversal over all 
nodes in the scenegraph.


At present, the cells used in ListView, TreeView, TableView, and 
TreeTableView all support the concept of switching between a normal 
state and an editing state. Due to the way cells were built, when the 
editing state was enabled, the developer was simply entitled to change 
the controls displayed to the user. The cell is never made aware of what 
the editing controls were, and as such has no way to know what the user 
has entered. Today, when focus is lost, this results in a cancel edit 
event being fired, the user input is dismissed, and the cell returns to 
its non-editing state displaying the previous data. This is less than 
ideal, and is what we are trying to fix here.


Unfortunately, there is no way to enable this 'for free'. If we used 
heuristics and looked at the nodes in the cell when in the editing 
state, it would be error-prone. For one, what if the cell has, say, 
multiple TextFields? Even in the simple cases, what if the data model 
was not String-based, and required some conversion from String to T? 
What if the user had a custom cell for editing that could not be 
understood by the Cell? We'd have no choice but to offer some form of 
fallback API to support this.


Therefore, my proposal is to flip this around - we introduce a single 
new method on the Cell class, as shown below, and then retrofit our 
custom cell factories (most notably the TextField*Cell classes) to 
implement this method, providing the commit-on-focus-loss behavior that 
is expected.


/**
 * Developers of custom cell implementations should override this 
method when
 * the cell provides editing functionality, with this method 
returning the
 * user input after the user has interacted with the editing 
components.
 * The form of the returned data (wrapped in an {@link Optional}) 
should

 * be the same as the form of the underlying data model, hence the
 * use of the {@code T} generic type. If no value is available, or 
if the
 * value to be returned is invalid, {@code Optional.empty()} should 
be returned

 * to indicate that the commit should not proceed..
 *
 * @return The value provided by the user into this cell when it 
was in its
 *  editing state, in the form of the underlying data model. If 
the value
 *  is invalid or unable to be determined, {@code 
Optional.empty()} should

 *  be returned.
 * @since 10
 */
protected Optional getEditorValue() {
return Optional.empty();
}

As noted, this method is already overridden by the pre-built 
TextField*Cell classes (so by default, users of TextFieldListCell, 
TextFieldTreeCell, TextFieldTableCell, and TextFieldTreeTableCell get 
commit on focus loss by default). Developers of custom cells who want 
commit on focus loss to be enabled will have to override this method. I 
will work to ensure that this new method is well-understood and widely 
known, and that all relevant documentation I can change will be changed 
to mention this.


As part of this work, I have also developed a test app - 
HelloCommitOnFocusLoss - that tests this new functionality on ListView, 
TreeView, TableView, and TreeTableView. In all tests that I have 
implemented so far, I can see that commit on focus loss works in cases 
where the user clicks on a new row, and when they give focus to another 
node (by both mouse and tab key). I am happy to extend this test 
application with any test cases people fear may not be supported, and 
then we can find gaps in this implementation (or, hopefully, be 
pleasantly surprised that these cases are already being met).


Discussion can take place in the Jira issue at [1], and the webrev with 
my proposed changes is at [2], although note that I have yet to develop 
enough unit tests for this change - as I wanted to discuss the API first.


Your feedback now is much appreciated.

[1] https://bugs.openjdk.java.net/browse/JDK-8089514
[2] http://cr.openjdk.java.net/~jgiles/8089514.1/

Thanks,
-- Jonathan

Re: javafx.scene.shape.Path (memory) inefficient PathElements

[Jim Graham]

Hi Tom,

Those look like good suggestions.  I would file bugs in JBS and create them 
separately:

- Bug for lazy property creation in path elements
- Feature request for lower-memory paths

Did you benchmark how much the lazy properties, on their own, would save your 
application?

...jim

On 5/4/17 2:22 PM, Tom Schindl wrote:

Hi,

We are currently working on a PDF-Rendering library in JavaFX and we
need to draw glyphs using the JavaFX Path API (there are multiple
reasons why we don't use the Text-Node and or Canvas).

When drawing a page full of Text this means that we have a Path-Object
with gazillions of MoveTo and CubicCurveTo elements who sum up to 30MB
just to represent them in the SG because PathElements store their
information in properties and forcefully intialize them in their
constructor.

The only public API to work around this problem is to construct a
StringBuffer and use SVGPath which means:
* it takes time to construct the SVG-Path-String
* it takes time to parse the SVG-Path-String in JavaFX

As an experiment (and because we are still on Java8 we can easily do
that) was that we created our own Shape-Subclass who:
* uses floats (there's no reason to use double in the SG when the
  backing API - Path2D - is float based)
* passes the floats directly to the Path2D/NGPath API

Guess what: We now need 2.5MB / page which means 27.5MB is the overhead
added by the current Path-API - ouch!

I think a fairly low hanging memory optimization for the PathElement
would be to create properties lazy (only if someone access the property).

For MoveTo this would mean the following minimal change (eg for the
x-value):

private DoubleProperty x;
private double _x;

public final void setX(double value) {
  if (x != null) {
 xProperty().set(value);
  } else {
 _x = value;
 u();
  }
}

public final double getX() {
  return x == null ? _x : x.get();
}

public final DoubleProperty xProperty() {
  if (x == null) {
x = new DoublePropertyBase(_x) {

  @Override
  public void invalidated() {
u();
  }

  @Override
  public Object getBean() {
return MoveTo.this;
  }

  @Override
  public String getName() {
return "x";
  }
   };
  }
  return x;
}

I guess 99% of the code out there never access the Property so the small
footprint added by the primitive field is justifiable.

This still has the overhead of all the needless PathElement objects
hanging around so another idea is to have a 3rd SG-Path-Type who
strictly uses the float-Primitives with a similar API to Path2D (in fact
it only acts as a public API to Path2D).

Thoughts?

Tom

javafx.scene.shape.Path (memory) inefficient PathElements

[Tom Schindl]

Hi,

We are currently working on a PDF-Rendering library in JavaFX and we
need to draw glyphs using the JavaFX Path API (there are multiple
reasons why we don't use the Text-Node and or Canvas).

When drawing a page full of Text this means that we have a Path-Object
with gazillions of MoveTo and CubicCurveTo elements who sum up to 30MB
just to represent them in the SG because PathElements store their
information in properties and forcefully intialize them in their
constructor.

The only public API to work around this problem is to construct a
StringBuffer and use SVGPath which means:
* it takes time to construct the SVG-Path-String
* it takes time to parse the SVG-Path-String in JavaFX

As an experiment (and because we are still on Java8 we can easily do
that) was that we created our own Shape-Subclass who:
* uses floats (there's no reason to use double in the SG when the
  backing API - Path2D - is float based)
* passes the floats directly to the Path2D/NGPath API

Guess what: We now need 2.5MB / page which means 27.5MB is the overhead
added by the current Path-API - ouch!

I think a fairly low hanging memory optimization for the PathElement
would be to create properties lazy (only if someone access the property).

For MoveTo this would mean the following minimal change (eg for the
x-value):

private DoubleProperty x;
private double _x;

public final void setX(double value) {
  if (x != null) {
 xProperty().set(value);
  } else {
 _x = value;
 u();
  }
}

public final double getX() {
  return x == null ? _x : x.get();
}

public final DoubleProperty xProperty() {
  if (x == null) {
x = new DoublePropertyBase(_x) {

  @Override
  public void invalidated() {
u();
  }

  @Override
  public Object getBean() {
return MoveTo.this;
  }

  @Override
  public String getName() {
return "x";
  }
   };
  }
  return x;
}

I guess 99% of the code out there never access the Property so the small
footprint added by the primitive field is justifiable.

This still has the overhead of all the needless PathElement objects
hanging around so another idea is to have a 3rd SG-Path-Type who
strictly uses the float-Primitives with a similar API to Path2D (in fact
it only acts as a public API to Path2D).

Thoughts?

Tom

-- 
Thomas Schindl, CTO
BestSolution.at EDV Systemhaus GmbH
Eduard-Bodem-Gasse 5-7, A-6020 Innsbruck
http://www.bestsolution.at/
Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck

API docs for javafx.* modules are now included with the JDK 9 API docs

[Kevin Rushforth]

Some of you may have noticed already that the API docs for javafx.* 
modules are now included as part of the JDK 9 API docs on java.net:


http://download.java.net/java/jdk9/docs/api/

The search box also works as expected to find JavaFX classes as well an 
any classes in the rest of the JDK. Hopefully this new unified bundle 
will make it easier to navigate.


-- Kevin

Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Kevin Rushforth]

This seems like something that could be useful, although at this point 
in the release we would more likely do it for JDK 10. I do note that 
including the class that made the illegal access is generally a good 
idea when that class is attempting the access on its own behalf. For 
frameworks such as JavaFX beans or FXML, we are making the request on 
behalf of an application, it wasn't as helpful to have the class itself 
be in the error message (which was Mandy's main point). So there are at 
least two cases to consider if we end up creating such a utility method.


Thanks.

-- Kevin


Gregg Wonderly wrote:

If there is not already such an exception, it would seem like a good idea to 
have an exception that formats such a message from constructor parameters 
providing the details so that it’s the same everywhere, and so that it can be 
changed in once place if needed.

Gregg

  

On May 3, 2017, at 9:48 AM, Kevin Rushforth  wrote:

OK, I'll create a more informative message. I think it will be more clear in the message 
to just say that it needs to "open" the package to javafx.base, since that 
would be the recommendation for a package that isn't already exported unconditionally. 
I'll send out a new webrev this morning with all feedback incorporated.

-- Kevin


Mandy Chung wrote:


On May 2, 2017, at 2:22 PM, Kevin Rushforth  wrote:

Here is the message:

IllegalAccessException: class com.sun.javafx.property.MethodHelper cannot access class com.foo (in module foo.app) because module foo.app does not open com.foo to javafx.base 
   


It would be better to emit a more informative message e.g. “javafx.base cannot 
access class com.foo (in module foo.app).  Either exports com.foo unqualifiedly 
or open com.foo to javafx.base”.  Also in MethodUtil::invoke
 61if (!module.isExported(packageName)) {
You can do this check only if module.isNamed.

 
  

It is roughly the same message that any similar illegal access would generate (e.g., we 
get similar error messages when FXML tries to call setAccessible for a field annotated 
with @FXML if the module is not "open" to javafx.fxml).

   


javafx.base/src/main/java/com/sun/javafx/property/MethodHelper.java
javafx.fxml/src/main/java/com/sun/javafx/fxml/MethodHelper.java
javafx.web/src/main/java/com/sun/webkit/MethodHelper.java
 45 public static Object invoke(Method m, Object obj, Object[] params)

To avoid 3 ModuleHelper classes, the invoke method can take
the callerModule argument to replace this line:   56 final Module 
thisModule = MethodHelper.class.getModule();
   
  

I'm fairly certain that won't work. Module::addOpens is caller sensitive and 
will only work when called from the module in question.

   


You are right.  Wont’t work.
 
  

javafx.base/src/main/java/com/sun/javafx/reflect/MethodUtil.java
 There are a few other public methods which I think JavaFX doesn’t
 need and can be removed.
   
  

Yes, I could do this to reduce the public footprint of the class. To minimize the diffs 
between the original and our copy, I might just comment out the "public". That 
would also make it easier to add them back in a future version (e.g., to eventually get 
rid of all dependency on sun.reflect.misc). Thoughts?
   


I will leave it up to you.
Mandy

 
  


  

Re: [9] Review request: 8177566: FX user module gets IllegalAccessException from sun.reflect.misc.Trampoline

[Gregg Wonderly]

If there is not already such an exception, it would seem like a good idea to 
have an exception that formats such a message from constructor parameters 
providing the details so that it’s the same everywhere, and so that it can be 
changed in once place if needed.

Gregg

> On May 3, 2017, at 9:48 AM, Kevin Rushforth  
> wrote:
> 
> OK, I'll create a more informative message. I think it will be more clear in 
> the message to just say that it needs to "open" the package to javafx.base, 
> since that would be the recommendation for a package that isn't already 
> exported unconditionally. I'll send out a new webrev this morning with all 
> feedback incorporated.
> 
> -- Kevin
> 
> 
> Mandy Chung wrote:
>>> On May 2, 2017, at 2:22 PM, Kevin Rushforth  
>>> wrote:
>>> 
>>> Here is the message:
>>> 
>>> IllegalAccessException: class com.sun.javafx.property.MethodHelper cannot 
>>> access class com.foo (in module foo.app) because module foo.app does not 
>>> open com.foo to javafx.base 
>>>
>> 
>> It would be better to emit a more informative message e.g. “javafx.base 
>> cannot access class com.foo (in module foo.app).  Either exports com.foo 
>> unqualifiedly or open com.foo to javafx.base”.  Also in MethodUtil::invoke
>>  61if (!module.isExported(packageName)) {
>> You can do this check only if module.isNamed.
>> 
>>  
>>> It is roughly the same message that any similar illegal access would 
>>> generate (e.g., we get similar error messages when FXML tries to call 
>>> setAccessible for a field annotated with @FXML if the module is not "open" 
>>> to javafx.fxml).
>>> 
>>>
 javafx.base/src/main/java/com/sun/javafx/property/MethodHelper.java
 javafx.fxml/src/main/java/com/sun/javafx/fxml/MethodHelper.java
 javafx.web/src/main/java/com/sun/webkit/MethodHelper.java
  45 public static Object invoke(Method m, Object obj, Object[] params)
 
 To avoid 3 ModuleHelper classes, the invoke method can take
 the callerModule argument to replace this line:   56 final Module 
 thisModule = MethodHelper.class.getModule();

>>> I'm fairly certain that won't work. Module::addOpens is caller sensitive 
>>> and will only work when called from the module in question.
>>> 
>>>
>> 
>> You are right.  Wont’t work.
>>  
 javafx.base/src/main/java/com/sun/javafx/reflect/MethodUtil.java
  There are a few other public methods which I think JavaFX doesn’t
  need and can be removed.

>>> Yes, I could do this to reduce the public footprint of the class. To 
>>> minimize the diffs between the original and our copy, I might just comment 
>>> out the "public". That would also make it easier to add them back in a 
>>> future version (e.g., to eventually get rid of all dependency on 
>>> sun.reflect.misc). Thoughts?
>>>
>> 
>> I will leave it up to you.
>> Mandy
>> 
>>