[jfx15] Integrated: 8249537: Update copyright header for files modified in 2020
On Mon, 17 Aug 2020 06:39:08 GMT, Ambarish Rapte wrote: > Update last modified year of copyright headers for files modified in 2020. This pull request has now been integrated. Changeset: 41ff3fa8 Author:Ambarish Rapte URL: https://git.openjdk.java.net/jfx/commit/41ff3fa8 Stats: 92 lines in 91 files changed: 0 ins; 0 del; 92 mod 8249537: Update copyright header for files modified in 2020 Reviewed-by: kcr - PR: https://git.openjdk.java.net/jfx/pull/281
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 12:31:14 GMT, Nir Lisker wrote:
>> Added missing explicit no-arg constructors to classes in package
>> javafx.scene, javafx.css and javafx.stage.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/TableFocusModel.java
> line 51:
>
>> 50:
>> 51: /**
>> 52: * Causes the item at the given index to receive the focus.
>
> Please add a missing `)` for the class docs on line 30.
The change to the class header seems unrelated. It would be a good thing to
fix, but could be done as part of a
follow-on doc cleanup issue.
> modules/javafx.graphics/src/main/java/javafx/application/Preloader.java line
> 121:
>
>> 120: public Preloader() {
>> 121: }
>> 122:
>
> Not sure that "default" means anything here. I don't see any configuration.
Right, but isn't that true of most of the classes, including those in the two
related bugs that were fixed?
Worth noting is that the JDK chose different language for abstract classes than
concrete ones. For abstract classes,
they just use the following language:
* Constructor for subclasses to call.
And for concrete ones:
Constructs a {@code Foo}.
This gets around the problem of whether or not `default` adds any useful
information since it is the only constructor.
Not saying we should adopt this now, but just adding another data point.
> modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 83:
>
>> 82:
>> 83: /**
>> 84: * There is only one PseudoClass instance for a given pseudoClass.
>
> 1. Is having a public constructor for this class a good idea? Instances are
> created by a factory method.
> 2. Both methods in this class need documentation update. `getPseudoClass` has
> a poor method description and the
> formatting of the `@return` tag is wrong (lowercase and no period).
> `getPseudoClassName` is missing a description.
Yes, this constructor is needed. `PseudoClass` is abstract, so it's constructor
is just there for subclasses to call
(note that for a constructor of an abstract class, `public` and `protected`
mean the same thing).
As for the other methods in the class, I agree that they should be
changed...but in a follow-up issue.
> modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 51:
>
>> 50:
>> 51: private static class UniversalSelector {
>> 52: private static final Selector INSTANCE =
>
> Is a public constructor suitable in this class? `createSelector(String)` is
> the factory. There are public abstract
> methods here, on the other hand, so I don't know what the design idea is. The
> class docs should be updated too to say
> how to use this class.
This one does not look like it should be public. It seems quite by accident,
since the only two classes that subclass
`Selector` are in the same package and are constructed by factory methods
(their constructors are package-scope).
This seems like a good candidate for removal (via the deprecate-for-removal,
remove route).
> modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 95:
>
>> 94:
>> 95: /**
>> 96: * Convert from the parsed CSS value to the target property type.
>
> Looks like a constructor is fine here if the predefined factories are not
> suitable, but I'm not familiar with this.
This one needs to be public since it is subclassed in many other classes.
> modules/javafx.graphics/src/main/java/javafx/css/converter/ShapeConverter.java
> line 51:
>
>> 50: }
>> 51:
>> 52: @Override public Shape convert(ParsedValue value,
>> Font font) {
>
> Looks like a singleton class.
Agreed. This is another candidate for removal.
-
PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary wrote: > Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. I think that two of the classes have implicit constructors that are there by accident. Once we get agreement, I'll file a follow-on bug for those, and those changes should be reverted. As for the other comments, I would prefer a follow-up bug for any doc cleanup that isn't part of adding in an explicit constructor. Tempting as it might be to fix it, it seems out of scope. That leaves the question about the wording. The more I think about it the more I like what the JDK did as opposed to what we did. The question then becomes: if we agree that it's a better pattern, do we adopt it here and then clean it up for the other two batches or just leave it as is for now, and file one cleanup bug for later. I'd like to hear from @aghaisas and @nlisker - PR: https://git.openjdk.java.net/jfx/pull/283
Re: [jfx15] RFR: 8249537: Update copyright header for files modified in 2020
On Mon, 17 Aug 2020 06:39:08 GMT, Ambarish Rapte wrote: > Update last modified year of copyright headers for files modified in 2020. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/281
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Tue, 18 Aug 2020 17:49:06 GMT, Nir Lisker wrote: > The classes should be inspected to see if one is needed and if its doc is > suitable. This is an excellent point. One of the main reasons for not relying on implicit, no-arg constructors is that you might get a public constructor where one isn't intended. See [JDK-8229472](https://bugs.openjdk.java.net/browse/JDK-8229472) and [JDK-8240688](https://bugs.openjdk.java.net/browse/JDK-8240688) for what is needed if we determine that there should not be a public constructor. To that end, I'll go through it more carefully and comment on this specific question. - PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary wrote: > Added missing explicit no-arg constructors to classes in package > javafx.scene, javafx.css and javafx.stage. @nlisker raises some good issues, so they should be addressed - Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/283
Re: RFR: 8251353: Many javafx scenegraph classes have implicit no-arg constructors
On Mon, 17 Aug 2020 11:16:55 GMT, Bhawesh Choudhary
wrote:
> Added missing explicit no-arg constructors to classes in package
> javafx.scene, javafx.css and javafx.stage.
I've completed a partial review. I think that just mechanically adding a
constructor with the same doc everywhere is
not a good approach. The classes should be inspected to see if one is needed
and if its doc is suitable. See my
comments on some of the classes I've looked at.
modules/javafx.controls/src/main/java/javafx/scene/control/TableFocusModel.java
line 51:
> 50:
> 51: /**
> 52: * Causes the item at the given index to receive the focus.
Please add a missing `)` for the class docs on line 30.
modules/javafx.controls/src/main/java/javafx/scene/control/TableSelectionModel.java
line 46:
> 45:
> 46: /**
> 47: * Convenience function which tests whether the given row and column
> index
Same missing `)`
modules/javafx.graphics/src/main/java/javafx/css/PseudoClass.java line 83:
> 82:
> 83: /**
> 84: * There is only one PseudoClass instance for a given pseudoClass.
1. Is having a public constructor for this class a good idea? Instances are
created by a factory method.
2. Both methods in this class need documentation update. `getPseudoClass` has a
poor method description and the
formatting of the `@return` tag is wrong (lowercase and no period).
`getPseudoClassName` is missing a description.
modules/javafx.graphics/src/main/java/javafx/css/Selector.java line 51:
> 50:
> 51: private static class UniversalSelector {
> 52: private static final Selector INSTANCE =
Is a public constructor suitable in this class? `createSelector(String)` is the
factory. There are public abstract
methods here, on the other hand, so I don't know what the design idea is. The
class docs should be updated too to say
how to use this class.
modules/javafx.graphics/src/main/java/javafx/css/StyleConverter.java line 95:
> 94:
> 95: /**
> 96: * Convert from the parsed CSS value to the target property type.
Looks like a constructor is fine here if the predefined factories are not
suitable, but I'm not familiar with this.
modules/javafx.graphics/src/main/java/javafx/css/converter/ShapeConverter.java
line 51:
> 50: }
> 51:
> 52: @Override public Shape convert(ParsedValue value, Font
> font) {
Looks like a singleton class.
modules/javafx.graphics/src/main/java/javafx/scene/input/ClipboardContent.java
line 45:
> 44: */
> 45: public ClipboardContent() {
> 46: }
Not sure that "default" means anything here. I don't see any configuration.
modules/javafx.graphics/src/main/java/javafx/application/Preloader.java line
121:
> 120: public Preloader() {
> 121: }
> 122:
Not sure that "default" means anything here. I don't see any configuration.
-
Changes requested by nlisker (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/283
Integrated: 8251352: Many javafx.base classes have implicit no-arg constructors
On Mon, 17 Aug 2020 09:23:34 GMT, Bhawesh Choudhary wrote: > Added missing explicit no-arg constructors to classes in package > javafx.beans.property, javafx.collections, javafx.util > and javafx.util.converter in module javafx.base. This pull request has now been integrated. Changeset: b25ffc7a Author:Bhawesh Choudhary Committer: Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/b25ffc7a Stats: 224 lines in 35 files changed: 0 ins; 224 del; 0 mod 8251352: Many javafx.base classes have implicit no-arg constructors Reviewed-by: nlisker, kcr - PR: https://git.openjdk.java.net/jfx/pull/282
Re: PageOrientation partially ignored?
I created a minimal example to illustrate the issue. If I print with
'orientation' set to PORTRAIT i get the expected result [1]. But if i
print with 'orientation' set to LANDSCAPE, then i still get a page in
portrait orientation and with mixed up margins [2]. Picture [3]
illustrates what i would expect.
=
[1] https://pasteboard.co/JmUr91w.png (Portrait)
[2] https://pasteboard.co/JmUryc2.png (Landscape actual result)
[3] https://pasteboard.co/JmUsc04.png (Landscape expected result)
=
public class TestApp extends Application {
private final PageOrientation orientation = PageOrientation.LANDSCAPE;
@Override
public void start(Stage stage) throws Exception {
Button printButton = new Button("Print Example Page");
Scene scene = new Scene(printButton);
stage.setScene(scene);
Rectangle rect = new Rectangle(0,0,null);
rect.setStrokeType(StrokeType.INSIDE);
rect.setStrokeWidth(4);
rect.setStroke(Color.RED);
Circle circle = new Circle(0, 0, 40, null);
circle.setStrokeWidth(4);
circle.setStroke(Color.BLUE);
Group group = new Group(rect, circle);
printButton.setOnAction(e -> {
Printer printer = Printer.getDefaultPrinter();
PrinterJob job = PrinterJob.createPrinterJob(printer);
System.out.println(printer);
System.out.println("Supports A4 paper: " +
printer.getPrinterAttributes().getSupportedPapers().contains(Paper.A4));
System.out.println("Supports Portrait: " +
printer.getPrinterAttributes().getSupportedPageOrientations().contains(PageOrientation.PORTRAIT));
System.out.println("Supports Landscape: " +
printer.getPrinterAttributes().getSupportedPageOrientations().contains(PageOrientation.LANDSCAPE));
PageLayout layout = printer.createPageLayout(Paper.A4, orientation, 160, 40, 20, 80);
job.getJobSettings().setPageLayout(layout);
layout = job.getJobSettings().getPageLayout();
System.out.println(layout);
rect.setWidth(layout.getPrintableWidth());
rect.setHeight(layout.getPrintableHeight());
boolean success = job.printPage(group);
if (success) {
job.endJob();
}
});
stage.show();
}
public static void main(String[] args) {
Application.launch(TestApp.class, args);
}
}
On 18.08.2020 06:42, Phil Race wrote:
what printer, driver, and os?
did you call any methods to verify the values being passed are supported ?
-Phil.
On Aug 17, 2020, at 9:01 PM, Tobias Oelgarte wrote:
I just experimented with the JavaFX printing API and I'm confused about the
results.
If I request to print in landscape format the resulting page is still in
portrait mode, but rotated by 90 degrees and with mirrored borders/margins. Is
this intentional or should I file a bug report?
Simplified Example:
Printer printer = Printer.getDefaultPrinter();
PageLayout layout = printer.createPageLayout(Paper.A4,
PageOrientation.LANDSCAPE, 80, 20, 10, 40);
PrinterJob job = PrinterJob.createPrinterJob(printer);
job.getJobSettings().setPageLayout(layout);
boolean success = job.printPage(someNode);
if (success) {
job.endJob();
}
What i would expect would be an A4 page in landscape orientation, with a top
border (long side) of 10, a bottom border of 40, a left border of 80 and a
right border of 20. The content/node will not be rotated.
But what i get is the following. The paper size is A4 as requested. The
orientation is portrait (not as requested). The top border (long side) is 40,
the bottom border is 10, the left border is 20, the right border is 80. The
content/node is rotated by 90 degrees to the left.
My guess is that the orientation is partially ignored, resulting in the
confusing margins.
--
Tobias Oelgarte
Mail: tobias.oelga...@gmail.com
