[9] Code Review Request For 8130675:Document that setting scene on stage changes stage size unless explicitly set

[Chien Yang]

Hi Kevin,

Please review this proposed doc fix:

JIRA:https://bugs.openjdk.java.net/browse/JDK-8130675
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8130675/webrev.00/

Thanks,
- Chien

Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Kevin Rushforth]

The 2016-08-12 revision looks fine to me, except for a missing space as 
noted in JBS (no need for a new patch if that is the only issue found).


While we wait for approval from the JDK 9 release team, we need another 
reviewer for this.


Alexander Z: can you take a look?

-- Kevin


Alexander Nyssen wrote:

Hi Kevin,

attached please find a revised patch that contains the corrections you 
requested. The patch is now applicable to the new module structure 
(modues with 'javafx.' prefix).


Regards,
Alexander





Am 12.08.2016 um 00:00 schrieb Kevin Rushforth 
>:




Alexander Nyssen wrote:

Hi Kevin,

thanks for your feedback. Please fin my comments inline.

  

Am 09.08.2016 um 03:10 schrieb Kevin Rushforth :

I uploaded the patch, reviewed it, and provided comments in the bug report. The 
short version is:

* The new API looks good

* There is a missing '@since 9' in the javadoc comments along with a few typos 
/ style issues


I will take care of it. Is there a style guide?
  


Not a current one. The ones I pointed out in the JBS issue were 
either grammatical or capitalization (other than the '@since' which 
is required for all new API).



* Rather than using reflection and setAccessible in the implementation, please 
add a public getHostContainer method to EmbeddedWindow (since it is an internal 
method, there is no concern with doing that -- it isn't API).


Such a getHost() method was already introduced to EmbeddedWindow as part of the 
patch (to obtain the HostContainer from it). The reflection code within 
getFXCanvas() is used to access the enclosing instance (i.e. the FXCanvas) of 
the HostContainer. We could only circumvent this by introducing a constructor 
to HostContainer and by passing in the enclosing FXCanvas instance explicitly 
(so we can query it via an explicit getter, which would have to be introduced 
in addition). Would you prefer that?
  


Ah, I see what you are doing now. There is an easier way, then. Just 
add a default (package) scope 'fxCanvas' variable in the inner class 
initialized to 'FXCanvas.this' and you will be able to access it 
directly, since an instance of an inner class can always get access 
to the instance of the enclosing class.


private class HostContainer implements HostInterface {
final FXCanvas fxCanvas = FXCanvas.this;
...

I updated the bug report with this and the other style issues. I 
haven't tested it yet, but other than the listed issues it looks very 
close to being done.


-- Kevin


  

Additionally, I requested JDK 9 release team approval for this. The approval 
process can proceed in parallel with your addressing the issues I raised.

— Kevin


Regards,
Alexander

  

Alexander Nyssen wrote:


Hi Kevin,

attached please find a revised patch. My comments are inlined.

  

Am 28.07.2016 um 18:03 schrieb Kevin Rushforth >:

Hi

Alexander Nyssen wrote:


Hi,

I have added my comments below:


  

Am 28.07.2016 um 17:22 schrieb Kevin Rushforth >:

I got the attachment, since Alexander also CCed me directly. I will attach it 
shortly.


Thanks!
  

Done.



I do have two comments on this:

1) We are past Feature Freeze, so all Enhancements need formal JDK 9 R-team 
approval [1][2]. In this case, the justification can be internal API that is no 
longer accessible in JDK 9 due to Jigsaw (I would be very reluctant to consider 
any other Enhancement request this late in the process), but I will need to 
look at it and then take it through the approval process, provided that I feel 
it is in scope.


I was not aware about this, but I would of course appreciate if it could be 
included (due to Jigsaw). Thanks for considering it at least.
  

I'll take a closer look tomorrow or Monday (no more time today). At first 
glance it seems like something reasonable to take forward.


That sounds promising. Thanks!

  

2) Some of the changes you list seem unrelated to this enhancement and are 
better done as separate issues (e.g., the rework of the SWTCursorsTest). Also, 
I am unconvinced of the need to force GTK 2; in fact it seems at odds with the 
work we have done with JEP 283 [3].


Well, the test case refactoring is somehow related, as I introduced the common 
SWT rule while introducing the second SWT test. However, I could provide it as 
a separate contribution if that was wished (and a JIRA issue was provided), but 
the rest of this contribution of course requires it as a prerequisite. If this 
enhancement could not be included in JDK 9, I would have to provide it as a 
separate contribution, as I would have to re-introduce 

Re: [PATCH] 8161282: FXCanvas does not forward horizontal mouse scroll events to the embedded scene

[Kevin Rushforth]

OK, I'll upload this revised version of the patch today.

-- Kevin


Alexander Nyssen wrote:

Hi Kevin,

please consider the following updated patch instead, which contains an 
additional null-check.


Regards
Alexander





Am 12.08.2016 um 16:04 schrieb Alexander Nyssen >:


Hi Kevin,

attached please find an initial patch for 
https://bugs.openjdk.java.net/browse/JDK-8161282.


The patch is not as minimal as I had hoped, as the 
EmbeddedSceneInterface had to be changed to differentiate between 
mouse and scroll events (while up to now, scroll events are handled 
as mouse events), but for me this seemed necessary to fix this issue 
properly. As a result, JFXPanel had to be adjusted as well to comply 
to the changes in the EmbeddedSceneInterface, while its behavior 
should not have changed.


As horizontal mouse events cannot be synthesized via 
Display.post(Event) yet (an open issue for SWT), I did not add an 
automated test, but instead added a manual one 
(FXCanvasMouseWheelEventsTest). Therefore, this patch does not depend 
on the patch I provided earlier for JDK-8160325.


Best Regards,
Alexander






=

Re: [PATCH] 8161282: FXCanvas does not forward horizontal mouse scroll events to the embedded scene

[Alexander Nyssen]

Hi Kevin,

please consider the following updated patch instead, which contains an 
additional null-check.

Regards
Alexander


> Am 12.08.2016 um 16:04 schrieb Alexander Nyssen :
> 
> Hi Kevin,
> 
> attached please find an initial patch for 
> https://bugs.openjdk.java.net/browse/JDK-8161282 
> .
> 
> The patch is not as minimal as I had hoped, as the EmbeddedSceneInterface had 
> to be changed to differentiate between mouse and scroll events (while up to 
> now, scroll events are handled as mouse events), but for me this seemed 
> necessary to fix this issue properly. As a result, JFXPanel had to be 
> adjusted as well to comply to the changes in the EmbeddedSceneInterface, 
> while its behavior should not have changed.
> 
> As horizontal mouse events cannot be synthesized via Display.post(Event) yet 
> (an open issue for SWT), I did not add an automated test, but instead added a 
> manual one (FXCanvasMouseWheelEventsTest). Therefore, this patch does not 
> depend on the patch I provided earlier for JDK-8160325.
> 
> Best Regards,
> Alexander
> 
> 
> 
> 

Re: Structuring CSS Stylesheets

[Kevin Rushforth]

One slight correction on how to contribute below.


David Grieve wrote:



On 8/15/16 10:52 AM, Daniel Glöckner wrote:

We found the culprits by patching the JRE, adding some statistics to
SimpleSelector and CompoundSelector. I was wondering whether there are
easier ways but anyway, it works ;)
This sounds like some code that would be good to share with the 
community. :)
[DG] Sure thing. It's not too complicated and doesn't use external 
libs. Any hint where I could post it / paste it?
See https://wiki.openjdk.java.net/display/OpenJFX/Community for how to 
contribute


That page is just a placeholder, and finding what you need in the 
sub-pages a bit tricky. See the following page for how to contribute:


http://openjdk.java.net/contribute/

-- Kevin



need them, for example our UI component factory would add table.css to a
TableView's list of stylesheets (tv. 
getStylesheets().add("/path/to/table.css").

The global theme.css would be minimal and only define colors and fonts.
What do you think about this approach? Will this work nicely with 
caching of

CSS styles in JavaFX?
I think if you are going to go this route, you might want to use
Region#getUserAgentStylesheets() which adds the styles as user-agent 
styles.

But I don't think it will buy you much in terms of CSS performance.
[DG] We also want to control / override the CSS of standard JavaFX 
controls like TreeTableView. Ideally we don't need to sub class them 
so we would need to use parent. getStylesheets().add(), right?
I doubt that getUserAgentStylesheets() or getStylesheets() is going to 
have much impact. My guess is that having the stylesheets added to the 
scene is going to be your best bet. I say this because the code that 
does the style matching has to combine styles together from 
Region#getUserAgentStylesheets() and Parent#getStylesheets(), whereas 
the styles from scene stylesheets are already combined. You have to 
think of these different sources of styles as sets of styles. When you 
have Region or Parent stylesheets, you have to create a union of those 
styles with the default user-agent stylesheets (e.g., caspian.css). 
With just scene stylesheets, you have just one set (this isn't 100% 
accurate, but close enough for this discussion).


If you the biggest bang for your buck relative to JavaFX CSS 
performance, avoid

style lookup and relative font sizes.

[DG] Could you explain what you mean by "avoid style lookups"?
You know about styles like '-fx-base' used in caspian.css. You change 
the color for -fx-base and the basic colors of the UI change. This 
magic happens at runtime. So if I have a label in a cell in a table, 
and it has a style "-fx-border-color: -fx-base", JavaFX will - at 
runtime - try to resolve -fx-base into an actual color. It starts at 
the leaf and looks tries to resolve -fx-base. If it can't resolve it, 
it looks for a style in the parent node, and so on up the parent-chain 
all the way to the root node. The worst case scenario, then, is that 
there are no styles that resolve the value until you get to .root.


This is what I mean by 'style lookups'.

Its great stuff (the brainchild of Jasper Potts) because I can change 
the look of my UI just by setting '-fx-base'. But if I were developing 
a UI and I didn't care to let the users of my UI make such changes, 
I'd go through and remove all the lookups in caspian.css (not trivial 
because there are many many lookups - not just -fx-base). Or use a 
pre-processor such as SASS or LESS.


The same sort of lookup happens when you have an em (or other relative 
size) because you need a font or a font-size to complete the 
calculation. In most cases, the lookup for a font or font-size goes 
all the way to .root, where it fails and falls back on 
Font.getDefault(). But its a trade off since em sizes let your UI more 
easily scale to different displays.

Re: Structuring CSS Stylesheets

[David Grieve]

On 8/15/16 10:52 AM, Daniel Glöckner wrote:

We found the culprits by patching the JRE, adding some statistics to
SimpleSelector and CompoundSelector. I was wondering whether there are
easier ways but anyway, it works ;)

This sounds like some code that would be good to share with the community. :)

[DG] Sure thing. It's not too complicated and doesn't use external libs. Any 
hint where I could post it / paste it?
See https://wiki.openjdk.java.net/display/OpenJFX/Community for how to 
contribute

need them, for example our UI component factory would add table.css to a
TableView's list of stylesheets (tv. getStylesheets().add("/path/to/table.css").
The global theme.css would be minimal and only define colors and fonts.

What do you think about this approach? Will this work nicely with caching of

CSS styles in JavaFX?
I think if you are going to go this route, you might want to use
Region#getUserAgentStylesheets() which adds the styles as user-agent styles.
But I don't think it will buy you much in terms of CSS performance.

[DG] We also want to control / override the CSS of standard JavaFX controls 
like TreeTableView. Ideally we don't need to sub class them so we would need to 
use parent. getStylesheets().add(), right?
I doubt that getUserAgentStylesheets() or getStylesheets() is going to 
have much impact. My guess is that having the stylesheets added to the 
scene is going to be your best bet. I say this because the code that 
does the style matching has to combine styles together from 
Region#getUserAgentStylesheets() and Parent#getStylesheets(), whereas 
the styles from scene stylesheets are already combined. You have to 
think of these different sources of styles as sets of styles. When you 
have Region or Parent stylesheets, you have to create a union of those 
styles with the default user-agent stylesheets (e.g., caspian.css). With 
just scene stylesheets, you have just one set (this isn't 100% accurate, 
but close enough for this discussion).



If you the biggest bang for your buck relative to JavaFX CSS performance, avoid
style lookup and relative font sizes.

[DG] Could you explain what you mean by "avoid style lookups"?
You know about styles like '-fx-base' used in caspian.css. You change 
the color for -fx-base and the basic colors of the UI change. This magic 
happens at runtime. So if I have a label in a cell in a table, and it 
has a style "-fx-border-color: -fx-base", JavaFX will - at runtime - try 
to resolve -fx-base into an actual color. It starts at the leaf and 
looks tries to resolve -fx-base. If it can't resolve it, it looks for a 
style in the parent node, and so on up the parent-chain all the way to 
the root node. The worst case scenario, then, is that there are no 
styles that resolve the value until you get to .root.


This is what I mean by 'style lookups'.

Its great stuff (the brainchild of Jasper Potts) because I can change 
the look of my UI just by setting '-fx-base'. But if I were developing a 
UI and I didn't care to let the users of my UI make such changes, I'd go 
through and remove all the lookups in caspian.css (not trivial because 
there are many many lookups - not just -fx-base). Or use a pre-processor 
such as SASS or LESS.


The same sort of lookup happens when you have an em (or other relative 
size) because you need a font or a font-size to complete the 
calculation. In most cases, the lookup for a font or font-size goes all 
the way to .root, where it fails and falls back on Font.getDefault(). 
But its a trade off since em sizes let your UI more easily scale to 
different displays.

Focus Traversal of Nodes / Cells in TreeTableView

[Daniel Glöckner]

Hi,

We're trying to implement an editable data grid using a TreeTableView.

The tree table contains nodes:
List> columns = ...;
theTreeTable.getColumns().setAll(columns);

Some of the nodes are custom controls (basically advanced text fields) which 
are editable:

someColumn.setCellValueFactory(cellDate -> {
SomeModel lineModel = cellDate.getValue().getValue();
Node someEditor = 
GuiComponentUtils.createEditorComponent();
someEditor.setModel(lineModel);
return new SimpleObjectProperty<>( someEditor);
});

The task at hand is to make these controls focus traversable (column by column 
and row by row). 
We have to consider the case that the table is not fully visible, i.e. wrapped 
in a ScrollPane. This does not work out of the box since the components might 
not exist if they are not visible.

Any ideas how this could be implemented? Is this case considered at all in 
JavaFX's implementation of focus traversal? Maybe it will be in Java 9?

Kind regards,
Daniel

RE: Structuring CSS Stylesheets

[Daniel Glöckner]

Hi,

Thanks a lot for your comments, David.

> -Original Message-
> From: openjfx-dev [mailto:openjfx-dev-boun...@openjdk.java.net] On Behalf
> Of David Grieve
> Sent: Monday, August 15, 2016 4:35 PM
> To: openjfx-dev@openjdk.java.net
> Subject: Re: Structuring CSS Stylesheets
> 
> 
> 
> On 8/15/16 9:46 AM, Daniel Glöckner wrote:
> > Hi,
> >
> > We recently came across a number of performance issues which were caused
> by our poor CSS.
> >
> > Our stylesheet contained too many selectors, specifically too many generic
> selectors targeting "common" JavaFX controls (.text, .label etc.).
> Make the selectors as specific as possible. Avoid the universal selector '*' 
> if you
> can.
> >
> > We found the culprits by patching the JRE, adding some statistics to
> > SimpleSelector and CompoundSelector. I was wondering whether there are
> > easier ways but anyway, it works ;)
> This sounds like some code that would be good to share with the community. :)
[DG] Sure thing. It's not too complicated and doesn't use external libs. Any 
hint where I could post it / paste it?
> >
> > We've meanwhile improved our CSS performance (by making a bunch of
> selectors more specific) but want to re-structure the stylesheets to be more
> future-proof (with even better performance ;)).
> >
> > Could you quickly comment on the following idea?
> >
> > Our CSS is already divided into several stylesheets (e.g. table.css, some-
> dialog.css etc.).
> > These stylesheets are all imported via @import into a global theme.css.
> theme.css is then added to the scene.
> >
> > We're thinking about adding the individual stylesheets only to nodes which
> need them, for example our UI component factory would add table.css to a
> TableView's list of stylesheets (tv. 
> getStylesheets().add("/path/to/table.css").
> The global theme.css would be minimal and only define colors and fonts.
> >
> > What do you think about this approach? Will this work nicely with caching of
> CSS styles in JavaFX?
> I think if you are going to go this route, you might want to use
> Region#getUserAgentStylesheets() which adds the styles as user-agent styles.
> But I don't think it will buy you much in terms of CSS performance.
[DG] We also want to control / override the CSS of standard JavaFX controls 
like TreeTableView. Ideally we don't need to sub class them so we would need to 
use parent. getStylesheets().add(), right?
> 
> If you the biggest bang for your buck relative to JavaFX CSS performance, 
> avoid
> style lookup and relative font sizes.

[DG] Could you explain what you mean by "avoid style lookups"?

Re: Structuring CSS Stylesheets

[David Grieve]

On 8/15/16 9:46 AM, Daniel Glöckner wrote:

Hi,

We recently came across a number of performance issues which were caused by our 
poor CSS.

Our stylesheet contained too many selectors, specifically too many generic selectors 
targeting "common" JavaFX controls (.text, .label etc.).
Make the selectors as specific as possible. Avoid the universal selector 
'*' if you can.


We found the culprits by patching the JRE, adding some statistics to 
SimpleSelector and CompoundSelector. I was wondering whether there are easier 
ways but anyway, it works ;)
This sounds like some code that would be good to share with the 
community. :)


We've meanwhile improved our CSS performance (by making a bunch of selectors 
more specific) but want to re-structure the stylesheets to be more future-proof 
(with even better performance ;)).

Could you quickly comment on the following idea?

Our CSS is already divided into several stylesheets (e.g. table.css, 
some-dialog.css etc.).
These stylesheets are all imported via @import into a global theme.css. 
theme.css is then added to the scene.

We're thinking about adding the individual stylesheets only to nodes which need them, for 
example our UI component factory would add table.css to a TableView's list of stylesheets 
(tv. getStylesheets().add("/path/to/table.css"). The global theme.css would be 
minimal and only define colors and fonts.

What do you think about this approach? Will this work nicely with caching of 
CSS styles in JavaFX?
I think if you are going to go this route, you might want to use 
Region#getUserAgentStylesheets() which adds the styles as user-agent 
styles. But I don't think it will buy you much in terms of CSS performance.


If you the biggest bang for your buck relative to JavaFX CSS 
performance, avoid style lookup and relative font sizes.




Kind regards,
Daniel

Structuring CSS Stylesheets

[Daniel Glöckner]

Hi,

We recently came across a number of performance issues which were caused by our 
poor CSS.

Our stylesheet contained too many selectors, specifically too many generic 
selectors targeting "common" JavaFX controls (.text, .label etc.).

We found the culprits by patching the JRE, adding some statistics to 
SimpleSelector and CompoundSelector. I was wondering whether there are easier 
ways but anyway, it works ;)

We've meanwhile improved our CSS performance (by making a bunch of selectors 
more specific) but want to re-structure the stylesheets to be more future-proof 
(with even better performance ;)).

Could you quickly comment on the following idea?

Our CSS is already divided into several stylesheets (e.g. table.css, 
some-dialog.css etc.). 
These stylesheets are all imported via @import into a global theme.css. 
theme.css is then added to the scene.

We're thinking about adding the individual stylesheets only to nodes which need 
them, for example our UI component factory would add table.css to a TableView's 
list of stylesheets (tv. getStylesheets().add("/path/to/table.css"). The global 
theme.css would be minimal and only define colors and fonts.

What do you think about this approach? Will this work nicely with caching of 
CSS styles in JavaFX?

Kind regards,
Daniel