Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[Kevin Rushforth]

The only additional comments I have are couple typos and a white-space 
issue:


1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:


modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be 
changed to spaces:


modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
public static void setUpClass() {
>>>System.err.println("SelectBindingTest : log messages are 
expected from these tests.");



All my testing looks good. With this patch I am now able to run 
applications with OpenJDK 10 + a standalone FX SDK with no qualified 
exports on the command line (as long as it doesn't use Swing interop).


-- Kevin


Ajit Ghaisas wrote:

Hi Kevin, Mandy and Daniel,

Please review the changeset that removes dependency on sun.util.logging 
package from JavaFX code.

Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/

Request you to review.

Regards,
Ajit
  

Re: Proposal For Inclusion of Robot and ParametersImpl in the Public API

[Michael Ennen]

Kevin,

I believe I followed all of your suggestions, except the one with a
re-usable WritableImage.
If you want me to implement that as well, I can, but you seemed unsure
about the necessity
of it.

Hopefully it is easy for you to review using the GitHub PR:
https://github.com/javafxports/openjdk-jfx/pull/36
as it takes less effort for me as the manual process of converting to a hg
patch and importing it to generate
a webrev is somewhat laborious.

Once you approve the changes I can of course generate a hg patch, commit,
and webrev.

Since you suggested the params to be doubles I took the initiative and
changed the params
"down the line" to the native API calls, where appropriate. What I mean is,
take the Mac
GlassRobot.m implementation, the native `_mouseMove` implementation uses
`CGEventCreateMouseEvent`
which expects a CGPoint for the (x,y) location, which uses floats. So in
this case the doubles
are "down-casted" to floats to interact the native API, and then
"up-casted" to doubles when
returned by the public Robot class.

For other native API implementations the double params are "down-casted"
all the way to
ints if that's what the native API expects in the following sense:

 (starts as double)  --> Downcasted to what the native API wants
(sometimes int, sometimes float)
 | <-- Always upcasted to double as that's
what public Robot API uses
public Robot class <-> private GlassRobot <-> platform-specific Java class
(e.g. MacRobot.java) <-> native implementation (e.g. MacRobot.m)

This way the values returned by, for example, getMouseX and getMouseY, more
closely
match the actual values of the system, and the double params given to, for
example, mouseMove,
are used to provide better accuracy when the native API supports it, but if
not, the double
is simply casted to an int the extra precision does nothing. In other
words, calling robot.mouseMove(75.5, 75.5)
will try and pass the extra accuracy along to the native API if it expects
it, but if not, won't.

Hopefully that makes sense.

Thanks!

On Thu, Mar 22, 2018 at 5:54 AM, Kevin Rushforth  wrote:

>
>
> Michael Ennen wrote:
>
> Quick question:
>
> Currently a Robot is created by calling `Application.createRobot` which
> delegates
> to the underlying platform-specific application class (GtkApplication,
> WinApplication, etc.)
> via `com.sun.glass.ui.Application.GetApplication().createRobot();`
>
>
> I just meant that the Toolkit class was a convenient place to call
> com.sun.glass.ui.Application.GetApplication().createRobot() -- something
> like this:
>
> Toolkit:
>
> public GlassRobot createRobot() {   // or could be abstract
> throw new UnsupportedOperationException("not implemented");
> }
>
> QuantumToolkit:
>
> public GlassRobot createRobot() {
> return com.sun.glass.ui.Application.GetApplication().createRobot();
> }
>
> The reason for doing it there is because it already has the mechanism for
> handling QuantumToolkit versus StubToolkit. Otherwise you wouldn't need the
> extra level of indirection.
>
> -- Kevin
>
>
> You suggest moving this to the Toolkit class? If GtkRobot, WinRobot, etc.
> all extend
> the abstract base class GlassRobot (the peer), how will Toolkit be able to
> instantiate
> the correct, platform-specific class? Sorry, I got stuck trying to
> implement this part
> of your review. The rest is easy for me to follow and will be no problem.
>
>
> On Tue, Mar 20, 2018 at 4:28 PM, Kevin Rushforth <
> kevin.rushfo...@oracle.com> wrote:
>
>> Hi Michael,
>>
>> Here is some quick feedback.
>>
>> I think what you have is heading in the right direction as far as the
>> public API goes. I'd like to get some feedback from other developers as
>> well. I would want to make sure that the API meets the needs of multiple
>> developers.
>>
>> I took a look at the public class and as I may have mentioned earlier, it
>> will help to split the API and the implementation even further, by creating
>> a peer object as we do for Scene, Stage, etc., rather than having the glass
>> platform implementation directly subclass the public Robot class.
>>
>> Then you can easily delegate to the Glass Robot peer without having any
>> implementation leak into the public API (e.g., the overridden create and
>> dispose methods).
>>
>> So what you would have in that case is something more like this:
>>
>> public final class Robot {
>>
>> private final GlassRobot peer;
>>
>> public Robot() {
>> // Ensure we have proper permission for creating a robot.
>> final SecurityManager sm = System.getSecurityManager();
>> if (sm != null) {
>> sm.checkPermission(CREATE_ROBOT_PERMISSION);
>> }
>>
>> Application.checkEventThread();
>>
>> peer = Toolkit.createRobot();
>> }
>>
>> // NOTE: for the rest, the peer can do the thread check
>>
>> public void keyPress(KeyCode keyCode) {
>> peer.keyPress(keyCode);
>> }
>>
>> 

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[Kevin Rushforth]

mandy chung wrote:



On 3/23/18 10:51 AM, Kevin Rushforth wrote:

Hi Daniel,

Thanks for the review.

I like the idea of removing the unused levels and methods.

As for directly using System.Logger.Level, we have enough usages of 
the Level and convenience logging methods (e.g., "warn", "fine", 
etc.), that I think it's better to file a follow-up issue (to 
minimize the changes and so it would be more feasible to review the 
existing changes). The idea of doing a refactor / rename of the 
convenience methods and level names to match seems like a good one 
for that follow-up JBS issue.




When you do the clean up, I suggest to drop PlatformLogger completely 
and directly use System.Logger (in addition to System.Logger.Level)


If not too hard, that might be a good way to go.

-- Kevin



Mandy


-- Kevin


Daniel Fuchs wrote:

Hi Ajit,

I have two remarks,

1. I wonder if it's wise to keep the old unused levels like e.g.
   Level.CONFIG in your new PlatformLogger.

   The sun.util.logging.PlatformLogger had a bridge that allowed
   it to transfer these levels unchanged to java.logging when
   java.logging was the backend.

   Your new PlatformLogger does not have these bridges.
   Which means that code that might log with Level.CONFIG
   will in reality log with Level.DEBUG, which will be translated
   to Level.FINE when java.logging is the backend.

   So with the old sun.u.l.PlatformLogger, logging with Level.CONFIG
   would have ended up logging with Level.CONFIG in java.logging,
   but with your new implementation, this will end up as Level.FINE
   (as DEBUG maps to FINE when java.logging is the backend).

   AFAICS - there's no code in JavaFX (at least in the files present
   in your webrev) that logs with Level.CONFIG.
   So why not just remove these unused levels from your implementation
   of PlatformLogger? You certainly don't want new code
   to start using PlatformLogger::config (as that's actually
   an alias to DEBUG).
   The best way of avoiding future usage when there's none
   today is just to remove these problematic API point.

2. I understand the desire of minimizing the code changes, and
   introducing a PlatformLogger class that mimics the old
   sun.util.logging.PlatformLogger certainly achieve this goal.
   Now the interesting thing is: how many log statements are there
   throughout the JavaFX code base?

   If there are not too many - then you might want consider
   changing them to use directly the levels provided by
   System.Logger - and then you might want to use the
   "Refactor -> Rename" option of your IDE to simply
   rename the methods

   PlatformLogger::severe to PlatformLogger::error
   PlatformLogger::fine to PlatformLogger::debug
   PlatformLogger::finer to PlatformLogger::trace
   PlatformLogger::finest to PlatformLogger::trace


Otherwise looks mostly good - I guess.

best regards,

-- daniel

On 23/03/2018 16:34, Ajit Ghaisas wrote:

Hi Kevin, Mandy and Daniel,

 Please review the changeset that removes dependency on 
sun.util.logging package from JavaFX code.


 Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
 Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/

 Request you to review.

Regards,
Ajit

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[Daniel Fuchs]

Hi Kevin,

This sounds reasonable to me.

best regards,

-- daniel

On 23/03/2018 17:51, Kevin Rushforth wrote:

Hi Daniel,

Thanks for the review.

I like the idea of removing the unused levels and methods.

As for directly using System.Logger.Level, we have enough usages of the 
Level and convenience logging methods (e.g., "warn", "fine", etc.), that 
I think it's better to file a follow-up issue (to minimize the changes 
and so it would be more feasible to review the existing changes). The 
idea of doing a refactor / rename of the convenience methods and level 
names to match seems like a good one for that follow-up JBS issue.


-- Kevin

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[Kevin Rushforth]

Hi Daniel,

Thanks for the review.

I like the idea of removing the unused levels and methods.

As for directly using System.Logger.Level, we have enough usages of the 
Level and convenience logging methods (e.g., "warn", "fine", etc.), that 
I think it's better to file a follow-up issue (to minimize the changes 
and so it would be more feasible to review the existing changes). The 
idea of doing a refactor / rename of the convenience methods and level 
names to match seems like a good one for that follow-up JBS issue.


-- Kevin


Daniel Fuchs wrote:

Hi Ajit,

I have two remarks,

1. I wonder if it's wise to keep the old unused levels like e.g.
   Level.CONFIG in your new PlatformLogger.

   The sun.util.logging.PlatformLogger had a bridge that allowed
   it to transfer these levels unchanged to java.logging when
   java.logging was the backend.

   Your new PlatformLogger does not have these bridges.
   Which means that code that might log with Level.CONFIG
   will in reality log with Level.DEBUG, which will be translated
   to Level.FINE when java.logging is the backend.

   So with the old sun.u.l.PlatformLogger, logging with Level.CONFIG
   would have ended up logging with Level.CONFIG in java.logging,
   but with your new implementation, this will end up as Level.FINE
   (as DEBUG maps to FINE when java.logging is the backend).

   AFAICS - there's no code in JavaFX (at least in the files present
   in your webrev) that logs with Level.CONFIG.
   So why not just remove these unused levels from your implementation
   of PlatformLogger? You certainly don't want new code
   to start using PlatformLogger::config (as that's actually
   an alias to DEBUG).
   The best way of avoiding future usage when there's none
   today is just to remove these problematic API point.

2. I understand the desire of minimizing the code changes, and
   introducing a PlatformLogger class that mimics the old
   sun.util.logging.PlatformLogger certainly achieve this goal.
   Now the interesting thing is: how many log statements are there
   throughout the JavaFX code base?

   If there are not too many - then you might want consider
   changing them to use directly the levels provided by
   System.Logger - and then you might want to use the
   "Refactor -> Rename" option of your IDE to simply
   rename the methods

   PlatformLogger::severe to PlatformLogger::error
   PlatformLogger::fine to PlatformLogger::debug
   PlatformLogger::finer to PlatformLogger::trace
   PlatformLogger::finest to PlatformLogger::trace


Otherwise looks mostly good - I guess.

best regards,

-- daniel

On 23/03/2018 16:34, Ajit Ghaisas wrote:

Hi Kevin, Mandy and Daniel,

 Please review the changeset that removes dependency on 
sun.util.logging package from JavaFX code.


 Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
 Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/

 Request you to review.

Regards,
Ajit

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[Daniel Fuchs]

Hi Mandy,

On 23/03/2018 17:00, mandy chung wrote:
System::getLogger should return the same instance if it has been 
created.  


Not necessarily. j.u.l does that, but System::getLogger may return a
new cheap wrapper. As you noted, if JavaFX sources only creates a
handful of loggers, the cost of maintaining a Map and
dealing with the complexity of weak reference may not be justified.

Best regards,

-- daniel

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[Daniel Fuchs]

Hi Ajit,

I have two remarks,

1. I wonder if it's wise to keep the old unused levels like e.g.
   Level.CONFIG in your new PlatformLogger.

   The sun.util.logging.PlatformLogger had a bridge that allowed
   it to transfer these levels unchanged to java.logging when
   java.logging was the backend.

   Your new PlatformLogger does not have these bridges.
   Which means that code that might log with Level.CONFIG
   will in reality log with Level.DEBUG, which will be translated
   to Level.FINE when java.logging is the backend.

   So with the old sun.u.l.PlatformLogger, logging with Level.CONFIG
   would have ended up logging with Level.CONFIG in java.logging,
   but with your new implementation, this will end up as Level.FINE
   (as DEBUG maps to FINE when java.logging is the backend).

   AFAICS - there's no code in JavaFX (at least in the files present
   in your webrev) that logs with Level.CONFIG.
   So why not just remove these unused levels from your implementation
   of PlatformLogger? You certainly don't want new code
   to start using PlatformLogger::config (as that's actually
   an alias to DEBUG).
   The best way of avoiding future usage when there's none
   today is just to remove these problematic API point.

2. I understand the desire of minimizing the code changes, and
   introducing a PlatformLogger class that mimics the old
   sun.util.logging.PlatformLogger certainly achieve this goal.
   Now the interesting thing is: how many log statements are there
   throughout the JavaFX code base?

   If there are not too many - then you might want consider
   changing them to use directly the levels provided by
   System.Logger - and then you might want to use the
   "Refactor -> Rename" option of your IDE to simply
   rename the methods

   PlatformLogger::severe to PlatformLogger::error
   PlatformLogger::fine to PlatformLogger::debug
   PlatformLogger::finer to PlatformLogger::trace
   PlatformLogger::finest to PlatformLogger::trace


Otherwise looks mostly good - I guess.

best regards,

-- daniel

On 23/03/2018 16:34, Ajit Ghaisas wrote:

Hi Kevin, Mandy and Daniel,

 Please review the changeset that removes dependency on sun.util.logging 
package from JavaFX code.

 Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
 Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/

 Request you to review.

Regards,
Ajit

Re: [11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[mandy chung]

On 3/23/18 9:34 AM, Ajit Ghaisas wrote:

Hi Kevin, Mandy and Daniel,

 Please review the changeset that removes dependency on sun.util.logging 
package from JavaFX code.

 Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
 Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/



buildSrc/addExports
   FX modules are compiled together and I don't expect these 
--add-exports are needed.  I suspect it's because of the boot JDK and 
this is a temporary dance?



PlatformLogger.java
 150 public static synchronized PlatformLogger getLogger(String name) {

This keeps the weak reference to all PlatformLogger created.  A 
simplification is to return


   new PlatformLogger(System.getLogger(name));

System::getLogger should return the same instance if it has been 
created.  I also suspect the caller of PlatformLogger::getLogger keeps a 
strong reference and calls it once.


I recalled there were some native methods calling the Java API to set 
level.  It has been a while back since I looked at it and things miight 
have changed.  Is there such reference any more?


Other than the above comments, this change looks good.

Mandy

[11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[Ajit Ghaisas]

Hi Kevin, Mandy and Daniel,

Please review the changeset that removes dependency on sun.util.logging 
package from JavaFX code.

Bug :  https://bugs.openjdk.java.net/browse/JDK-8195799
Fix :  http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/

Request you to review.

Regards,
Ajit