Integrated: 8274669: Dialog sometimes ignores max height
On Sat, 2 Oct 2021 23:53:02 GMT, Marius Hanl wrote: > This PR fixes a visual glitch which may happen when showing a dialog. > When a max height is set and the pref height of the dialog content is bigger > the dialog starts to flicker between the max height and the pref height. > > This happens because in one case the height is not bound between the min and > the max height (-> max height is ignored). > - First layout pass: The dialog will incorrectly resize to a the pref height, > which is bigger than the max height > - Second layout pass: The dialog will correctly resize to the max height > - repeat > > The fix is to bound the height there as well (Fix in **layoutChildren()**). > Example where a red stackpane has a bigger pref height then the max height of > the dialog (see also example in ticket): > > https://user-images.githubusercontent.com/66004280/135734463-03b422f4-710d-4436-9715-c91bdb768d76.mp4 > > * only the max height test fails before and succeeds after. This pull request has now been integrated. Changeset: 717cfdc8 Author:Marius Hanl Committer: Ajit Ghaisas URL: https://git.openjdk.java.net/jfx/commit/717cfdc85817aee57d5326e592340c849382d7a4 Stats: 108 lines in 2 files changed: 107 ins; 0 del; 1 mod 8274669: Dialog sometimes ignores max height Reviewed-by: aghaisas - PR: https://git.openjdk.java.net/jfx/pull/637
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v11]
On Sun, 24 Oct 2021 17:23:40 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Merge branch 'openjdk:master' into fix-8255015 > - 8255015: Comments corrected > - 8255015: Comment about copying pixel scale factors corrected > - 8255015: Tabs removed from PointLightIllumination.java > - Merge branch 'openjdk:master' into fix-8255015 > - 8255015: JUnit Test class added. > - Merge branch 'openjdk:master' into fix-8255015 > - Merge branch 'openjdk:master' into fix-8255015 > - Merge branch 'openjdk:master' into fix-8255015 > - 8255015: Copy pixel scale factors from graphics object to subscene > graphics so that the position of lights will be scaled correctly on retina > displays The fix and the test looks good. I left a couple minor comments on the test. I ran the test on macOS with retina. I'll double-check on the other platforms, but I think this is about ready to go. tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java line 67: > 65: private static final intLOWER_CORNER_Y = (int) > (SCENE_WIDTH_HEIGHT * (1 - CORNER_FACTOR)); > 66: private static final double COLOR_TOLERANCE= 0.07; > 67: private static Scene testScene; This is created on one thread and tested on another (to see whether it's already been created), so I recommend making it `volatile` (i.e., `private static volatile ...`). Also, you might want to explicitly set it to `null` since you rely on it (yes, I know `null` is the default). tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java line 180: > 178: return new Scene(new Group(subScene), SCENE_WIDTH_HEIGHT, > SCENE_WIDTH_HEIGHT); > 179: } > 180: } Minor: there should be a new line at the end of the file. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Mon, 25 Oct 2021 08:30:36 GMT, Ajit Ghaisas wrote:
>> This PR fixes javadoc warnings primarily in javafx.graphics module along
>> with a remaining few in javafx.fxml, javafx.base and javafx.media modules.
>>
>> Note :
>> - The javadoc needs to be generated with the JDK 18 EA build.
>> - There are still few remaining warnings in these modules. The root cause is
>> different and they will be addressed under
>> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996)
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8271090 - fix review comments
Looks good with a couple suggestions on `setScene`. We might want to also file
a follow-up javadoc bug so we can get rid of the javadocs for that method
altogether.
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 781:
> 779: * Sets the value of the {@code scene} property.
> 780: *
> 781: * The {@code Scene} to be rendered on this {@code Window}. There
> can only
Can you add a `` tag here? This will be closer to what an
implicitly-generated setter would do.
modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 798:
> 796: * @defaultValue null
> 797: *
> 798: * @param value the value for the {@code scene} property
Can you add the following tags?
`@see getScene()`
`@see sceneProperty()`
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271091: Missing API docs in UI controls classes [v3]
On Mon, 25 Oct 2021 17:47:38 GMT, Ajit Ghaisas wrote: >> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. >> Note : >> - The javadoc needs to be generated with the JDK 18 EA build. >> - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - >> [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) >> - There are still 20 javadoc warnings remaining in javafx.controls module >> and 3 warnings remaining in javafx.web module. The root cause is different >> and they will be addressed under >> [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > fix review comments Two minor comments. The rest looks great. modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java line 179: > 177: * HTML element. Note that, like the HTML style attribute, this > 178: * variable contains style properties and values and not the > 179: * selector portion of a style rule. It would be good to add a sentence here with the information (which was formerly in the getter) that a value of `null` is converted to the empty string. modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line 508: > 506: * The maximum height of the tabs in the TabPane. > 507: * @return the maximum height of the tabs > 508: */ In reverting this, you introduced a whitespace (indentation) change. - PR: https://git.openjdk.java.net/jfx/pull/646
Aw: Easier handling of ObservableList/Set/Map change events
I like this idea.
Pretty sure some listener I wrote won't handle a permutation correctly.
I may have one question: Is there something that needs to be done in
case of an update (wasUpdated)?
-- Marius
Gesendet: Mittwoch, 20. Oktober 2021 um 21:07 Uhr
Von: "Michael Strau�"
An: "openjfx-dev@openjdk.java.net List"
Betreff: Easier handling of ObservableList/Set/Map change events
I'd like to gauge interest on a small feature addition for JavaFX
collections.
ObservableList (and similarly, ObservableSet/ObservableMap) allows
developers to register ListChangeListeners to observe changes to the
list. In some cases, these changes are applied to another list or
projected into a different form.
Implementing ListChangeListener correctly is quite tricky, especially
if the ObservableList implementation also fires "replace" and
"permutate" events. As a result, it is very easy to implement this
interface wrongly, which often goes undetected when the implementation
is only tested against basic operations like "add" and "remove".
Maybe we could make it easier for developers to get it right more of
the time, by offering pre-built adapters for ListChangeListener,
SetChangeListener and MapChangeListener.
Here's what that could look like:
public abstract class ListChangeListenerAdapter implements
ListChangeListener {
// Basic methods
public abstract void onAdded(int index, E element);
public abstract void onRemoved(int index);
// Optional methods
public void onAdded(int index, List elements);
public void onRemoved(int from, int to);
public void onReplaced(int index, E element);
public void onUpdated(int index, E element);
}
An implementation of this adapter must at the very least implement the
basic `onAdded` and `onRemoved` methods. The adapter will then
correctly map all other change events ("replace" and "permutate") to
these methods. All adapter methods will always be called in exactly
the right order, such that they always refer to the current state of
the list.
An adapter implementation can improve performance characteristics by
overriding the optional `onAdded`, `onRemoved` and `onReplaced`
methods (which map to `addAll`, `remove` and `set` of the List
interface). Optional methods are implemented by default to throw a
private exception in ListChangeListenerAdapter, which is used to
discover whether the methods are overridden and should be called
instead of the basic methods.
Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v3]
On Fri, 22 Oct 2021 10:36:24 GMT, Jeanette Winzenburg wrote: >> cell startEdit is supposed to update the editing location on its associated >> control - was done in ListCell, not in Tree-/TableCell nor TreeCell. >> >> Fix was to add control.edit(..). Note that ListCell was also touched to use >> the exact same method call pattern as the fixed cell types. >> >> Added/unignored cell tests that are failing/passing before/after the fix. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > next try with fixing the formatting Looks good. - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/638
Re: RFR: 8271091: Missing API docs in UI controls classes [v3]
> This PR fixes javadoc warnings in javafx.controls and javafx.web modules. > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - 2 javadoc warnings in javafx.controls TabPane class will be fixed under - > [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085) > - There are still 20 javadoc warnings remaining in javafx.controls module and > 3 warnings remaining in javafx.web module. The root cause is different and > they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/646/files - new: https://git.openjdk.java.net/jfx/pull/646/files/38563835..7cc2de52 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=646=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=646=01-02 Stats: 124 lines in 8 files changed: 48 ins; 21 del; 55 mod Patch: https://git.openjdk.java.net/jfx/pull/646.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/646/head:pull/646 PR: https://git.openjdk.java.net/jfx/pull/646
Re: RFR: 8271091: Missing API docs in UI controls classes [v2]
On Fri, 22 Oct 2021 13:37:38 GMT, Kevin Rushforth wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> javadoc minor corrections
>
> modules/javafx.controls/src/main/java/javafx/scene/control/DatePicker.java
> line 470:
>
>> 468: * {@code CssMetaData} of its superclasses.
>> 469: * @return the {@code CssMetaData}
>> 470: * @since JavaFX 8.0
>
> The class already has an `@since JavaFX 8.0` tag so this is unnecessary. It's
> also unrelated to this fix (and we would need a CSR for any changes to
> `@since` tags).
OK. I will remove this since tag.
> modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java
> line 160:
>
>> 158: /**
>> 159: * Focuses the item at the given index.
>> 160: * @param index the index of the item that needs to be focused
>
> Minor: I recommend removing "that needs" from this `@param`.
Done.
> modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java
> line 201:
>
>> 199: * this specific {@code PopupControl}.
>> 200: * @return the {@code StringProperty} representing the CSS style
>> 201: */
>
> I recommend taking the information from the setter and copying it here. The
> docs can/should then be removed from both the setter and the getter. The
> first sentence should describe the property so does not need to start with
> "Gets the ...". You can add the information about `null` being turned into
> the empty string as a sentence in the Description. The `@defaultValue empty
> string` means that the information in the `@return` of the getter is
> unnecessary.
I could move the description from the setter to the javadoc of `getStyle()`
method as suggested. A `@return` is still needed for `getStyle()` as not to get
the javadoc warning.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 47:
>
>> 45: /**
>> 46: * Gets the default singleton {@code SortEvent}.
>> 47: * @param the type of control
>
> Can you also add this `@param` tag to the class description? I'm a little
> surprised that javadoc doesn't flag it as missing.
Added.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 59:
>
>> 57: /**
>> 58: * Constructs a new {@code Event} with the specified event source,
>> target
>> 59: * and type. If the source or target is set to {@code null}, it is
>> replaced
>
> That should be "a new `{@code SortEvent}` ...". Also, since there is no type
> parameter, you should remove "and type" (and replace the comma after source
> with "and").
Done.
> modules/javafx.controls/src/main/java/javafx/scene/control/SortEvent.java
> line 63:
>
>> 61: *
>> 62: * @param source the event source which sent the event
>> 63: * @param target the target of the scroll to operation
>
> That should be "the target of the event".
Changed.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 405:
>
>> 403: }
>> 404:
>> 405: public final DoubleProperty tabMaxWidthProperty() {
>
> The best practice for properties is to put the description on exactly one of
> the `xxx` private field or the `xxxProperty` method, and not on the setter or
> getter (unless there is a specific need to document something special in the
> setter or getter). There is a separate JBS issue,
> [JDK-8271085](https://bugs.openjdk.java.net/browse/JDK-8271085), tracking the
> fix for the `maxWidth` and `maxHeight` properties.
>
> You can either revert this change, in which case we'll need a new PR for that
> issue (it can be done later), or else modify this change to match the best
> practice as noted in JDK-8271085 and add that issue to this PR. The latter
> will require addressing the other methods in this file as noted in that JBS
> bug.
I prefer reverting this and handling it separately under a separate PR.
> modules/javafx.controls/src/main/java/javafx/scene/control/TabPane.java line
> 501:
>
>> 499: }
>> 500:
>> 501: public final DoubleProperty tabMaxHeightProperty() {
>
> Same comment as above. This needs to be reverted or modified.
I prefer reverting this and handling it separately under a separate PR.
-
PR: https://git.openjdk.java.net/jfx/pull/646
Re: [jfx17u] RFR: 8275837: Change JavaFX release version in jfx17u to 17.0.2
On Mon, 25 Oct 2021 14:27:44 GMT, Kevin Rushforth wrote: > Bump release version in `jfx17u` repo to 17.0.2 for the start of a new > release. Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx17u/pull/15
Re: [jfx11u] RFR: 8275835: Change JavaFX release version in jfx11u to 11.0.14
On Mon, 25 Oct 2021 14:34:39 GMT, Kevin Rushforth wrote: > Bump release version in `jfx11u` repo to 11.0.14 for the start of a new > release. Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx11u/pull/55
[jfx11u] RFR: 8275835: Change JavaFX release version in jfx11u to 11.0.14
Bump release version in `jfx11u` repo to 11.0.14 for the start of a new release. - Commit messages: - 8275835: Change JavaFX release version in jfx11u to 11.0.14 Changes: https://git.openjdk.java.net/jfx11u/pull/55/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx11u=55=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275835 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx11u/pull/55.diff Fetch: git fetch https://git.openjdk.java.net/jfx11u pull/55/head:pull/55 PR: https://git.openjdk.java.net/jfx11u/pull/55
[jfx17u] RFR: 8275837: Change JavaFX release version in jfx17u to 17.0.2
Bump release version in `jfx17u` repo to 17.0.2 for the start of a new release. - Commit messages: - 8275837: Change JavaFX release version in jfx17u to 17.0.2 Changes: https://git.openjdk.java.net/jfx17u/pull/15/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u=15=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275837 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx17u/pull/15.diff Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/15/head:pull/15 PR: https://git.openjdk.java.net/jfx17u/pull/15
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Fri, 22 Oct 2021 15:00:58 GMT, Kevin Rushforth wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8271090 - fix review comments
>
> modules/javafx.graphics/src/main/java/javafx/stage/Window.java line 781:
>
>> 779: * Sets the {@code Scene} of this {@code Window}.
>> 780: * @param value the {@code Scene} to be set
>> 781: */
>
> The javadoc tool automatically generates docs for this setter (you can see
> this from the generated docs without this change), including the property
> description, so this change will actually lose information. I'm guessing that
> the tool warns on this method because the setter is protected (rather than
> public). So we should probably file a bug against the javadoc tool. As for
> how to fix it, you can either suppress the warning (I'm not sure whether
> that's possible), or you can copy the information from the property method
> with a leading sentence that matches what javadoc _would_ generate.
Suppressing this warning is logical - but not possible.
As suggested, I have copied the property information and added the set method
description that was generated by javadoc tool before this fix.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Fri, 22 Oct 2021 15:30:36 GMT, Kevin Rushforth wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/shape/Box.java line 91:
>>
>>> 89: * Default size of the {@code Box}.
>>> 90: */
>>> 91: public static final double DEFAULT_SIZE = 2;
>>
>> This field was exposed by mistake probably. The other shapes don't expose
>> theirs. I recommend to deprecate for removal.
>
> Agreed. That would need to be done in a follow-up issue, and with a CSR. So
> just revert this addition for this PR.
Reverted this change.
Filed [JDK-8275848](https://bugs.openjdk.java.net/browse/JDK-8275848) for
addressing this.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
On Fri, 22 Oct 2021 14:37:14 GMT, Nir Lisker wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8271090 - fix review comments
>
> modules/javafx.media/src/main/java/javafx/scene/media/Track.java line 85:
>
>> 83: /**
>> 84: * Gets the Map containing all known metadata for this
>> Track.
>> 85: * @return the Map containing all known metadata for
>> this Track
>
> We tend to use `{@code }` over ` `, but I don't think it matter.
I have just maintained the local convention used in this file.
-
PR: https://git.openjdk.java.net/jfx/pull/650
Re: RFR: 8271090: Missing API docs in scenegraph classes [v2]
> This PR fixes javadoc warnings primarily in javafx.graphics module along with > a remaining few in javafx.fxml, javafx.base and javafx.media modules. > > Note : > - The javadoc needs to be generated with the JDK 18 EA build. > - There are still few remaining warnings in these modules. The root cause is > different and they will be addressed under > [JDK-8270996](https://bugs.openjdk.java.net/browse/JDK-8270996) Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: 8271090 - fix review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/650/files - new: https://git.openjdk.java.net/jfx/pull/650/files/239c0634..9ff692db Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=650=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=650=00-01 Stats: 38 lines in 6 files changed: 21 ins; 12 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/650.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/650/head:pull/650 PR: https://git.openjdk.java.net/jfx/pull/650
