JDK-8155998 : Encapsulate JavaFX impl_* implementation methods in stage package
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: graphics
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2016-05-04
  • Updated: 2016-05-18
  • Resolved: 2016-05-18
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Fixed
Related Reports
Blocks :  
Blocks :  
Description
This is to make JDK-8144585 more manageable and easier for reviewer by breaking it into smaller pieces of work.

Comments
Changeset: a5c524369e14 Author: ckyang Date: 2016-05-17 19:23 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/a5c524369e14
18-05-2016

+1
18-05-2016

This is likely I have fixed mine and forgot to regenerate the old webrev v2. I will redo a full test run before I commit if it passes your testing.
17-05-2016

The delta looks good. I noticed one white space issue (trailing white space) in the v2 patch that isn't in the delta, so you will need to fix that. Running tests now.
17-05-2016

Thanks for the review. Here is revised webrev that includes all your comments: http://cr.openjdk.java.net/~ckyang/JDK-8155998/webrev.02/ Here is the delta between v1 and v2: http://cr.openjdk.java.net/~ckyang/JDK-8155998/webrev01-02.diff/ This latest webrev passes full test run on Linux.
17-05-2016

Overall this looks very good now. A few things I noticed: 1. I think that the accessor methods that call the "doXxxxx" methods should themselves be named "doXxxxx" for consistency. This would help underscore the fact that ordinary code should not call them, but that they are only called by the *Impl helper methods. 2. The following file can be reverted, since there are no changes other than an (unneeded in this case) Copyright header change: modules/graphics/src/main/java/javafx/stage/Popup.java 3. You might want to call the following get/set methods directly, since they are in the same package? Other than the one in Window.java itself, this is optional, but might be cleaner. PopupWindow.java: if (visible && (WindowHelper.getPeer(this) == null)) { ... WindowHelper.setPeer(this, toolkit.createTKPopupStage(this, popupStyle, WindowHelper.getPeer(getOwnerWindow()), acc)); WindowHelper.setPeerListener(this, new PopupWindowPeerListener(PopupWindow.this)); ... WindowHelper.setFocused(this, ownerWindowValue.isFocused()); ... WindowHelper.setFocused(this, false); ... WindowHelper.setFocused(this, ownerWindowValue.isFocused()); Stage.java: if (WindowHelper.getPeer(this) != null) WindowHelper.getPeer(this).setFullScreen(value); ... if (WindowHelper.getPeer(Stage.this) != null) { WindowHelper.getPeer(Stage.this).setIcons(platformImages); ... etc. Window.java: TKStage peer = WindowHelper.getPeer(window); 4. The following should be more strongly worded: * Note: This method should only be called via its accessor method. Maybe something like: * Note: This method MUST only be called via its accessor method. 5. The following could use a comment indicating what is stored here (the singleton instance of the WindowHelper subclass corresponding to the subclass of this instance of Window). Window.java: private WindowHelper windowHelper = null; 6. You need to remove the javadoc comments -- replace '/**' with '/*' -- from the following methods: PopupWindow.java: ObservableList<Node> getContent() void doVisibleChanged(boolean visible) Stage.java: void setPrimary(boolean primary) void setImportant(boolean important) Window.java: private WindowPeerListener peerListener; private TKStage peer; TKStage getPeer() 6. Indentation is wrong for the following: EmbeddedWindowHelper.java: 50 setHelper(embeddedWindow, getInstance());
17-05-2016

Here is the revised webrev after discussion with Kevin: http://cr.openjdk.java.net/~ckyang/JDK-8155998/webrev.01/
17-05-2016

Chien and I went over this this afternoon, and found a few things that are a bit inconsistent, as well as desired name change for the actual methods that were formerly impl_*, so Chien will produce a new version.
16-05-2016

+1
16-05-2016

Please review the purposed fix: http://cr.openjdk.java.net/~ckyang/JDK-8155998/webrev.00/
16-05-2016