Re: [openjdk/jfx] 8234712: Add pivot properties for scale and rotation in Node, ScaleTransition and RotateTransition (#53)

[Nir Lisker]

>
> As long as you do the following, it should work as expected:
>
> T(pivotRot) * T(rotate) * T(-pivotRot) * T(pivotScale) * T(scale) *
> T(-pivotScale) * object coords
>

 I think that this is what the PR does already. Line 5061 onward is:

localToParentTx = localToParentTx.deriveWithTranslation(rotPivotX,
rotPivotY, rotPivotZ)
 .deriveWithRotation(rotAngle, rotAxisX,
rotAxisY, rotAxisZ)
 .deriveWithTranslation(-rotPivotX,
-rotPivotY, -rotPivotZ);
...
localToParentTx = localToParentTx.deriveWithTranslation(scalePivotX,
scalePivotY, scalePivotZ)
 .deriveWithScale(getScaleX(), getScaleY(),
getScaleZ())
 .deriveWithTranslation(-scalePivotX,
-scalePivotY, -scalePivotZ);

On Tue, Dec 17, 2019 at 2:07 AM Kevin Rushforth 
wrote:

>
>
> On 12/15/2019 6:06 PM, Nir Lisker wrote:
>
> Replying on the mailing list to the questions raised on GitHub.
>
> The state of whether to use the computed center pivot or the value of the
>> pivot attribute is implicit with no way for an application to know which it
>> is, and no way to set it back to using the computed center (i.e., the state
>> is sticky once you set it). Perhaps if you defined a null value as meaning
>> "computed center" then an app could at least reset it to the "computed
>> center" state, although there would still be no way for the app to know
>> that it was in that state.
>>
>
> In the JBS issue  I
> alluded to this in point 5. I think that null should represent the default
> (node center). However, if we use 3 doubles instead of a Point3D we might
> need to use Double.NaN for this instead, which would also be the default
> for this case. The docs will explain this.
>
>
> Using Double.NaN as an out of band value would be odd, and probably not
> what we want. In addition to the fact that it is somewhat artificial, it
> would mean that X, Y, and Z would independently be treated as coming from
> the set value or from the computed center. I guess this is one argument for
> using a Point3D object, but as you note, there are other drawbacks.
>
> Do we need separate properties for scale pivot and center pivot?
>>
>
> I say yes, otherwise the enhancement will be very limited. I think of this
> enhancement as adding pivot control to Rotate/Scale transitions, and adding
> them to Node is a necessary (and desirable) step.
>
>
> Yeah, I figured this was the case.
>
>... you need to worry about what coordinate space the rotation pivot is
>> defined in. Perhaps if the rotation pivot were defined in unscaled space,
>> it might work.
>>
>
> Isn't it already? If I set the rotation pivot to the edge of the node,
> then scale it down, then rotate, the rotation pivot would be outside of the
> node's boundaries. In scaled space it would still be on the edge. Or did I
> misunderstand you?
>
>
> I think I was wrong in the comment I added to the PR.
>
> What I meant is that you would need to define the coordinate space that
> the pivot values are in, and it needs to be defined in a way that it is
> consistent with current behavior. Today, the part of the matrix
> transformation that does the scale and rotate, including the computed
> pivot, is this:
>
> T(pivot) * T(rotate) * T(scale) * T(-pivot) * object coords
>
> As long as you do the following, it should work as expected:
>
>
> T(pivotRot) * T(rotate) * T(-pivotRot) * T(pivotScale) * T(scale) *
> T(-pivotScale) * object coords
>
> Importantly, this will work exactly as it does today when pivotRot ==
> pivotScale which is the case I was most concerned about.
>
> In any case, I don't think that there's a single correct answer here.
>
>
> It needs to be consistent with current behavior, and match what an
> application would expect. I think it should not be a problem if defined as
> above.
>
> Should the pivot be specified as a Point3D or 3 separate doubles?
>> Separate doubles... there would be no out-of-band null value to use
>
>
> See my point above about Double.NaN.
>
>
> OK, there is no "natural" out of band value that is likely to be
> satisfying. We'd probably end up wanting a boolean that controls computed
> versus explicit (either a single flag or one for each of scalePivot and
> rotatePivot)
>
> The doubles vs Point3D is an important choice. We might want to look into
> the future even where Point3D (and 2D) could be made into an Inline class
> with Valhalla, which will help with the performance aspect. Binding to one
> of the coordinates is sill a problem there, however.
>
>
> I think this (doubles vs Point3D) is really the main question. I don't
> know what the best answer is, but I'd like to hear from other developers.
>
> -- Kevin
>
>
> On Sat, Dec 14, 2019 at 6:25 PM Kevin Rushforth 
> wrote:
>
>> This will need discussion on the openjfx-dev mailing list. Here are the
>> questions that need to be resolved:
>>
>>1.
>>
>>  

Re: Skara - bot sending can-be-integrated message prematurely?

[Nir Lisker]

Do other projects also have multi-reviewers requirements?

I would think that at least for a CSR, which is OpenJDK-wide, a request
could be put in with the Skara to track it. A Reviewer (or Committer?)
could invoke a /csr command which will require the PR author to provide a
link to the CSR, and check that it was approved as part of the patch
approval process.

Not sure how complicated it would be to implement.

- Nir

On Mon, Dec 16, 2019 at 5:39 PM Kevin Rushforth 
wrote:

> That's a good question about whether we can ask for a slight rewording
> of the Skara bot message to indicate that there might be other things to
> check before "/integrate". I'll raise that question with the Skara team.
>
> As an aside, I noticed the other day that you typed "/integrate" after a
> single review, but in that case, there was no clear indication from Ajit
> (the first reviewer and the sponsor) that a second review was required,
> so I didn't comment on it.
>
> -- Kevin
>
>
> On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:
> >
> > Kevin,
> >
> > thanks for the clarification :) My bad that I didn't re-read the
> > contrib.md. But then, who does? The lazy like myself do it
> > occasionally only (down to once and then forget about it )
> >
> > Maybe the bot message can be improved? With some indication that its
> > (the bot's) knowledge about review requirements is limited, so would
> > require a careful check by the contributor before actually post the
> > /integrate comment? Actually, I think I goofed the other day, was
> > safed only by Ajit who waited for the 2nd review before his /sponsor.
> >
> > -- Jeanette
> >
> > Zitat von Kevin Rushforth :
> >
> >> I added a comment to the two PRs in question, but it bears discussion
> >> here.
> >>
> >> The Skara bot can't know whether all criteria have been met. It
> >> can't, for example, know whether there are outstanding comments from
> >> some reviewers that need to be addressed. Nor does it know which PRs
> >> need two reviewers (or sometimes a third if there is a specific
> >> person we would like to review it), which ones need a CSR, etc.
> >>
> >> So having it state authoritatively that the PR is ready to integrate
> >> is a bit misleading. This is documented in the Code Review section of
> >> the CONTRIBUTING [1] doc:
> >>
> >>> NOTE: while the Skara tooling will indicate that the PR is ready to
> >>> integrate once the first reviewer with a "Reviewer" role in the
> >>> project has approved it, this may or may not be sufficient depending
> >>> on the type of fix. For example, you must wait for a second approval
> >>> for enhancements or high-impact bug fixes.
> >>
> >> If anyone can think of a way to improve this and make it more clear,
> >> that would be helpful.
> >>
> >> -- Kevin
> >>
> >> [1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md
> >>
> >>
> >> On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:
> >>>
> >>> Looks like it assumes a pull request as properly reviewed as soon as
> >>> it gets a single approve - independent on how many reviewers are
> >>> required, see f.i.
> >>>
> >>> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
> >>> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
> >>>
> >>> -- Jeanette
> >>>
> >
> >
> >
>
>

Re: [openjdk/jfx] 8234712: Add pivot properties for scale and rotation in Node, ScaleTransition and RotateTransition (#53)

[Kevin Rushforth]

On 12/15/2019 6:06 PM, Nir Lisker wrote:

Replying on the mailing list to the questions raised on GitHub.

The state of whether to use the computed center pivot or the value
of the pivot attribute is implicit with no way for an application
to know which it is, and no way to set it back to using the
computed center (i.e., the state is sticky once you set it).
Perhaps if you defined a null value as meaning "computed center"
then an app could at least reset it to the "computed center"
state, although there would still be no way for the app to know
that it was in that state.


In the JBS issue  I 
alluded to this in point 5. I think that null should represent the 
default (node center). However, if we use 3 doubles instead of a 
Point3D we might need to use Double.NaN for this instead, which would 
also be the default for this case. The docs will explain this.


Using Double.NaN as an out of band value would be odd, and probably not 
what we want. In addition to the fact that it is somewhat artificial, it 
would mean that X, Y, and Z would independently be treated as coming 
from the set value or from the computed center. I guess this is one 
argument for using a Point3D object, but as you note, there are other 
drawbacks.



Do we need separate properties for scale pivot and center pivot?


I say yes, otherwise the enhancement will be very limited. I think of 
this enhancement as adding pivot control to Rotate/Scale transitions, 
and adding them to Node is a necessary (and desirable) step.


Yeah, I figured this was the case.


 ... you need to worry about what coordinate space the rotation
pivot is defined in. Perhaps if the rotation pivot were defined in
unscaled space, it might work.


Isn't it already? If I set the rotation pivot to the edge of the node, 
then scale it down, then rotate, the rotation pivot would be outside 
of the node's boundaries. In scaled space it would still be on the 
edge. Or did I misunderstand you?


I think I was wrong in the comment I added to the PR.

What I meant is that you would need to define the coordinate space that 
the pivot values are in, and it needs to be defined in a way that it is 
consistent with current behavior. Today, the part of the matrix 
transformation that does the scale and rotate, including the computed 
pivot, is this:


    T(pivot) * T(rotate) * T(scale) * T(-pivot) * object coords

As long as you do the following, it should work as expected:


    T(pivotRot) * T(rotate) * T(-pivotRot) * T(pivotScale) * T(scale) * 
T(-pivotScale) * object coords


Importantly, this will work exactly as it does today when pivotRot == 
pivotScale which is the case I was most concerned about.



In any case, I don't think that there's a single correct answer here.


It needs to be consistent with current behavior, and match what an 
application would expect. I think it should not be a problem if defined 
as above.



Should the pivot be specified as a |Point3D| or 3 separate
doubles? Separate doubles... there would be no out-of-band
|null| value to use

See my point above about Double.NaN.


OK, there is no "natural" out of band value that is likely to be 
satisfying. We'd probably end up wanting a boolean that controls 
computed versus explicit (either a single flag or one for each of 
scalePivot and rotatePivot)


The doubles vs Point3D is an important choice. We might want to look 
into the future even where Point3D (and 2D) could be made into an 
Inline class with Valhalla, which will help with the performance 
aspect. Binding to one of the coordinates is sill a problem there, 
however.


I think this (doubles vs Point3D) is really the main question. I don't 
know what the best answer is, but I'd like to hear from other developers.


-- Kevin


On Sat, Dec 14, 2019 at 6:25 PM Kevin Rushforth 
mailto:notificati...@github.com>> wrote:


This will need discussion on the openjfx-dev mailing list. Here
are the questions that need to be resolved:

1.

The state of whether to use the computed center pivot or the
value of the pivot attribute is implicit with no way for an
application to know which it is, and no way to set it back to
using the computed center (i.e., the state is sticky once you
set it). Perhaps if you defined a null value as meaning
"computed center" then an app could at least reset it to the
"computed center" state, although there would still be no way
for the app to know that it was in that state.

2.

Do we need separate properties for scale pivot and center
pivot? A single pivot property would be easier to define, but
wouldn't allow you to set it from a |RotationTransition| and a
|ScaleTransition| if you wanted to apply both to the same
|Node|. With two separate properties, as you have defined it,
it is more 

Re: [openjdk/jfx] 8234712: Add pivot properties for scale and rotation in Node, ScaleTransition and RotateTransition (#53)

[Nir Lisker]

Thanks for the input Michael,


> Introducing two separate centers for rotation and scaling is, to my
> opinion, just academic without any practical use-case and in the end much
> more complicated for the user.


I have a practical use case in my app. I assume that the people who
requested these enhancements also had one. I also don't think that it's
"much" more complicated as it just aligns with the existing transforms
functionality.

1. Three doubles always use their full space even if you don't need them.


No sure what you mean by "full space". Properties are initialized lazily,
so if a value was not set, the reference is null, and the default pivot
value applies (computed center).

2. Using Double.NaNs as a switch between computed and user-provided
> center is just confusing and error-prone.


It is confusing indeed, but I don't think it's error prone as you'd be hard
pressed to fall on a NaN value for a pivot point as a result of a
computation. We could use one of the INFINITY values instead if it's
better. We should create a constant, public static final double CENTER =
Double.NaN (placeholder name), just like Animation.INDEFINITE = -1,
Region.USE_PREF_SIZE
= Double.NEGATIVE_INFINITY, Region.USE_COMPUTED_SIZE = -1 etc. It's not
ideal, but it's a solution for allowing the user to revert to the default
behavior without additional boolean properties.


> What would be the meaning of setting x to some value but leaving y and z
> as NaN?
>

That y and z are the computed center (Node computes the x, y and z of the
center separately to being with), and x is the user value.

I bet most users will forget to set the Z-value anyway if they are thinking
> 2D.


They wouldn't need to, it will behave as it does now (overall, backwards
compatibility is guaranteed).


> A simple null value is much easier to handle and explain.


Yes, it's certainly an advantage of a Point3D, which was my initial
implementation. The main issue with it, I would say, is that it's difficult
to bind to.

- Nir

On Mon, Dec 16, 2019 at 1:36 PM Michael Paus  wrote:

> I don't think there are clearly right or wrong answers to all these
> questions,
> so I would just like to provide a view thoughts on this.
>
> The governing principle should be a principle of "least surprise" from a
> users
> point of view. That means we should not make things more complicated as
> they already are and should always keep the practical use-cases in mind.
> If you need full flexibility, you can always use the explicit transforms.
>
> We currently have the notion of the "Node's center" which is used for
> scaling
> and rotation and I think we should stick to that. The documentation later
> explains that "The pivot point about which the rotation occurs is the
> center
> of the untransformed layoutBounds." which is a completely arbitrary
> decision
> because, e.g., from a mathematically point of view the geometric center
> of the
> node would seem to be more natural but is more difficult to compute for
> general
> shapes. So, we should just extend this scheme in such a way that the
> user can
> replace the single computed center with a single user-defined center
> specified in untransformed coordinates. Introducing two separate centers
> for rotation and scaling is, to my opinion, just academic without any
> practical
> use-case and in the end much more complicated for the user.
>
> I would also specify this center point as a Point3D for two reasons:
> 1. Three doubles always use their full space even if you don't need them.
> 2. Using Double.NaNs as a switch between computed and user-provided center
>  is just confusing and error-prone.
>  What would be the meaning of setting x to some value but leaving y
> and z as
>  NaN? Would you implicitly set them to 0.0 or use the computed values
> or
>  would you discard this user setting completely and use all the
> computed
>  values if any of the components is NaN? I bet most users will
> forget to set the
>  Z-value anyway if they are thinking 2D. A simple null value is much
> easier
>  to handle and explain.
>
> Just my two €ent.
>
> --Michael
>
> Am 16.12.19 um 03:06 schrieb Nir Lisker:
> > Replying on the mailing list to the questions raised on GitHub.
> >
> > The state of whether to use the computed center pivot or the value of the
> >> pivot attribute is implicit with no way for an application to know
> which it
> >> is, and no way to set it back to using the computed center (i.e., the
> state
> >> is sticky once you set it). Perhaps if you defined a null value as
> meaning
> >> "computed center" then an app could at least reset it to the "computed
> >> center" state, although there would still be no way for the app to know
> >> that it was in that state.
> >>
> > In the JBS issue  I
> > alluded to this in point 5. I think that null should represent the
> default
> > (node center). However, if we use 3 doubles instead of a Point3D we 

Re: [Rev 02] RFR: 8207759: VK_ENTER not consumed by TextField when default Button exists

[Kevin Rushforth]

On Mon, 16 Dec 2019 23:41:37 GMT, Jeanette Winzenburg  
wrote:

>> This is a fix for https://bugs.openjdk.java.net/browse/JDK-8207759
>> 
>> The issue is that default/cancel button on a scene are triggered even though 
>> a onKeyPressed handler for ENTER/CANCEL consumes the keyEvent. See the bug 
>> for details on both cause and fix.
>> 
>> There are additional/changed tests to verify the fix. All old tests are 
>> passing.
> 
> The pull request has been updated with 1 additional commit.



-

Marked as reviewed by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/15

Re: openjfx 13 MediaPlayer vodeo stutter

[Farrukh Najmi]

Sorry I just realized that this is the dev team mailing list and my post
may not be appropriate here. I have posted a question on this at
stackoverflow along with a minimal standalone test program:

https://stackoverflow.com/questions/59364435/javafx-video-stutter-after-playing-same-video-over-and-over-for-a-couple-of-hour

On Sun, Dec 15, 2019 at 6:13 PM Farrukh Najmi 
wrote:

> On Ubuntu linux 18.04 I have been successfully using openjfx 13 during
> development of a new app. My app plays the same video over and over
> using javafx.scene.media.MediaView and javafx.scene.media.MediaPlayer.
> At first the video plays flawlessly. After a few hours the video becomes
> choppy and starts displaying stutter. I have analyzed my app for memory
> leaks and thread leaks and do not believe it has any.
> I observe that "top -H" command shows the following with QuantumRenderer
> at top:
>
> Threads: 575 total,   6 running, 515 sleeping,   0 stopped,   1 zombie
> %Cpu(s): 64.3 us,  8.1 sy,  0.0 ni, 25.0 id,  0.0 wa,  0.0 hi,  2.6 si,
>  0.0 st
> KiB Mem : 26.7/7988780  [|||
>   ]
> KiB Swap:  0.0/2097148  [
>]
>
>   PID USER  PR  NIVIRTRESSHR S %CPU %MEM TIME+ COMMAND
>
>  1432 root  20   0  0.101t 702424  90656 R 93.5  8.8   1471:14
> QuantumRenderer
>  9901 root  20   0  0.101t 702424  90656 R 60.1  8.8 424:41.22
> queue7:src
>  2592 root  20   0  0.101t 702424  90656 S 59.8  8.8 505:55.76
> queue5:src
>  1133 ubuntu20   0  912192 172908  87736 R 58.2  2.2   1133:23 Xorg
>
>  1335 root  20   0  334104  34200   9968 S  5.6  0.4  77:41.33 val
>
>  9900 root  20   0  0.101t 702424  90656 S  4.2  8.8  28:22.37
> qtdemux3:sink
>  2594 root  20   0  0.101t 702424  90656 S  2.9  8.8  30:41.56
> qtdemux2:sink
>  2590 root  20   0  0.101t 702424  90656 S  1.6  8.8  16:05.02
> queue4:src
>  9903 root  20   0  0.101t 702424  90656 S  1.6  8.8  11:08.72
> queue6:src
>  1976 ubuntu-6   0 1172376  12012   8796 S  1.6  0.2  26:16.32
> alsa-sink-ALC25
>  9546 ubuntu20   0   51728   4560   3544 R  1.6  0.1   0:00.26 top
>
>  1620 root  20   0  0.101t 702424  90656 S  0.7  8.8  14:45.24 JavaFX
> Applicat
>  9887 root  20   0  0.101t 702424  90656 S  0.7  8.8   4:27.78
> Timer-47
>  9902 root  20   0  0.101t 702424  90656 S  0.7  8.8   2:46.59
> threaded-ml
>  9905 root  20   0  0.101t 702424  90656 S  0.7  8.8   4:37.95
> Timer-49
>  1454 ubuntu20   0 3389116 131052  68972 S  0.7  1.6   3:11.03
> gnome-shell
>  1487 ubuntu 9 -11 1172376  12012   8796 S  0.7  0.2  10:28.21
> pulseaudio
>   912 root  20   0  0.101t 702424  90656 S  0.3  8.8   1:09.97 GC
> Thread#0
>   916 root  20   0  0.101t 702424  90656 S  0.3  8.8   0:30.33 G1
> Young RemSet
>   920 root  20   0  0.101t 702424  90656 S  0.3  8.8   0:43.78 VM
> Thread
>  1619 root  20   0  0.101t 702424  90656 S  0.3  8.8   4:41.19
> InvokeLaterDisp
>  2228 root  20   0  0.101t 702424  90656 S  0.3  8.8   1:06.68 GC
> Thread#3
>  9883 root  20   0  0.101t 702424  90656 S  0.3  8.8   1:25.20
> JFXMedia Player
>
> I am looking for suggestions on how to fix the video stutter issue. Is
> this a known issue. If so, please share link to issue.
> If it is a known issue, has it been fixed in openjfx 14-ea+4? I tried
> using that EA release but with that as dependency my video seems to stop
> and first frame and does not play at all.
> I am wondering if there are any migration steps that I may be missing.
>
> TIA for any suggestion on how to fix my video stutter issue with openjfx
> 13 or openjfx 14-ea+4.
>
> --
> Regards,
> Farrukh
>


-- 
Regards,
Farrukh

Re: [Rev 07] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Kevin Rushforth]

On Mon, 16 Dec 2019 21:33:11 GMT, Hadzic Samir  wrote:

>> Allright, this is a fix for JDK-8207957
> 
> The pull request has been updated with 1 additional commit.

Looks good.

Once the CSR is approved, this can be integrated.

-

Marked as reviewed by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/6

Re: Proposed schedule for JavaFX 14

[Kevin Rushforth]

The announced RDP1 date is the first Monday of the new year. In taking a 
closer look at the upcoming holiday schedule and accounting for 
additional days off some will be taking before or after, this doesn't 
allow enough time for the final stages of the review process to 
complete, including CSR approval. It is quite possible that an 
enhancement with a mostly-completed review and a CSR that is Finalized 
this Thursday or Friday (Dec 19 or 20), won't be approved in time for 
integration.


To address this, we are delaying RDP1 by three days. The new RDP1 date is:

Thursday, Jan 9, 23:59 PST

The dates for RDP2, code freeze, and GA are unchanged.

Please be aware that this three-day extension isn't an invitation to try 
to get additional features into 14 at the last minute, but rather an 
adjustment to better align with the holiday schedule in order to give 
the already-in-flight features that are nearing completion a little more 
time to make it. It is still the case that if an enhancement isn't ready 
(meaning API discussed & approved, code review mostly done, and CSR 
finalized) by this Friday, it should be retargeted to JavaFX 15.


-- Kevin


On 12/10/2019 1:45 PM, Kevin Rushforth wrote:
As a reminder, RDP1 for JavaFX 14 starts on January 6, 2020 (at 23:59 
Pacific time).


With the holidays approaching, please allow sufficient time for any 
feature that needs a CSR. New features should be far enough along in 
the review process so you can finalize the CSR before next Friday, Dec 
20, or it is likely to miss the window for this release, in which case 
it can be targeted for JavaFX 15.


During rampdown of JavaFX 14, the "master" branch of the jfx repo will 
be open for JavaFX 15 fixes.


We will follow the same process as previous releases for getting fixes 
into JavaFX 14 during rampdown. I'll send a message with detailed 
information later, but generally candidates for fixing after RDP1 are 
P1-P3 bugs (as long as they are not risky) and test or doc bugs of any 
priority.


-- Kevin


On 9/20/2019 8:00 AM, Kevin Rushforth wrote:

Here is the proposed schedule for JavaFX 14.

RDP1: Jan 6, 2020 (aka “feature freeze”)
RDP2: Feb 3, 2020
Freeze: Feb 24, 2020
GA: March 10, 2020

We plan to fork a jfx14 stabilization branch at RDP1. The GA date is 
expected to be one week ahead of JDK 14, matching what we did for 13.


Please let Johan or me know if you have any questions.

-- Kevin

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

That's a good question about whether we can ask for a slight rewording 
of the Skara bot message to indicate that there might be other things to 
check before "/integrate". I'll raise that question with the Skara team.


As an aside, I noticed the other day that you typed "/integrate" after a 
single review, but in that case, there was no clear indication from Ajit 
(the first reviewer and the sponsor) that a second review was required, 
so I didn't comment on it.


-- Kevin


On 12/16/2019 6:41 AM, Jeanette Winzenburg wrote:


Kevin,

thanks for the clarification :) My bad that I didn't re-read the 
contrib.md. But then, who does? The lazy like myself do it 
occasionally only (down to once and then forget about it )


Maybe the bot message can be improved? With some indication that its 
(the bot's) knowledge about review requirements is limited, so would 
require a careful check by the contributor before actually post the 
/integrate comment? Actually, I think I goofed the other day, was 
safed only by Ajit who waited for the 2nd review before his /sponsor.


-- Jeanette

Zitat von Kevin Rushforth :

I added a comment to the two PRs in question, but it bears discussion 
here.


The Skara bot can't know whether all criteria have been met. It 
can't, for example, know whether there are outstanding comments from 
some reviewers that need to be addressed. Nor does it know which PRs 
need two reviewers (or sometimes a third if there is a specific 
person we would like to review it), which ones need a CSR, etc.


So having it state authoritatively that the PR is ready to integrate 
is a bit misleading. This is documented in the Code Review section of 
the CONTRIBUTING [1] doc:


NOTE: while the Skara tooling will indicate that the PR is ready to 
integrate once the first reviewer with a "Reviewer" role in the 
project has approved it, this may or may not be sufficient depending 
on the type of fix. For example, you must wait for a second approval 
for enhancements or high-impact bug fixes.


If anyone can think of a way to improve this and make it more clear, 
that would be helpful.


-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:


Looks like it assumes a pull request as properly reviewed as soon as 
it gets a single approve - independent on how many reviewers are 
required, see f.i.


https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Jeanette Winzenburg]

Kevin,

thanks for the clarification :) My bad that I didn't re-read the  
contrib.md. But then, who does? The lazy like myself do it  
occasionally only (down to once and then forget about it )


Maybe the bot message can be improved? With some indication that its  
(the bot's) knowledge about review requirements is limited, so would  
require a careful check by the contributor before actually post the  
/integrate comment? Actually, I think I goofed the other day, was  
safed only by Ajit who waited for the 2nd review before his /sponsor.


-- Jeanette

Zitat von Kevin Rushforth :


I added a comment to the two PRs in question, but it bears discussion here.

The Skara bot can't know whether all criteria have been met. It  
can't, for example, know whether there are outstanding comments from  
some reviewers that need to be addressed. Nor does it know which PRs  
need two reviewers (or sometimes a third if there is a specific  
person we would like to review it), which ones need a CSR, etc.


So having it state authoritatively that the PR is ready to integrate  
is a bit misleading. This is documented in the Code Review section  
of the CONTRIBUTING [1] doc:


NOTE: while the Skara tooling will indicate that the PR is ready to  
integrate once the first reviewer with a "Reviewer" role in the  
project has approved it, this may or may not be sufficient  
depending on the type of fix. For example, you must wait for a  
second approval for enhancements or high-impact bug fixes.


If anyone can think of a way to improve this and make it more clear,  
that would be helpful.


-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:


Looks like it assumes a pull request as properly reviewed as soon  
as it gets a single approve - independent on how many reviewers are  
required, see f.i.


https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Kevin Rushforth]

I added a comment to the two PRs in question, but it bears discussion here.

The Skara bot can't know whether all criteria have been met. It can't, 
for example, know whether there are outstanding comments from some 
reviewers that need to be addressed. Nor does it know which PRs need two 
reviewers (or sometimes a third if there is a specific person we would 
like to review it), which ones need a CSR, etc.


So having it state authoritatively that the PR is ready to integrate is 
a bit misleading. This is documented in the Code Review section of the 
CONTRIBUTING [1] doc:


NOTE: while the Skara tooling will indicate that the PR is ready to 
integrate once the first reviewer with a "Reviewer" role in the 
project has approved it, this may or may not be sufficient depending 
on the type of fix. For example, you must wait for a second approval 
for enhancements or high-impact bug fixes.


If anyone can think of a way to improve this and make it more clear, 
that would be helpful.


-- Kevin

[1] https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md


On 12/16/2019 4:23 AM, Jeanette Winzenburg wrote:


Looks like it assumes a pull request as properly reviewed as soon as 
it gets a single approve - independent on how many reviewers are 
required, see f.i.


https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: [Rev 07] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Kevin Rushforth]

On Mon, 16 Dec 2019 12:12:31 GMT, Jeanette Winzenburg  
wrote:

>>
> 
> hmm ... think that the bot isn't yet clever enough: this pull request needs 
> approval of two reviewer with _review_ role (mine is only informal)

This one still needs an approved CSR (the CSR is Finalized, but needs to be 
marked as Approved), and I still need to finish my review.

The Skara bot can't know whether all criteria have been met. It can't, for 
example, know whether there are outstanding comments from some reviewers that 
need to be addressed. Nor does it know which PRs need two reviewers (or 
sometimes a third if there is a specific person we would like to review it), 
which ones need a CSR, etc.

So having it state authoritatively that the PR is ready to integrate is a bit 
misleading in this case. This is documented in the Code Review section of the 
[CONTRIBUTING](https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md) doc:

>NOTE: while the Skara tooling will indicate that the PR is ready to integrate 
>once the first reviewer with a "Reviewer" role in the project has approved it, 
>this may or may not be sufficient depending on the type of fix. For example, 
>you must wait for a second approval for enhancements or high-impact bug fixes.

I wonder if there is a way to improve this?

-

PR: https://git.openjdk.java.net/jfx/pull/6

Re: [Rev 02] RFR: 8207759: VK_ENTER not consumed by TextField when default Button exists

[Kevin Rushforth]

On Mon, 16 Dec 2019 11:21:45 GMT, Jeanette Winzenburg  
wrote:

>>
> 
> Zitat von "openjdk[bot]" :
> 
>> @kleopatra This change can now be integrated. The commit message will be:
>> ```
> 
> Ajit, Kevin,
> 
> hmm .. wondering if this message is a bot-error? Kevin stated that it  
> needs two reviewers (approving, I assume?) but currently have one only  
> - so question is: should I post the /integrate comment or not yet?
> 
> -- Jeanette
> 
> 
> 
>> 8207759: VK_ENTER not consumed by TextField when default Button exists
>>
>> Reviewed-by: aghaisas
>> ```
>> - If you would like to add a summary, use the `/summary` command.
>> - To list additional contributors, use the `/contributor` command.
>>
>> Since the source branch of this PR was last updated there have been  
>> 53 commits pushed to the `master` branch:
>>  * fc539b57fbbebaab3c99c509d6e5f0e3b88858fa: 8223296:  
>> NullPointerException in GlassScene.java at line 325
>>  * 71ca899fbfc74d825c29b20a778d6d9eb243f90f: 8220722:  
>> ProgressBarSkin: adds strong listener to control's width property
>>  * 07af89a9e1a5e044d416e73dd14a84056fadf53c: 8221334: TableViewSkin:  
>> must initialize flow's cellCount in constructor
>>  * 4ddf55428d06dddadfc8cd14baabdc4d17318ee2: 8234916: [macos 10.15]  
>> Garbled text running with native-image
>>  * 46338d02754a5b5d475b065d492315fa307a0e3d: 8232524:  
>> SynchronizedObservableMap cannot be be protected for copying/iterating
>>  * a68347cbbf2191c981472c82c2b64d77bfbc613d: 8235150: IosApplication  
>> does not pass the required object in _leaveNestedEventLoopImpl
>>  * 1c27fbd218f1be2beac5cacbe93f202c11de439f: 8210955:  
>> DOMTest::testEventListenerCascade fails
>>  * 2d4096a8ffefb787bd71eb535231766a045e515f: 8235151: Nonexistent  
>> notifyQuit method referred from iOS GlassHelper.m
>>  * 98035cb2afef0c230d55095f0addeab73693d0ac: 8211308: Support HTTP/2  
>> in WebView
>>  * 6892fa1b3bbcfdb0e643a26441622916be987a38: 8232064: Switch FX  
>> build to use JDK 13.0.1 as boot JDK
>>  * 1d670f18d16d060dc40e83733cb3595c41df2a97: 8200224: Multiple press  
>> event when JFXPanel gains focus
>>  * 83eb0a7c9150566aae96db8ce9d1f78b714c9d1b: 8193445: JavaFX CSS is  
>> applied redundantly leading to significant performance degradation
>>  * 798afbcaeb73b90ba4bd511380786f7956e1b443: 8230610: Upgrade  
>> GStreamer to version 1.16.1
>>  * 126896db9928ae334332d38486fb61ad3a2e00a0: 8234704: Fix  
>> attribution in libxslt.md
>>  * 4d3c723e2f18efaca32a9df234063c7895b5540c: 8234593: Mark  
>> LeakTest.testGarbageCollectability as unstable
>>  * 5a398244af8f029d99d9cffc291667fc9c1ad9df: 8234056: Upgrade to  
>> libxslt 1.1.34
>>  * 8bea7b71454ada53881847c751add63e8fc99ea3: 8229472: Deprecate for  
>> removal JavaBeanXxxPropertyBuilders constructors
>>  * aad1720608856e910edaf05d0d53e87942642326: 8233420: Upgrade to gcc  
>> 8.3 on Linux
>>  * 42040c4cdb58eb0c6029a727df61b00248ee8e2b: 8232063: Upgrade gradle  
>> to version 6.0
>>  * aab07a4d01604355516de641b6141c91a30e0dc7: 8234239: [TEST_BUG]  
>> Reenable few ignored web tests
>>  * 95ad6017dc85b9504a4c20dd20f31094d9075f42: 8233421: Upgrade to  
>> Visual Studio 2017 version 15.9.16
>>  * 3e0557a815b6336d4e2eeaf5e833fe65f4298833: 8234303: [TEST_BUG]  
>> Correct ignore tag in graphics unit tests
>>  * e37cb37005b71edbf462eaa360c529a1e68c8656: 8234150: Address  
>> ignored tests in ComboBoxTest, LabeledTest, HyperLinkTest and  
>> TextInputControlTest
>>  * 4f496d417915b450880cbe664cba9130ab76ca08: 8234194: [TEST_BUG]  
>> Reenable few graphics unit tests
>>  * 927fc8a0f88adcaeb069bf0a7218a9c3f875d707: 8234174: Change IDEA  
>> VCS mapping to Git
>>  * 3d0cb4966a240ce81d18acffd64939757b79bed5: 8234189: [TEST_BUG]  
>> Remove ignored and invalid graphics unit tests
>>  * dc0130944365b61ac4a4333df29d684e9566d3cf: 8234110:  
>> SwingFXUtilsTest is unsuitable for unit test framework
>>  * 5b96ee42ac786ec23b99564715679d352c1637be: 8231188: Update SQLite  
>> to version 3.30.1
>>  * d46dcae7a571724f10d58e424842d703ea1cb0e3: 828: FX javadoc  
>> headings are out of sequence
>>  * 94bcf3fcfce83bae8d2014f15e993ba2edf24e0f: 8231692: Test  
>> Infrastructure: enhance KeyEventFirer to inject keyEvents into scene
>>  * 286d1b5427a2b9724cb400335741ed99295c5141: 8230492: font-family  
>> not set in HTMLEditor if font name has a number in it
>>  * f74f3afba53404e5169d97ef0d78c3ef2eeb42ef: 8233040:  
>> ComboBoxPopupControl: remove eventFilter for F4
>>  * a1cc4ab03698e9e9add95ecad191c9d89c6f35dd: 8232210: Update Mesa  
>> 3-D Headers to version 19.2.1
>>  * dca8df4e5d9071622dbaf83be48ae71b0927e6ae: 8232943: Gesture  
>> support is not initialized on iOS
>>  * 5a70b0c58cd723b535a6385fc8b431175ef44565: 8189092:  
>> ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
>>  * ac71396c9b1eb390cf09fde928fe5fbd9c31e089: 8232929: Duplicate  
>> symbols when building static libraries
>>  * ab6ea3b936f319cdea997aabed63ffdc639a75e1: 8232158: [macOS]  
>> Fallback to command line tools if xcode is missing

Re: Skara - bot sending can-be-integrated message prematurely?

[Jeanette Winzenburg]

Zitat von Nir Lisker :


The bot doesn't know how many reviewers are needed. This was brought up
before in point 3 in
http://mail.openjdk.java.net/pipermail/openjfx-dev/2019-October/023662.html.



missed that - thanks for the pointer :)


- Nir

On Mon, Dec 16, 2019 at 2:24 PM Jeanette Winzenburg 
wrote:



Looks like it assumes a pull request as properly reviewed as soon as
it gets a single approve - independent on how many reviewers are
required, see f.i.

https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
https://github.com/openjdk/jfx/pull/6#issuecomment-566028296

-- Jeanette

Re: Skara - bot sending can-be-integrated message prematurely?

[Nir Lisker]

The bot doesn't know how many reviewers are needed. This was brought up
before in point 3 in
http://mail.openjdk.java.net/pipermail/openjfx-dev/2019-October/023662.html.

- Nir

On Mon, Dec 16, 2019 at 2:24 PM Jeanette Winzenburg 
wrote:

>
> Looks like it assumes a pull request as properly reviewed as soon as
> it gets a single approve - independent on how many reviewers are
> required, see f.i.
>
> https://github.com/openjdk/jfx/pull/15#issuecomment-565964995
> https://github.com/openjdk/jfx/pull/6#issuecomment-566028296
>
> -- Jeanette
>
>

Re: [Rev 07] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Jeanette Winzenburg]

On Mon, 16 Dec 2019 11:46:57 GMT, Ajit Ghaisas  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> 

hmm ... think that the bot isn't yet clever enough: this pull request needs 
approval of two reviewer with _review_ role (mine is only informal)

-

PR: https://git.openjdk.java.net/jfx/pull/6

Re: [Rev 07] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Ajit Ghaisas]

On Mon, 16 Dec 2019 11:47:15 GMT, Hadzic Samir  wrote:

>> Allright, this is a fix for JDK-8207957
> 
> The pull request has been updated with 1 additional commit.



-

Marked as reviewed by aghaisas (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/6

Re: [openjdk/jfx] 8234712: Add pivot properties for scale and rotation in Node, ScaleTransition and RotateTransition (#53)

[Michael Paus]

I don't think there are clearly right or wrong answers to all these 
questions,

so I would just like to provide a view thoughts on this.

The governing principle should be a principle of "least surprise" from a 
users

point of view. That means we should not make things more complicated as
they already are and should always keep the practical use-cases in mind.
If you need full flexibility, you can always use the explicit transforms.

We currently have the notion of the "Node's center" which is used for 
scaling

and rotation and I think we should stick to that. The documentation later
explains that "The pivot point about which the rotation occurs is the center
of the untransformed layoutBounds." which is a completely arbitrary decision
because, e.g., from a mathematically point of view the geometric center 
of the
node would seem to be more natural but is more difficult to compute for 
general
shapes. So, we should just extend this scheme in such a way that the 
user can

replace the single computed center with a single user-defined center
specified in untransformed coordinates. Introducing two separate centers
for rotation and scaling is, to my opinion, just academic without any 
practical

use-case and in the end much more complicated for the user.

I would also specify this center point as a Point3D for two reasons:
1. Three doubles always use their full space even if you don't need them.
2. Using Double.NaNs as a switch between computed and user-provided center
    is just confusing and error-prone.
    What would be the meaning of setting x to some value but leaving y 
and z as

    NaN? Would you implicitly set them to 0.0 or use the computed values or
    would you discard this user setting completely and use all the 
computed
    values if any of the components is NaN? I bet most users will 
forget to set the
    Z-value anyway if they are thinking 2D. A simple null value is much 
easier

    to handle and explain.

Just my two €ent.

--Michael

Am 16.12.19 um 03:06 schrieb Nir Lisker:

Replying on the mailing list to the questions raised on GitHub.

The state of whether to use the computed center pivot or the value of the

pivot attribute is implicit with no way for an application to know which it
is, and no way to set it back to using the computed center (i.e., the state
is sticky once you set it). Perhaps if you defined a null value as meaning
"computed center" then an app could at least reset it to the "computed
center" state, although there would still be no way for the app to know
that it was in that state.


In the JBS issue  I
alluded to this in point 5. I think that null should represent the default
(node center). However, if we use 3 doubles instead of a Point3D we might
need to use Double.NaN for this instead, which would also be the default
for this case. The docs will explain this.

Do we need separate properties for scale pivot and center pivot?
I say yes, otherwise the enhancement will be very limited. I think of this
enhancement as adding pivot control to Rotate/Scale transitions, and adding
them to Node is a necessary (and desirable) step.

... you need to worry about what coordinate space the rotation pivot is

defined in. Perhaps if the rotation pivot were defined in unscaled space,
it might work.


Isn't it already? If I set the rotation pivot to the edge of the node, then
scale it down, then rotate, the rotation pivot would be outside of the
node's boundaries. In scaled space it would still be on the edge. Or did I
misunderstand you? In any case, I don't think that there's a single correct
answer here.

Should the pivot be specified as a Point3D or 3 separate doubles? Separate

doubles... there would be no out-of-band null value to use


See my point above about Double.NaN.
The doubles vs Point3D is an important choice. We might want to look into
the future even where Point3D (and 2D) could be made into an Inline class
with Valhalla, which will help with the performance aspect. Binding to one
of the coordinates is sill a problem there, however.

On Sat, Dec 14, 2019 at 6:25 PM Kevin Rushforth 
wrote:


This will need discussion on the openjfx-dev mailing list. Here are the
questions that need to be resolved:

1.

The state of whether to use the computed center pivot or the value of
the pivot attribute is implicit with no way for an application to know
which it is, and no way to set it back to using the computed center (i.e.,
the state is sticky once you set it). Perhaps if you defined a null value
as meaning "computed center" then an app could at least reset it to the
"computed center" state, although there would still be no way for the app
to know that it was in that state.
2.

Do we need separate properties for scale pivot and center pivot? A
single pivot property would be easier to define, but wouldn't allow you to
set it from a RotationTransition and a ScaleTransition if 

Re: [Rev 02] RFR: 8207759: VK_ENTER not consumed by TextField when default Button exists

[Jeanette Winzenburg]

On Mon, 16 Dec 2019 08:52:56 GMT, Ajit Ghaisas  wrote:

>> The pull request has been updated with 1 additional commit.
> 
> 

Zitat von "openjdk[bot]" :

> @kleopatra This change can now be integrated. The commit message will be:
> ```

Ajit, Kevin,

hmm .. wondering if this message is a bot-error? Kevin stated that it  
needs two reviewers (approving, I assume?) but currently have one only  
- so question is: should I post the /integrate comment or not yet?

-- Jeanette



> 8207759: VK_ENTER not consumed by TextField when default Button exists
>
> Reviewed-by: aghaisas
> ```
> - If you would like to add a summary, use the `/summary` command.
> - To list additional contributors, use the `/contributor` command.
>
> Since the source branch of this PR was last updated there have been  
> 53 commits pushed to the `master` branch:
>  * fc539b57fbbebaab3c99c509d6e5f0e3b88858fa: 8223296:  
> NullPointerException in GlassScene.java at line 325
>  * 71ca899fbfc74d825c29b20a778d6d9eb243f90f: 8220722:  
> ProgressBarSkin: adds strong listener to control's width property
>  * 07af89a9e1a5e044d416e73dd14a84056fadf53c: 8221334: TableViewSkin:  
> must initialize flow's cellCount in constructor
>  * 4ddf55428d06dddadfc8cd14baabdc4d17318ee2: 8234916: [macos 10.15]  
> Garbled text running with native-image
>  * 46338d02754a5b5d475b065d492315fa307a0e3d: 8232524:  
> SynchronizedObservableMap cannot be be protected for copying/iterating
>  * a68347cbbf2191c981472c82c2b64d77bfbc613d: 8235150: IosApplication  
> does not pass the required object in _leaveNestedEventLoopImpl
>  * 1c27fbd218f1be2beac5cacbe93f202c11de439f: 8210955:  
> DOMTest::testEventListenerCascade fails
>  * 2d4096a8ffefb787bd71eb535231766a045e515f: 8235151: Nonexistent  
> notifyQuit method referred from iOS GlassHelper.m
>  * 98035cb2afef0c230d55095f0addeab73693d0ac: 8211308: Support HTTP/2  
> in WebView
>  * 6892fa1b3bbcfdb0e643a26441622916be987a38: 8232064: Switch FX  
> build to use JDK 13.0.1 as boot JDK
>  * 1d670f18d16d060dc40e83733cb3595c41df2a97: 8200224: Multiple press  
> event when JFXPanel gains focus
>  * 83eb0a7c9150566aae96db8ce9d1f78b714c9d1b: 8193445: JavaFX CSS is  
> applied redundantly leading to significant performance degradation
>  * 798afbcaeb73b90ba4bd511380786f7956e1b443: 8230610: Upgrade  
> GStreamer to version 1.16.1
>  * 126896db9928ae334332d38486fb61ad3a2e00a0: 8234704: Fix  
> attribution in libxslt.md
>  * 4d3c723e2f18efaca32a9df234063c7895b5540c: 8234593: Mark  
> LeakTest.testGarbageCollectability as unstable
>  * 5a398244af8f029d99d9cffc291667fc9c1ad9df: 8234056: Upgrade to  
> libxslt 1.1.34
>  * 8bea7b71454ada53881847c751add63e8fc99ea3: 8229472: Deprecate for  
> removal JavaBeanXxxPropertyBuilders constructors
>  * aad1720608856e910edaf05d0d53e87942642326: 8233420: Upgrade to gcc  
> 8.3 on Linux
>  * 42040c4cdb58eb0c6029a727df61b00248ee8e2b: 8232063: Upgrade gradle  
> to version 6.0
>  * aab07a4d01604355516de641b6141c91a30e0dc7: 8234239: [TEST_BUG]  
> Reenable few ignored web tests
>  * 95ad6017dc85b9504a4c20dd20f31094d9075f42: 8233421: Upgrade to  
> Visual Studio 2017 version 15.9.16
>  * 3e0557a815b6336d4e2eeaf5e833fe65f4298833: 8234303: [TEST_BUG]  
> Correct ignore tag in graphics unit tests
>  * e37cb37005b71edbf462eaa360c529a1e68c8656: 8234150: Address  
> ignored tests in ComboBoxTest, LabeledTest, HyperLinkTest and  
> TextInputControlTest
>  * 4f496d417915b450880cbe664cba9130ab76ca08: 8234194: [TEST_BUG]  
> Reenable few graphics unit tests
>  * 927fc8a0f88adcaeb069bf0a7218a9c3f875d707: 8234174: Change IDEA  
> VCS mapping to Git
>  * 3d0cb4966a240ce81d18acffd64939757b79bed5: 8234189: [TEST_BUG]  
> Remove ignored and invalid graphics unit tests
>  * dc0130944365b61ac4a4333df29d684e9566d3cf: 8234110:  
> SwingFXUtilsTest is unsuitable for unit test framework
>  * 5b96ee42ac786ec23b99564715679d352c1637be: 8231188: Update SQLite  
> to version 3.30.1
>  * d46dcae7a571724f10d58e424842d703ea1cb0e3: 828: FX javadoc  
> headings are out of sequence
>  * 94bcf3fcfce83bae8d2014f15e993ba2edf24e0f: 8231692: Test  
> Infrastructure: enhance KeyEventFirer to inject keyEvents into scene
>  * 286d1b5427a2b9724cb400335741ed99295c5141: 8230492: font-family  
> not set in HTMLEditor if font name has a number in it
>  * f74f3afba53404e5169d97ef0d78c3ef2eeb42ef: 8233040:  
> ComboBoxPopupControl: remove eventFilter for F4
>  * a1cc4ab03698e9e9add95ecad191c9d89c6f35dd: 8232210: Update Mesa  
> 3-D Headers to version 19.2.1
>  * dca8df4e5d9071622dbaf83be48ae71b0927e6ae: 8232943: Gesture  
> support is not initialized on iOS
>  * 5a70b0c58cd723b535a6385fc8b431175ef44565: 8189092:  
> ArrayIndexOutOfBoundsException on Linux in getCachedGlyph
>  * ac71396c9b1eb390cf09fde928fe5fbd9c31e089: 8232929: Duplicate  
> symbols when building static libraries
>  * ab6ea3b936f319cdea997aabed63ffdc639a75e1: 8232158: [macOS]  
> Fallback to command line tools if xcode is missing
>  * 2ae171a2a0e6d1c24e6943f4f9a48987f6ed8ef4: 8232687: 

Re: [Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

On Wed, 11 Dec 2019 00:32:32 GMT, Kevin Rushforth  wrote:

>> Thanks for the review. I do not have access to a computer right now, I'll
>> update next week on Monday.
>> 
>> Le lun. 9 déc. 2019 à 17:01, Kevin Rushforth  a
>> écrit :
>> 
>>> I would like both @aghaisas  and myself to
>>> review / approve this. I will sponsor it.
>>>
>>> In addition, the CSR needs to be approved before this can be integrated.
>>>
>>> —
>>> You are receiving this because you authored the thread.
>>> Reply to this email directly, view it on GitHub
>>> ,
>>> or unsubscribe
>>> 
>>> .
>>>
> 
> I took the liberty of updating the CSR to add the missing comma, so I could 
> mark it as Reviewed. You can Finalize the CSR when you are ready.

> 
> 
> I took the liberty of updating the CSR to add the missing comma, so I could 
> mark it as Reviewed. You can Finalize the CSR when you are ready.

Thank you, I have committed new changes according to your review. I have also 
moved the CSR to "finalize".

Let me know if I need to do something else, sorry for the delay.

-

PR: https://git.openjdk.java.net/jfx/pull/6

Re: [Rev 07] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

> Allright, this is a fix for JDK-8207957

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 79817025: Remove trailing spaces

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/9991ec48..79817025

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.07
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.06-07

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/6.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/6/head:pull/6

PR: https://git.openjdk.java.net/jfx/pull/6

Re: [Rev 06] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Hadzic Samir]

> Allright, this is a fix for JDK-8207957

The pull request has been updated with 1 additional commit.

-

Added commits:
 - 9991ec48: Minor typo fix upon Kevin's review

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/6/files
  - new: https://git.openjdk.java.net/jfx/pull/6/files/e1a9d2d0..9991ec48

Webrevs:
 - full: https://webrevs.openjdk.java.net/jfx/6/webrev.06
 - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.05-06

  Stats: 27 lines in 2 files changed: 8 ins; 3 del; 16 mod
  Patch: https://git.openjdk.java.net/jfx/pull/6.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/6/head:pull/6

PR: https://git.openjdk.java.net/jfx/pull/6

Re: [Rev 02] RFR: 8207759: VK_ENTER not consumed by TextField when default Button exists

[Ajit Ghaisas]

On Mon, 16 Dec 2019 08:53:10 GMT, Jeanette Winzenburg  
wrote:

>> This is a fix for https://bugs.openjdk.java.net/browse/JDK-8207759
>> 
>> The issue is that default/cancel button on a scene are triggered even though 
>> a onKeyPressed handler for ENTER/CANCEL consumes the keyEvent. See the bug 
>> for details on both cause and fix.
>> 
>> There are additional/changed tests to verify the fix. All old tests are 
>> passing.
> 
> The pull request has been updated with 1 additional commit.



-

Marked as reviewed by aghaisas (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/15