Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]
On Wed, 18 Jan 2023 06:56:57 GMT, Karthik P K wrote: >> Ignore text condition was not considered in `computePrefHeight` method while >> calculating the Label height. >> >> Added `isIgnoreText` condition in `computePrefHeight` method while >> calculating Label height. >> >> Tests are present for all the ContentDisplay types. Modified tests which >> were failing because of the new height value getting calculated. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Use width while calling prefHeight method. Use graphicHeight instead of > multiple calls to prefHeight @andy-goryachev-oracle please take a look at this PR and let me know if you have any comments on this. - PR: https://git.openjdk.org/jfx/pull/996
Re: RFR: 8251862: Wrong position of Popup windows at the intersection of 2 screens [v4]
On Tue, 31 Jan 2023 19:01:33 GMT, Kevin Rushforth wrote: >> On Windows platforms with more than one screen, a PopupWindow created for a >> Stage that straddles two windows will be drawn with an incorrect position >> and screen scale if the majority of the Stage is on one screen, and the >> popup is positioned on the other screen. In this case, the Stage is drawn >> using the screen scale of the screen that most of the window is on, while >> the popup is drawn using the scale of the screen that it is (typically >> entirely) on. >> >> The most common way this can happen is when you have two screens of a >> different scale with the secondary screen on the left or above the primary >> screen. If you position the Stage such that most of it is still on the >> primary screen (thus the Stage is drawn using the scale of the primary >> screen), with a menu, a control with a context menu, or a control with a >> Tooltip now on the secondary screen, the popup window for the menu or >> Tooltip will be drawn using the screen scale of the secondary window and >> thus won't be positioned or sized correctly relative to the menu bar, or >> control in the main window. >> >> The fix implemented by this PR is to always use the screen of the owner >> window, including the screen scales, when rendering a popup window. This >> matches the behavior of native Windows apps, such as Notepad. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year to 2023 LGTM - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.org/jfx/pull/971
[jfx11u] RFR: 8089986: Menu beeps when mnemonics is used
Almost clean backport, with a difference in copyright year. Otherwise clean. - Commit messages: - 8089986: Menu beeps when mnemonics is used Changes: https://git.openjdk.org/jfx11u/pull/127/files Webrev: https://webrevs.openjdk.org/?repo=jfx11u&pr=127&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8089986 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx11u/pull/127.diff Fetch: git fetch https://git.openjdk.org/jfx11u pull/127/head:pull/127 PR: https://git.openjdk.org/jfx11u/pull/127
[jfx11u] RFR: 8299272: Update copyright header for files modified in 2022
Update copyright year in files modified in year 2022. - Commit messages: - copyright year 2022 Changes: https://git.openjdk.org/jfx11u/pull/126/files Webrev: https://webrevs.openjdk.org/?repo=jfx11u&pr=126&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299272 Stats: 13 lines in 13 files changed: 0 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jfx11u/pull/126.diff Fetch: git fetch https://git.openjdk.org/jfx11u pull/126/head:pull/126 PR: https://git.openjdk.org/jfx11u/pull/126
[jfx17u] RFR: 8299272: Update copyright header for files modified in 2022
Update copyright year in files modified in year 2022. - Commit messages: - copyright year 2022 Changes: https://git.openjdk.org/jfx17u/pull/105/files Webrev: https://webrevs.openjdk.org/?repo=jfx17u&pr=105&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8299272 Stats: 14 lines in 14 files changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.org/jfx17u/pull/105.diff Fetch: git fetch https://git.openjdk.org/jfx17u pull/105/head:pull/105 PR: https://git.openjdk.org/jfx17u/pull/105
Re: RFR: 8088998: LineChart: duplicate child added exception when remove & add a series
On Mon, 30 Jan 2023 12:21:38 GMT, Karthik P K wrote: > While checking for duplicate series addition to the line chart, `setToRemove` > value was not considered before throwing exception. Hence code to handling > the case of adding the removed series was never run. > > Added condition to check `setToRemove` boolean value before throwing > exception. Made changes to call `setChart` method after calling > `seriesBeingRemovedIsAdded`. Otherwise chart will not be drawn for the > series, only points will be plotted. > > This issue is reproducible only when animation is enabled because timeline > will be still running when removed series is added back to the same chart. > Since timeline does not run in unit tests, added system test to validate the > fix. I'll continue with this PR and fix the same issue seen in other charts. - PR: https://git.openjdk.org/jfx/pull/1015
Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]
On Tue, 17 Jan 2023 11:40:47 GMT, John Hendrikx wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use width while calling prefHeight method. Use graphicHeight instead of
>> multiple calls to prefHeight
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/LabeledSkinBase.java
> line 393:
>
>> 391: height = graphic.prefHeight(-1) + gap + textHeight;
>> 392: } else {
>> 393: height = Math.max(textHeight, graphic.prefHeight(-1));
>
> The logic of this code seems to have changed more than just the fixing bug.
> Why are the `prefHeight` functions now called with `-1`? Normally these
> should respect the content bias of the node involved.
>
> Also, I notice that you check `graphic == null`, while the `isIgnoreGraphic`
> function checks quite a bit more:
>
> boolean isIgnoreGraphic() {
> return (graphic == null ||
> !graphic.isManaged() ||
> getSkinnable().getContentDisplay() ==
> ContentDisplay.TEXT_ONLY);
> }
>
> Doing all those operations on `graphic` when for example the `ContentDisplay`
> is `TEXT_ONLY` seems unnecessary. Looking at the rest of the code,
> `graphicHeight` is only used in one branch in your if/elseif/elseif/else.
>
> I also wonder if a simple fix like this would have the same result:
>
>
> - final double textHeight = Utils.computeTextHeight(font, cleanText,
> + final double textHeight = isIgnoreText() ? 0.0 :
> Utils.computeTextHeight(font, cleanText,
> labeled.isWrapText() ? textWidth : 0,
> labeled.getLineSpacing(), text.getBoundsType());
@hjohn please take a look and review the changes made.
-
PR: https://git.openjdk.org/jfx/pull/996
[jfx20] Integrated: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
On Fri, 13 Jan 2023 12:32:53 GMT, Ajit Ghaisas wrote: > This PR adds a warning about inserting Nodes directly into the virtualized > containers such as ListView, TreeView, TableView and TreeTableView. It also > adds code snippets showing the recommended pattern of using a custom cell > factory for each of the virtualized control. This pull request has now been integrated. Changeset: 39d87471 Author:Ajit Ghaisas URL: https://git.openjdk.org/jfx/commit/39d874712205f96befe4af07a332f1e747f3ecc2 Stats: 260 lines in 5 files changed: 241 ins; 2 del; 17 mod 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list Reviewed-by: angorya, kcr - PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8300705: Update boot JDK to 19.0.2 [v3]
On Thu, 2 Feb 2023 02:41:17 GMT, Ambarish Rapte wrote: >> Updating boot JDK to 19.0.2. > > Ambarish Rapte 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 one additional > commit since the last revision: > > boot jdk 19.0.2b7 All good now. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.org/jfx/pull/1019
Re: [jfx20] RFR: 8300705: Update boot JDK to 19.0.2 [v3]
> Updating boot JDK to 19.0.2. Ambarish Rapte has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: boot jdk 19.0.2b7 - Changes: https://git.openjdk.org/jfx/pull/1019/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1019&range=02 Stats: 11 lines in 2 files changed: 0 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1019.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1019/head:pull/1019 PR: https://git.openjdk.org/jfx/pull/1019
Re: [jfx20] RFR: 8300705: Update boot JDK to 19.0.2 [v2]
> Updating boot JDK to 19.0.2. Ambarish Rapte 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 eight additional commits since the last revision: - boot jdk 19.0.2b7 - 8299977: Update WebKit to 615.1 Co-authored-by: Jay Bhaskar Reviewed-by: kcr, sykora - 8237505: RadioMenuItem in ToggleGroup: deselected on accelerator Reviewed-by: angorya, aghaisas - Merge - 8275033: Drag and drop a file produces NullPointerException Cannot read field "dragboard" Reviewed-by: kcr, angorya - 8137244: Empty Tree/TableView with CONSTRAINED_RESIZE_POLICY is not properly resized Reviewed-by: aghaisas - Merge - 8299681: Change JavaFX release version to 21 Reviewed-by: arapte, angorya - Changes: - all: https://git.openjdk.org/jfx/pull/1019/files - new: https://git.openjdk.org/jfx/pull/1019/files/d3bd73c3..d3bd73c3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1019&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1019&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1019.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1019/head:pull/1019 PR: https://git.openjdk.org/jfx/pull/1019
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v9]
On Wed, 1 Feb 2023 10:15:36 GMT, Florian Kirmaier wrote: >> After thinking about this issue for some time, I've now got a solution. >> I know put the scene in the state it is, before is was shown, when the >> dirtyNodes are unset, the whole scene is basically considered dirty. >> This has the drawback of rerendering, whenever a window is "reshown", but it >> restores sanity about memory behaviour, which should be considered more >> important. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8269907 > renamed testclass, based on code review, > readded acidentally removed import Adding to the above, I couldn't find in what cases `Node` might be referred from `NGNode`. Where is this happening? Further, `NGNode` only has a parent pointer, not a full parent/child hierarchy like `Parent` has. This would mean that if you track `NGNode` in `Parent#removed` a big part of the problem is alleviated as only a few parent `NGNode` will be unable to be GC'd, versus entire UI trees when you refer to `Node`. I also did some more digging, and the only thing that the dirty region computation for removed nodes really requires is a `RectBounds`. To do this it accesses only a couple of fields of `NGNode`, no heavy calculations are involved in this part and so this could be done right away -- it's also possible that `Node` may already have this information in some form (basically we need to know the last bounds of the node when it was rendered on screen -- saving a copy in the `doUpdatePeer` call may be sufficient). When the dirty area computation for the parent is performed, you'd only need to apply the transforms and clipping logic that the parent provides on this `RectBounds` -- no further fields of the removed `NGNode` would need to be accessed. - PR: https://git.openjdk.org/jfx/pull/584
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v3]
On Wed, 1 Feb 2023 23:50:07 GMT, Glavo wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > reformat Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader
On Mon, 30 Jan 2023 21:56:45 GMT, Alexander Zuev wrote:
> Change the underlying class XYChart to take into account labels for axes.
> Make label patterns and default axes labels localized.
My initial testing (on Windows) looks good. I have a general question in
addition to the one inline comment. Since you added new i18n resource strings
have you verified that it works for other locales to ensure no regression? (it
should just speak the English string, since there are no translated strings yet)
As for the implementation, I see you've added a (private) shadow property for
the x and y axes and the series, as opposed to making them actual properties
with only the getter being exposed publicly. As long as the fields are only
ever set via the setter method, I think it should work fine the way you have
it, but want to take a second look.
modules/javafx.controls/src/main/java/javafx/scene/chart/XYChart.java line 1403:
> 1401: Object[] args = {seriesName, xAxisName,
> getCurrentX(), yAxisName, getCurrentY()};
> 1402: String retVal = mf.format(args);
> 1403: System.out.println("result = " + retVal);
You'll need to remove this debug print statement.
-
PR: https://git.openjdk.org/jfx/pull/1016
Re: RFR: 8298382: JavaFX ChartArea Accessibility Reader
On Tue, 31 Jan 2023 19:51:51 GMT, Alexander Zuev wrote: > > Can you provide an evaluation of the bug and a description of the fix? > > Done. Thanks. Can you enable GitHub actions for your repo? If you click on the "Checks" tab you 'll see a message from Skara with a pointer as to how to do that (the short answer is you go to "Actions" for your personal fork and click the big green button to enable it). Then you can either manually trigger a test run or push a commit (e.g., an empty commit or a merge from master) to trigger a run. - PR: https://git.openjdk.org/jfx/pull/1016
Re: [External] : Re: Some classes could be sealed
You can't extend these without tampering with internals. Pretty much, yes. Node is an abstract class that requires a concrete implementation to be useful. The set of subclasses that can be used in describing and rendering the scene graph is a finite and known set. The rendering of the scene graph is an implementation detail; each node in the scene graph has a corresponding peer (an NGNode subclass) that is needed to implement various node types (shapes, images, etc). So Node, as well as its abstract subclasses, like Shape, Shape3D, Camera, and LightBase, needs a known concrete subclass in order to do anything. Similarly, Material (which is not a Node) is abstract and has implementation that cannot be provided by an application class. By contrast, Parent can be usefully subclassed. It is a concrete class that is used as a container for other nodes, and has implementation of layout, traversal, bounds computation, etc. --- Kevin On 2/1/2023 2:48 PM, Nir Lisker wrote: For Material and LightBase it's because they are just facades whose implementation is in native code. You can't extend these without tampering with internals. I think that Camera and Shape3D also requires modifying internal stuff, though not at the native level. On Thu, Feb 2, 2023 at 12:38 AM John Hendrikx wrote: I'm curious to know why these classes are not allowed to be subclassed directly, as that may be important in order to decide whether these classes should really be sealed. --John On 01/02/2023 20:37, Kevin Rushforth wrote: I read the spec for sealed classes more carefully, and it turns out we can't make Node sealed. At least not without API changes to SwingNode and MediaView (and implementation changes to Printable in the javafx.web module). All of the classes listed in the "permits" clause must be in the same module, and SwingNode (javafx.swing) and MediaView (javafx.media) extend Node directly they would need to be "permitted" subtypes, but there is no way to specify that. We would also need to do something about the tests that extend Node and run in the unnamed module. So this doesn't seem feasible. We could still seal Shape, Shape3D, LightBase, and Material, since all permitted implementation are in the javafx.graphics module. It may or may not be worth doing that. -- Kevin On 2/1/2023 9:45 AM, Nir Lisker wrote: I'll add that internal classes, mostly NG___ peers, can also benefit from sealing. NGLightBase is an example. Material is another public class that can be sealed. On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth wrote: I agree that we should only seal existing classes that could not have been extended by application classes. The ones I listed in my previous email fit that bill, since an attempt to subclass them will throw an exception when it is used in a scene graph. Each documents that subclassing is disallowed. Btw, we've already started making use of pattern-matching instanceof in the implementation anyway. It would be the first API change that relies on a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. -- Kevin On 2/1/2023 9:06 AM, Philip Race wrote: In the JDK we've only sealed existing classes which provably could not have been extended by application classes, so I'm not sure about this .. also I think that might be the first change that absolutely means FX 21 can only be built with JDK 17 and later .. -phil On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: Yes, sorry, I made the email title in plural, but I meant what Michael said, Node would be sealed permitting only what is needed for JavaFx internally. -- Thiago Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß escreveu: I don't think that's what Thiago is proposing. Only `Node` would be sealed. The following subclasses would be non-sealed: Parent, SubScene, Camera, LightBase, Shape, Shape3D, Canvas, ImageView. And then there are additional subclasses, which don't fit into this idea since they are in other modules: SwingNode (in javafx.swing), MediaView (in javafx.media), Printable (in javafx.web). On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx wrote: > > I think this may be a bit unclear from this post, but you're proposing I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, you're not allowed to extend these classes (despite being public). For example Node says in its documentation: > > * An application should not extend the Node class
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v3]
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? Glavo has updated the pull request incrementally with one additional commit since the last revision: reformat - Changes: - all: https://git.openjdk.org/jfx/pull/1012/files - new: https://git.openjdk.org/jfx/pull/1012/files/33e22df6..4442eb09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=01-02 Stats: 14 lines in 1 file changed: 2 ins; 6 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/1012.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1012/head:pull/1012 PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v2]
On Wed, 1 Feb 2023 23:38:57 GMT, Glavo wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Glavo has updated the pull request incrementally with two additional commits > since the last revision: > > - Update FileChooser > - Update getTracks You can put the argument list of the constructors and methods in one line. Also the `IllegalArgumentException` and its message can be in the same line. After you add the empty lines after the `FileChooser` class declaration and the `ExtensionFilter` class declaration this can go in. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of [v2]
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? Glavo has updated the pull request incrementally with two additional commits since the last revision: - Update FileChooser - Update getTracks - Changes: - all: https://git.openjdk.org/jfx/pull/1012/files - new: https://git.openjdk.org/jfx/pull/1012/files/b7482f9a..33e22df6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=00-01 Stats: 33 lines in 2 files changed: 1 ins; 23 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1012.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1012/head:pull/1012 PR: https://git.openjdk.org/jfx/pull/1012
Re: Style themes API
> > 1) Ease of migration should be an important consideration, but there > are also other considerations. I'd argue that getting the feature > stacking and mental model right is more important, and giving themes a > way to individually select if the stylesheets are user-agent or user > stylesheets makes it hard to understand why my locally-set property > value is being ignored by JavaFX in one theme, but not in the other. 2) The scenario you describe occurs when an application sets the value > of styleable properties from code, but the values are overridden by an > author stylesheet. Yes, that might happen, and I think it's up to the > application developers to fix that. The reason why I think we should > only support `Application.userAgentStyleTheme` in the first version of > this new feature is quite simple: I'd rather have the feature actually > make it into JavaFX than be stuck in review because it's too large and > too complex of a change. User-agent style themes are pretty easy to > implement (most of the code is already in place), while author style > themes are not. We can add author style themes as a follow-up > enhancement. Author stylesheets are an inherent feature of JavaFX. It's already a feature that exists so I think developers shouldn't be surprised if author stylesheets override their styles set in code. But anyways, you've convinced me on making this just be a user agent stylesheet for now. Like you say, that way this feature doesn't get too large and we're able to ship it quicker. :-) I'll point you to this document, which describes the latest version of > this feature: > https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548 > What do you think about the interaction between the various parts of > the new API, especially in regards to the appearance of native > platform decorations? > I'm not convinced that it is a good idea to make either `StyleTheme` > or `Stage.appearance` more complicated by automatically inferring the > value of one from the other. The fact that a theme is either dark or light is strongly correlated to whether you as a developer want all window decorations to be light or dark. I'd say that very few use cases will want to have some windows to be light and others to be dark. As such, this API as it is, will force developers to set the window decorations to either light or dark every time they spawn a new Window (spawning a separate Window, a Dialog, etc). I'd say it would be a good idea to set the window decorations based on what is defined in the StyleTheme with the possibility to override them if you set it on the Window itself. Looking at the doc you created. Some comments: - I like the Preferences API - "Appearance" or "ThemeAppearance"? "Appearance" sounds like a rather generic term that could mean anything.. - perhaps rename the Stage appearance property to something like: "frameDecorations", e.g. "setFrameDecorations(Appearance)"? - " A future enhancement may refactor the built-in themes to make color definitions swappable". This is already possible. Modena has its color definitions given by css variables which can be easily overridden in CSS. That's also a strong part of how I currently switch JMetro to either dark or light version without having to duplicate every style definition. Side note: JMetro is composed of several stylesheets (more than 1) so it would be a nightmare or impossible to make JMetro be a user agent stylesheet with the current state of the JavaFX API (+1 for this new API :-) ). - Perhaps rename "Application.userAgentStylesheetXX" to "Application.styleThemeXX" and keep the semantics? If we make it be "userAgentStylesheetXX" it means we can no longer in a future version make StyleTheme have a property to define whether they are to be user agent stylesheets or author stylesheets. It gives less room for us to be able to change our minds in the future. - Perhaps instead of the section "Goals" being named "Goals" we could name it "Advantages of this API" or something along those lines and have it be the first topic. I think the first question people will ask is: "what's the advantage of this new feature?".. so I think it would be nice if it's the first topic so that we grab people's attention from the start. - instead of "Promote CSS user-agent themes from an implementation detail to a first-class concept.", perhaps something more clear? Something like: "Allow easier creation of themes that are composed of user agent stylesheets (like Modena and Caspian). These themes may be composed of more than 1 user agent stylesheet which isn't possible with the current javafx API". Also perhaps make reference to the fact that 90% (rough estimate) of themes out there are composed of author style sheets and it's difficult for them to be different, which can be problematic for users of those themes: styles set in code will be overridden, styles set in FXML, styles set in author stylesheets created by the developers if the specificity of those
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 16:54:30 GMT, John Hendrikx wrote:
>> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line
>> 103:
>>
>>> 101: }
>>> 102: }
>>> 103: return returnValue;
>>
>> This method can be reduced to
>>
>> public List getTracks() {
>> synchronized (tracks) {
>> return tracks.isEmpty() ? null : List.copyOf(tracks);
>> }
>> }
>>
>> though I find it highly questionable that it returns `null` for an empty
>> list instead of just an empty list. There are 2 use cases of this method and
>> both would do better with just an empty list.
>
> Yeah, I noticed this as well right away, it is documented to do this though.
> The documentation however does seem to suggest it might be possible that
> there are three results:
>
> 1. Tracks haven't been scanned yet -> `null`
> 2. Tracks have been scanned, but none where found -> empty list
> 3. Tracks have been scanned and some were found -> list
>
> Whether case 2 can ever happen is unclear, but distinguishing it from the
> case where nothing has been scanned yet with `null` does not seem
> unreasonable.
It's an internal class and no calling class makes this distinction. I don't
think it's meant to function in the way you described.
-
PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 15:11:57 GMT, Glavo wrote: > I have considered this, but I didn't make this change because I was worried > that there would be less descriptive information when null was encountered. I think it's fine. The method is documented to throw and it happens immediately on entry. NPEs don't always have a message since they are straightforward. - PR: https://git.openjdk.org/jfx/pull/1012
Re: [External] : Re: Some classes could be sealed
For Material and LightBase it's because they are just facades whose implementation is in native code. You can't extend these without tampering with internals. I think that Camera and Shape3D also requires modifying internal stuff, though not at the native level. On Thu, Feb 2, 2023 at 12:38 AM John Hendrikx wrote: > I'm curious to know why these classes are not allowed to be subclassed > directly, as that may be important in order to decide whether these classes > should really be sealed. > > --John > On 01/02/2023 20:37, Kevin Rushforth wrote: > > I read the spec for sealed classes more carefully, and it turns out we > can't make Node sealed. At least not without API changes to SwingNode and > MediaView (and implementation changes to Printable in the javafx.web > module). All of the classes listed in the "permits" clause must be in the > same module, and SwingNode (javafx.swing) and MediaView (javafx.media) > extend Node directly they would need to be "permitted" subtypes, but there > is no way to specify that. We would also need to do something about the > tests that extend Node and run in the unnamed module. So this doesn't seem > feasible. > > We could still seal Shape, Shape3D, LightBase, and Material, since all > permitted implementation are in the javafx.graphics module. It may or may > not be worth doing that. > > -- Kevin > > > On 2/1/2023 9:45 AM, Nir Lisker wrote: > > I'll add that internal classes, mostly NG___ peers, can also benefit from > sealing. NGLightBase is an example. > > Material is another public class that can be sealed. > > On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth > wrote: > >> I agree that we should only seal existing classes that could not have >> been extended by application classes. The ones I listed in my previous >> email fit that bill, since an attempt to subclass them will throw an >> exception when it is used in a scene graph. Each documents that subclassing >> is disallowed. >> >> Btw, we've already started making use of pattern-matching instanceof in >> the implementation anyway. It would be the first API change that relies on >> a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. >> >> -- Kevin >> >> >> On 2/1/2023 9:06 AM, Philip Race wrote: >> >> In the JDK we've only sealed existing classes which provably could not >> have been extended by application classes, >> so I'm not sure about this .. >> >> also I think that might be the first change that absolutely means FX 21 >> can only be built with JDK 17 and later .. >> >> -phil >> >> On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: >> >> Yes, sorry, I made the email title in plural, but I meant what Michael >> said, Node would be sealed permitting only what is needed for JavaFx >> internally. >> >> >> -- Thiago >> >> >> Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß < >> michaelstr...@gmail.com> escreveu: >> >>> I don't think that's what Thiago is proposing. Only `Node` would be >>> sealed. >>> The following subclasses would be non-sealed: Parent, SubScene, >>> Camera, LightBase, Shape, Shape3D, Canvas, ImageView. >>> And then there are additional subclasses, which don't fit into this >>> idea since they are in other modules: SwingNode (in javafx.swing), >>> MediaView (in javafx.media), Printable (in javafx.web). >>> >>> >>> >>> On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx >>> wrote: >>> > >>> > I think this may be a bit unclear from this post, but you're proposing >>> I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, >>> you're not allowed to extend these classes (despite being public). For >>> example Node says in its documentation: >>> > >>> >* An application should not extend the Node class directly. Doing >>> so may lead to >>> >* an UnsupportedOperationException being thrown. >>> > >>> > Currently this is enforced at runtime in NodeHelper. >>> > >>> > --John >>> > >>> > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: >>> > >>> > Hi, >>> > >>> > NodeHelper.java has this: >>> > >>> > throw new UnsupportedOperationException( >>> > "Applications should not extend the " >>> > + nodeType + " class directly."); >>> > >>> > >>> > I think it's replaceable with selead classes. Am I right? >>> > >>> > The benefit will be compile time error instead of runtime. >>> > >>> > >>> > -- Thiago. >>> > >>> >> >> >> >
Re: [External] : Re: Some classes could be sealed
I'm curious to know why these classes are not allowed to be subclassed directly, as that may be important in order to decide whether these classes should really be sealed. --John On 01/02/2023 20:37, Kevin Rushforth wrote: I read the spec for sealed classes more carefully, and it turns out we can't make Node sealed. At least not without API changes to SwingNode and MediaView (and implementation changes to Printable in the javafx.web module). All of the classes listed in the "permits" clause must be in the same module, and SwingNode (javafx.swing) and MediaView (javafx.media) extend Node directly they would need to be "permitted" subtypes, but there is no way to specify that. We would also need to do something about the tests that extend Node and run in the unnamed module. So this doesn't seem feasible. We could still seal Shape, Shape3D, LightBase, and Material, since all permitted implementation are in the javafx.graphics module. It may or may not be worth doing that. -- Kevin On 2/1/2023 9:45 AM, Nir Lisker wrote: I'll add that internal classes, mostly NG___ peers, can also benefit from sealing. NGLightBase is an example. Material is another public class that can be sealed. On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth wrote: I agree that we should only seal existing classes that could not have been extended by application classes. The ones I listed in my previous email fit that bill, since an attempt to subclass them will throw an exception when it is used in a scene graph. Each documents that subclassing is disallowed. Btw, we've already started making use of pattern-matching instanceof in the implementation anyway. It would be the first API change that relies on a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. -- Kevin On 2/1/2023 9:06 AM, Philip Race wrote: In the JDK we've only sealed existing classes which provably could not have been extended by application classes, so I'm not sure about this .. also I think that might be the first change that absolutely means FX 21 can only be built with JDK 17 and later .. -phil On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: Yes, sorry, I made the email title in plural, but I meant what Michael said, Node would be sealed permitting only what is needed for JavaFx internally. -- Thiago Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß escreveu: I don't think that's what Thiago is proposing. Only `Node` would be sealed. The following subclasses would be non-sealed: Parent, SubScene, Camera, LightBase, Shape, Shape3D, Canvas, ImageView. And then there are additional subclasses, which don't fit into this idea since they are in other modules: SwingNode (in javafx.swing), MediaView (in javafx.media), Printable (in javafx.web). On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx wrote: > > I think this may be a bit unclear from this post, but you're proposing I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, you're not allowed to extend these classes (despite being public). For example Node says in its documentation: > > * An application should not extend the Node class directly. Doing so may lead to > * an UnsupportedOperationException being thrown. > > Currently this is enforced at runtime in NodeHelper. > > --John > > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: > > Hi, > > NodeHelper.java has this: > > throw new UnsupportedOperationException( > "Applications should not extend the " > + nodeType + " class directly."); > > > I think it's replaceable with selead classes. Am I right? > > The benefit will be compile time error instead of runtime. > > > -- Thiago. >
Re: [External] : Re: Some classes could be sealed
I held on proposing sealed class changes because they will be more beneficial once switch patterns are introduced. I also held on refactoring to records (of which I have a branch in my fork) for the reason that record patterns are not out yet. I think that once we have more pieces of the algebraic data types puzzle it will be very beneficial to do a decent amount of refactoring. There's nothing stopping us from starting with what we have for the purpose of upgrading errors from runtime to compile time, but we will need a second iteration anyway in some of these cases. On Wed, Feb 1, 2023 at 9:37 PM Kevin Rushforth wrote: > I read the spec for sealed classes more carefully, and it turns out we > can't make Node sealed. At least not without API changes to SwingNode and > MediaView (and implementation changes to Printable in the javafx.web > module). All of the classes listed in the "permits" clause must be in the > same module, and SwingNode (javafx.swing) and MediaView (javafx.media) > extend Node directly they would need to be "permitted" subtypes, but there > is no way to specify that. We would also need to do something about the > tests that extend Node and run in the unnamed module. So this doesn't seem > feasible. > > We could still seal Shape, Shape3D, LightBase, and Material, since all > permitted implementation are in the javafx.graphics module. It may or may > not be worth doing that. > > -- Kevin > > > On 2/1/2023 9:45 AM, Nir Lisker wrote: > > I'll add that internal classes, mostly NG___ peers, can also benefit from > sealing. NGLightBase is an example. > > Material is another public class that can be sealed. > > On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth > wrote: > >> I agree that we should only seal existing classes that could not have >> been extended by application classes. The ones I listed in my previous >> email fit that bill, since an attempt to subclass them will throw an >> exception when it is used in a scene graph. Each documents that subclassing >> is disallowed. >> >> Btw, we've already started making use of pattern-matching instanceof in >> the implementation anyway. It would be the first API change that relies on >> a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. >> >> -- Kevin >> >> >> On 2/1/2023 9:06 AM, Philip Race wrote: >> >> In the JDK we've only sealed existing classes which provably could not >> have been extended by application classes, >> so I'm not sure about this .. >> >> also I think that might be the first change that absolutely means FX 21 >> can only be built with JDK 17 and later .. >> >> -phil >> >> On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: >> >> Yes, sorry, I made the email title in plural, but I meant what Michael >> said, Node would be sealed permitting only what is needed for JavaFx >> internally. >> >> >> -- Thiago >> >> >> Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß < >> michaelstr...@gmail.com> escreveu: >> >>> I don't think that's what Thiago is proposing. Only `Node` would be >>> sealed. >>> The following subclasses would be non-sealed: Parent, SubScene, >>> Camera, LightBase, Shape, Shape3D, Canvas, ImageView. >>> And then there are additional subclasses, which don't fit into this >>> idea since they are in other modules: SwingNode (in javafx.swing), >>> MediaView (in javafx.media), Printable (in javafx.web). >>> >>> >>> >>> On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx >>> wrote: >>> > >>> > I think this may be a bit unclear from this post, but you're proposing >>> I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, >>> you're not allowed to extend these classes (despite being public). For >>> example Node says in its documentation: >>> > >>> >* An application should not extend the Node class directly. Doing >>> so may lead to >>> >* an UnsupportedOperationException being thrown. >>> > >>> > Currently this is enforced at runtime in NodeHelper. >>> > >>> > --John >>> > >>> > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: >>> > >>> > Hi, >>> > >>> > NodeHelper.java has this: >>> > >>> > throw new UnsupportedOperationException( >>> > "Applications should not extend the " >>> > + nodeType + " class directly."); >>> > >>> > >>> > I think it's replaceable with selead classes. Am I right? >>> > >>> > The benefit will be compile time error instead of runtime. >>> > >>> > >>> > -- Thiago. >>> > >>> >> >> >> >
Re: [External] : Re: Some classes could be sealed
I read the spec for sealed classes more carefully, and it turns out we can't make Node sealed. At least not without API changes to SwingNode and MediaView (and implementation changes to Printable in the javafx.web module). All of the classes listed in the "permits" clause must be in the same module, and SwingNode (javafx.swing) and MediaView (javafx.media) extend Node directly they would need to be "permitted" subtypes, but there is no way to specify that. We would also need to do something about the tests that extend Node and run in the unnamed module. So this doesn't seem feasible. We could still seal Shape, Shape3D, LightBase, and Material, since all permitted implementation are in the javafx.graphics module. It may or may not be worth doing that. -- Kevin On 2/1/2023 9:45 AM, Nir Lisker wrote: I'll add that internal classes, mostly NG___ peers, can also benefit from sealing. NGLightBase is an example. Material is another public class that can be sealed. On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth wrote: I agree that we should only seal existing classes that could not have been extended by application classes. The ones I listed in my previous email fit that bill, since an attempt to subclass them will throw an exception when it is used in a scene graph. Each documents that subclassing is disallowed. Btw, we've already started making use of pattern-matching instanceof in the implementation anyway. It would be the first API change that relies on a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. -- Kevin On 2/1/2023 9:06 AM, Philip Race wrote: In the JDK we've only sealed existing classes which provably could not have been extended by application classes, so I'm not sure about this .. also I think that might be the first change that absolutely means FX 21 can only be built with JDK 17 and later .. -phil On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: Yes, sorry, I made the email title in plural, but I meant what Michael said, Node would be sealed permitting only what is needed for JavaFx internally. -- Thiago Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß escreveu: I don't think that's what Thiago is proposing. Only `Node` would be sealed. The following subclasses would be non-sealed: Parent, SubScene, Camera, LightBase, Shape, Shape3D, Canvas, ImageView. And then there are additional subclasses, which don't fit into this idea since they are in other modules: SwingNode (in javafx.swing), MediaView (in javafx.media), Printable (in javafx.web). On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx wrote: > > I think this may be a bit unclear from this post, but you're proposing I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, you're not allowed to extend these classes (despite being public). For example Node says in its documentation: > > * An application should not extend the Node class directly. Doing so may lead to > * an UnsupportedOperationException being thrown. > > Currently this is enforced at runtime in NodeHelper. > > --John > > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: > > Hi, > > NodeHelper.java has this: > > throw new UnsupportedOperationException( > "Applications should not extend the " > + nodeType + " class directly."); > > > I think it's replaceable with selead classes. Am I right? > > The benefit will be compile time error instead of runtime. > > > -- Thiago. >
Re: RFR: 8088998: LineChart: duplicate child added exception when remove & add a series
On Mon, 30 Jan 2023 12:21:38 GMT, Karthik P K wrote: > While checking for duplicate series addition to the line chart, `setToRemove` > value was not considered before throwing exception. Hence code to handling > the case of adding the removed series was never run. > > Added condition to check `setToRemove` boolean value before throwing > exception. Made changes to call `setChart` method after calling > `seriesBeingRemovedIsAdded`. Otherwise chart will not be drawn for the > series, only points will be plotted. > > This issue is reproducible only when animation is enabled because timeline > will be still running when removed series is added back to the same chart. > Since timeline does not run in unit tests, added system test to validate the > fix. @karthikpandelu are you going to fix the remaining charts as a part of https://bugs.openjdk.org/browse/JDK-8301445 or continue with this PR? - PR: https://git.openjdk.org/jfx/pull/1015
Re: RFR: 8300705: Update boot JDK to 19.0.2
On Wed, 1 Feb 2023 17:39:55 GMT, Ambarish Rapte wrote: > Updating boot JDK to 19.0.2. Looks good. Since it is just updating to a new update in the JDK 19 family (from 19.0.1 to 19.0.2), a single reviewer should be sufficient. You might wait until tomorrow (as long as it is before RDP2) to integrate it in case @johanvos has any concerns. Actually, I just noticed that this is targeted to `master`. It should be targeted to `jfx20`. And since it is based off of master, you will need to rebase on top of `jfx20` and force-push. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.org/jfx/pull/1019Changes requested by kcr (Lead).
Re: CFV: New OpenJFX Committer: Jay Bhaskar
Vote: yes -phil.
Re: CFV: New OpenJFX Committer: Hima Bindu Meda
Vote: yes -phil.
RFR: 8300705: Update boot JDK to 19.0.2
Updating boot JDK to 19.0.2. - Commit messages: - boot jdk 19.0.2b7 Changes: https://git.openjdk.org/jfx/pull/1019/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1019&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300705 Stats: 11 lines in 2 files changed: 0 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jfx/pull/1019.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1019/head:pull/1019 PR: https://git.openjdk.org/jfx/pull/1019
Re: Some classes could be sealed
I'll add that internal classes, mostly NG___ peers, can also benefit from sealing. NGLightBase is an example. Material is another public class that can be sealed. On Wed, Feb 1, 2023 at 7:37 PM Kevin Rushforth wrote: > I agree that we should only seal existing classes that could not have been > extended by application classes. The ones I listed in my previous email fit > that bill, since an attempt to subclass them will throw an exception when > it is used in a scene graph. Each documents that subclassing is disallowed. > > Btw, we've already started making use of pattern-matching instanceof in > the implementation anyway. It would be the first API change that relies on > a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. > > -- Kevin > > > On 2/1/2023 9:06 AM, Philip Race wrote: > > In the JDK we've only sealed existing classes which provably could not > have been extended by application classes, > so I'm not sure about this .. > > also I think that might be the first change that absolutely means FX 21 > can only be built with JDK 17 and later .. > > -phil > > On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: > > Yes, sorry, I made the email title in plural, but I meant what Michael > said, Node would be sealed permitting only what is needed for JavaFx > internally. > > > -- Thiago > > > Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß < > michaelstr...@gmail.com> escreveu: > >> I don't think that's what Thiago is proposing. Only `Node` would be >> sealed. >> The following subclasses would be non-sealed: Parent, SubScene, >> Camera, LightBase, Shape, Shape3D, Canvas, ImageView. >> And then there are additional subclasses, which don't fit into this >> idea since they are in other modules: SwingNode (in javafx.swing), >> MediaView (in javafx.media), Printable (in javafx.web). >> >> >> >> On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx >> wrote: >> > >> > I think this may be a bit unclear from this post, but you're proposing >> I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, >> you're not allowed to extend these classes (despite being public). For >> example Node says in its documentation: >> > >> >* An application should not extend the Node class directly. Doing so >> may lead to >> >* an UnsupportedOperationException being thrown. >> > >> > Currently this is enforced at runtime in NodeHelper. >> > >> > --John >> > >> > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: >> > >> > Hi, >> > >> > NodeHelper.java has this: >> > >> > throw new UnsupportedOperationException( >> > "Applications should not extend the " >> > + nodeType + " class directly."); >> > >> > >> > I think it's replaceable with selead classes. Am I right? >> > >> > The benefit will be compile time error instead of runtime. >> > >> > >> > -- Thiago. >> > >> > > >
Re: Some classes could be sealed
I agree that we should only seal existing classes that could not have been extended by application classes. The ones I listed in my previous email fit that bill, since an attempt to subclass them will throw an exception when it is used in a scene graph. Each documents that subclassing is disallowed. Btw, we've already started making use of pattern-matching instanceof in the implementation anyway. It would be the first API change that relies on a JDK 17 feature, but for JavaFX 21, I see no problem in doing that. -- Kevin On 2/1/2023 9:06 AM, Philip Race wrote: In the JDK we've only sealed existing classes which provably could not have been extended by application classes, so I'm not sure about this .. also I think that might be the first change that absolutely means FX 21 can only be built with JDK 17 and later .. -phil On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: Yes, sorry, I made the email title in plural, but I meant what Michael said, Node would be sealed permitting only what is needed for JavaFx internally. -- Thiago Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß escreveu: I don't think that's what Thiago is proposing. Only `Node` would be sealed. The following subclasses would be non-sealed: Parent, SubScene, Camera, LightBase, Shape, Shape3D, Canvas, ImageView. And then there are additional subclasses, which don't fit into this idea since they are in other modules: SwingNode (in javafx.swing), MediaView (in javafx.media), Printable (in javafx.web). On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx wrote: > > I think this may be a bit unclear from this post, but you're proposing I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, you're not allowed to extend these classes (despite being public). For example Node says in its documentation: > > * An application should not extend the Node class directly. Doing so may lead to > * an UnsupportedOperationException being thrown. > > Currently this is enforced at runtime in NodeHelper. > > --John > > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: > > Hi, > > NodeHelper.java has this: > > throw new UnsupportedOperationException( > "Applications should not extend the " > + nodeType + " class directly."); > > > I think it's replaceable with selead classes. Am I right? > > The benefit will be compile time error instead of runtime. > > > -- Thiago. >
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]
On Fri, 27 Jan 2023 14:51:59 GMT, John Hendrikx wrote: >> Florian Kirmaier has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains eight commits: >> >> - JDK-8269907 >>Added missing changes after merge >> - Merge remote-tracking branch 'origjfx/master' into >> JDK-8269907-dirty-and-removed >> >># Conflicts: >># modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java >># modules/javafx.graphics/src/main/java/javafx/scene/Scene.java >> - Merge remote-tracking branch 'origin/master' >> - JDK-8269907 >>Removed the sync methods for the scene, because they don't work when peer >> is null, and they are not necessary. >> - JDK-8269907 >>Fixed rare bug, causing bounds to be out of sync. >> - JDK-8269907 >>We now require the rendering lock when cleaning up dirty nodes. To do so, >> we moved some code required for snapshot into a reusable method. >> - JDK-8269907 >>The bug is now fixed in a new way. Toolkit now supports registering >> CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks. >> - JDK-8269907 >>Fixing dirty nodes and parent removed, when a window is no longer >> showing. This typically happens with context menus. > > I took another good look at this solution, and I would like to offer an > alternative as I think this solution is more dealing with symptoms of the > underlying problem. > > I think the underlying problem is: > > 1) `Parent` is tracking a `removed` list, which is a list of `Node`. However, > it only requires the peer for the dirty area calculation. This is a classic > case of not being specific enough with the information you need. If `Parent` > is modified to store `NGNode` in its `removed` list, then the problem of > `removed` keeping references to old nodes disappears as `NGNode` does not > refer back to `Node`. Note: there is a test method currently in `Parent` > that returns the list of removed nodes -- these tests would need to be > modified to work with a list of `NGNode` or some other way should be found to > check these cases. > > 2) `Scene` keeps tracking dirty nodes even when it is not visible. The list > of dirty nodes could (before this fix) become as big as you want as it was > never cleared as the pulse listener that synchronizes the nodes never runs > when `peer == null` -- keep adding and removing new nodes while the scene is > not shown, and the list of dirty nodes grows indefinitely. This only happens > if the scene has been shown at least once before, as before that time special > code kicks in that tries to avoid keeping track of all scene nodes in the > dirty list. > > I think the better solution here would be to reset the scene to go back to > its initial state (before it was shown) and sync all nodes when it becomes > visible again. This can be achieved by setting the dirty node list to `null` > to trigger the logic that normally only triggers on the first show. > `addToDirtyList` should simply do nothing when `peer == null` . > > I believe the `syncPeer` call is smart enough not to update the peer in the > situation where a scene is hidden and then reshown, even if `Scene` calls it > again on all its nodes (`syncPeer` in `Node` will check > `dirtyBits.isEmpty()`). > > I'd also highly recommend using `ArrayList` instead of a manual `Node[] > dirtyNodes` in `Scene`. > @hjohn > > ``` > The Parent is tracking a removed list, which is a list of Node. However, it > only requires the peer for the dirty area calculation. This is a classic case > of not being specific enough with the information you need. If Parent is > modified to store NGNode in its removed list, then the problem of removed > keeping references to old nodes disappears as NGNode does not refer back to > Node. Note: there is a test method currently in Parent that returns the list > of removed nodes -- these tests would need to be modified to work with a list > of NGNode or some other way should be found to check these cases. > ``` > > This would just end in an leak of the NGNode, instead of the Node? There are > also some leaks related to NGNode, bug guess I'll have to fix the more > obvious bugs first. Also, NGNode sometimes refers to the Node. There are a > lot of issues in this area. Why not fix the root causes? This PR introduces several new things to fix a bug in a round about way that would not be needed at all if the root causes are fixed. I'm happy to help out here, as I prefer fixing the underlying problems (which probably solves multiple problems at once) rather than having to deal with each symptom and making things even more inscrutable in places where these symptoms appear. If you know the root causes, then please explain them so they can be dealt with. I've only taken a short look, and I agree that `NGNode` probably would be leaked instead. However, the end goal is to track how big the dirty are
Re: Some classes could be sealed
Yes, now that JDK 17 is the minimum we can consider doing this. As you mentioned, it would provide earlier notification of the error: compile-time versus runtime. One thing to add is that in addition to sealing Node, we would leave at least Shape, Shape3D, Camera, and LightBase sealed, since they are also not extensible and throw a similar runtime exception. -- Kevin On 2/1/2023 8:59 AM, Thiago Milczarek Sayão wrote: Yes, sorry, I made the email title in plural, but I meant what Michael said, Node would be sealed permitting only what is needed for JavaFx internally. -- Thiago Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß escreveu: I don't think that's what Thiago is proposing. Only `Node` would be sealed. The following subclasses would be non-sealed: Parent, SubScene, Camera, LightBase, Shape, Shape3D, Canvas, ImageView. And then there are additional subclasses, which don't fit into this idea since they are in other modules: SwingNode (in javafx.swing), MediaView (in javafx.media), Printable (in javafx.web). On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx wrote: > > I think this may be a bit unclear from this post, but you're proposing I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, you're not allowed to extend these classes (despite being public). For example Node says in its documentation: > > * An application should not extend the Node class directly. Doing so may lead to > * an UnsupportedOperationException being thrown. > > Currently this is enforced at runtime in NodeHelper. > > --John > > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: > > Hi, > > NodeHelper.java has this: > > throw new UnsupportedOperationException( > "Applications should not extend the " > + nodeType + " class directly."); > > > I think it's replaceable with selead classes. Am I right? > > The benefit will be compile time error instead of runtime. > > > -- Thiago. >
Re: Some classes could be sealed
In the JDK we've only sealed existing classes which provably could not have been extended by application classes, so I'm not sure about this .. also I think that might be the first change that absolutely means FX 21 can only be built with JDK 17 and later .. -phil On 2/1/23 8:59 AM, Thiago Milczarek Sayão wrote: Yes, sorry, I made the email title in plural, but I meant what Michael said, Node would be sealed permitting only what is needed for JavaFx internally. -- Thiago Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß escreveu: I don't think that's what Thiago is proposing. Only `Node` would be sealed. The following subclasses would be non-sealed: Parent, SubScene, Camera, LightBase, Shape, Shape3D, Canvas, ImageView. And then there are additional subclasses, which don't fit into this idea since they are in other modules: SwingNode (in javafx.swing), MediaView (in javafx.media), Printable (in javafx.web). On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx wrote: > > I think this may be a bit unclear from this post, but you're proposing I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, you're not allowed to extend these classes (despite being public). For example Node says in its documentation: > > * An application should not extend the Node class directly. Doing so may lead to > * an UnsupportedOperationException being thrown. > > Currently this is enforced at runtime in NodeHelper. > > --John > > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: > > Hi, > > NodeHelper.java has this: > > throw new UnsupportedOperationException( > "Applications should not extend the " > + nodeType + " class directly."); > > > I think it's replaceable with selead classes. Am I right? > > The benefit will be compile time error instead of runtime. > > > -- Thiago. >
Re: Some classes could be sealed
Yes, sorry, I made the email title in plural, but I meant what Michael said, Node would be sealed permitting only what is needed for JavaFx internally. -- Thiago Em qua., 1 de fev. de 2023 às 13:48, Michael Strauß escreveu: > I don't think that's what Thiago is proposing. Only `Node` would be sealed. > The following subclasses would be non-sealed: Parent, SubScene, > Camera, LightBase, Shape, Shape3D, Canvas, ImageView. > And then there are additional subclasses, which don't fit into this > idea since they are in other modules: SwingNode (in javafx.swing), > MediaView (in javafx.media), Printable (in javafx.web). > > > > On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx > wrote: > > > > I think this may be a bit unclear from this post, but you're proposing I > think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, > you're not allowed to extend these classes (despite being public). For > example Node says in its documentation: > > > >* An application should not extend the Node class directly. Doing so > may lead to > >* an UnsupportedOperationException being thrown. > > > > Currently this is enforced at runtime in NodeHelper. > > > > --John > > > > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: > > > > Hi, > > > > NodeHelper.java has this: > > > > throw new UnsupportedOperationException( > > "Applications should not extend the " > > + nodeType + " class directly."); > > > > > > I think it's replaceable with selead classes. Am I right? > > > > The benefit will be compile time error instead of runtime. > > > > > > -- Thiago. > > >
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 14:36:51 GMT, Nir Lisker wrote:
>> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
>> of one or two elements.
>>
>> Because `List.of` can only store non-null elements, I have only replaced a
>> few usage.
>>
>> Can someone open an Issue on JBS for me?
>
> modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 103:
>
>> 101: }
>> 102: }
>> 103: return returnValue;
>
> This method can be reduced to
>
> public List getTracks() {
> synchronized (tracks) {
> return tracks.isEmpty() ? null : List.copyOf(tracks);
> }
> }
>
> though I find it highly questionable that it returns `null` for an empty list
> instead of just an empty list. There are 2 use cases of this method and both
> would do better with just an empty list.
Yeah, I noticed this as well right away, it is documented to do this though.
The documentation however does seem to suggest it might be possible that there
are three results:
1. Tracks haven't been scanned yet -> `null`
2. Tracks have been scanned, but none where found -> empty list
3. Tracks have been scanned and some were found -> list
Whether case 2 can ever happen is unclear, but distinguishing it from the case
where nothing has been scanned yet with `null` does not seem unreasonable.
-
PR: https://git.openjdk.org/jfx/pull/1012
Re: Some classes could be sealed
I don't think that's what Thiago is proposing. Only `Node` would be sealed. The following subclasses would be non-sealed: Parent, SubScene, Camera, LightBase, Shape, Shape3D, Canvas, ImageView. And then there are additional subclasses, which don't fit into this idea since they are in other modules: SwingNode (in javafx.swing), MediaView (in javafx.media), Printable (in javafx.web). On Wed, Feb 1, 2023 at 5:39 PM John Hendrikx wrote: > > I think this may be a bit unclear from this post, but you're proposing I > think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, > you're not allowed to extend these classes (despite being public). For > example Node says in its documentation: > >* An application should not extend the Node class directly. Doing so may > lead to >* an UnsupportedOperationException being thrown. > > Currently this is enforced at runtime in NodeHelper. > > --John > > On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: > > Hi, > > NodeHelper.java has this: > > throw new UnsupportedOperationException( > "Applications should not extend the " > + nodeType + " class directly."); > > > I think it's replaceable with selead classes. Am I right? > > The benefit will be compile time error instead of runtime. > > > -- Thiago. >
Re: Some classes could be sealed
I think this may be a bit unclear from this post, but you're proposing I think to make `Node`, `Shape` and `Shape3D` sealed. For those unaware, you're not allowed to extend these classes (despite being public). For example Node says in its documentation: * An application should not extend the Node class directly. Doing so may lead to * an UnsupportedOperationException being thrown. Currently this is enforced at runtime in NodeHelper. --John On 01/02/2023 15:47, Thiago Milczarek Sayão wrote: Hi, NodeHelper.java has this: throw new UnsupportedOperationException( "Applications should not extend the " + nodeType +" class directly."); I think it's replaceable with selead classes. Am I right? The benefit will be compile time error instead of runtime. -- Thiago.
Re: CFV: New OpenJFX Committer: Hima Bindu Meda
Vote: YES John On 2/1/23 5:45 AM, Kevin Rushforth wrote: I hereby nominate Hima Bindu Meda [1] to OpenJFX Committer.
Re: CFV: New OpenJFX Committer: Jay Bhaskar
Vote: YES John On 2/1/23 5:45 AM, Kevin Rushforth wrote: I hereby nominate Jay Bhaskar [1] to OpenJFX Committer.
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]
On Wed, 1 Feb 2023 06:37:21 GMT, Karthik P K wrote: >> In `selectIndices` method, zero length array is not considered while >> ignoring row number given as parameter. >> >> Updated the code to consider both null and zero length array in the >> condition before ignoring the row value given as parameter. >> >> Added unit test to validate the fix > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Fix first index selection issue in TreeTableView I don't think null should throw an NPE in this case. Thank you Karthik for writing unit tests. LGTM - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/1018
Re: CFV: New OpenJFX Committer: Jay Bhaskar
Vote: YES -andy From: openjfx-dev on behalf of Kevin Rushforth Date: Wednesday, February 1, 2023 at 05:45 To: openjfx-dev , Jay Bhaskar Subject: CFV: New OpenJFX Committer: Jay Bhaskar I hereby nominate Jay Bhaskar [1] to OpenJFX Committer. Jay is a member of the JavaFX team at Oracle who has contributed 11 commits [2] to OpenJFX. Votes are due by February 15, 2023 at 14:00 UTC. Only current OpenJFX Committers [3] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [4]. Nomination to a project Committer is described in [5]. Thanks. -- Kevin [1] https://openjdk.org/census#jbhaskar [2] https://github.com/openjdk/jfx/search?q=author-name%3A%22Jay+Bhaskar%22&s=author-date&type=commits https://github.com/openjdk/jfx/search?q=%22Jay+Bhaskar%22&type=commits [3] https://openjdk.org/census#openjfx [4] https://openjdk.org/bylaws#lazy-consensus [5] https://openjdk.org/projects#project-committer
Re: CFV: New OpenJFX Committer: Hima Bindu Meda
Vote: YES -andy From: openjfx-dev on behalf of Kevin Rushforth Date: Wednesday, February 1, 2023 at 05:45 To: openjfx-dev , Hima Bindu Meda Subject: CFV: New OpenJFX Committer: Hima Bindu Meda I hereby nominate Hima Bindu Meda [1] to OpenJFX Committer. Hima is a member of the JavaFX team at Oracle who has contributed 10 commits [2] to OpenJFX. Votes are due by February 15, 2023 at 14:00 UTC. Only current OpenJFX Committers [3] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [4]. Nomination to a project Committer is described in [5]. Thanks. -- Kevin [1] https://openjdk.org/census#hmeda [2] https://github.com/openjdk/jfx/search?q=author-name%3A%22Hima+Bindu+Meda%22&s=author-date&type=commits [3] https://openjdk.org/census#openjfx [4] https://openjdk.org/bylaws#lazy-consensus [5] https://openjdk.org/projects#project-committer
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote:
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
> of one or two elements.
>
> Because `List.of` can only store non-null elements, I have only replaced a
> few usage.
>
> Can someone open an Issue on JBS for me?
> ### FileChooser
> `List.of` and `List.copyOf` already check for a `null` argument and `null`
> elements. This means that `validateArgs` only needs to check the `length` of
> `extensions` and for an empty element. Since the only difference between the
> constructors is taking an array vs. taking a list, once a list is created
> from the array, the array constructor can call the list constructor.
>
> I suggest the following refactoring:
>
> ```java
> public ExtensionFilter(String description, String... extensions) {
> this(description, List.of(extensions));
> }
>
> public ExtensionFilter(String description, List extensions) {
> var extensionsList = List.copyOf(extensions);
> validateArgs(description, extensionsList);
> this.description = description;
> this.extensions = extensionsList;
> }
> ```
>
> Note that `List.copyOf` will not create another copy if it was called from
> the array constructor that already created an unmodifiable `List.of`.
>
> Now validation can be reduced to
>
> ```java
> private static void validateArgs(String description, List
> extensions) {
> Objects.requireNonNull(description, "Description must not be
> null");
>
> if (description.isEmpty()) {
> throw new IllegalArgumentException("Description must not be
> empty");
> }
>
> if (extensions.isEmpty()) {
> throw new IllegalArgumentException("At least one extension
> must be defined");
> }
>
> for (String extension : extensions) {
> if (extension.isEmpty()) {
> throw new IllegalArgumentException("Extension must not be
> empty");
> }
> }
> }
> ```
>
> Aditionally, please add empty lines after the class declarations of
> `FileChooser` and `ExtensionFilter`.
>
> Also please correct the typo in `getTracks`: "The returned `List` **is**
> unmodifiable."
I have considered this, but I didn't make this change because I was worried
that there would be less descriptive information when null was encountered.
If this is not a problem, I am very happy to be able to carry out such
refactoring.
-
PR: https://git.openjdk.org/jfx/pull/1012
Re: CFV: New OpenJFX Committer: Hima Bindu Meda
Vote: YES On Wed, Feb 1, 2023 at 2:46 PM Kevin Rushforth wrote: > I hereby nominate Hima Bindu Meda [1] to OpenJFX Committer. > > Hima is a member of the JavaFX team at Oracle who has contributed 10 > commits [2] to OpenJFX. > > Votes are due by February 15, 2023 at 14:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.org/census#hmeda > > [2] > > https://github.com/openjdk/jfx/search?q=author-name%3A%22Hima+Bindu+Meda%22&s=author-date&type=commits > > [3] https://openjdk.org/census#openjfx > > [4] https://openjdk.org/bylaws#lazy-consensus > > [5] https://openjdk.org/projects#project-committer > >
Re: CFV: New OpenJFX Committer: Jay Bhaskar
Vote: YES On Wed, Feb 1, 2023 at 2:46 PM Kevin Rushforth wrote: > I hereby nominate Jay Bhaskar [1] to OpenJFX Committer. > > Jay is a member of the JavaFX team at Oracle who has contributed 11 > commits [2] to OpenJFX. > > Votes are due by February 15, 2023 at 14:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.org/census#jbhaskar > > [2] > > https://github.com/openjdk/jfx/search?q=author-name%3A%22Jay+Bhaskar%22&s=author-date&type=commits > https://github.com/openjdk/jfx/search?q=%22Jay+Bhaskar%22&type=commits > > [3] https://openjdk.org/census#openjfx > > [4] https://openjdk.org/bylaws#lazy-consensus > > [5] https://openjdk.org/projects#project-committer >
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote:
> `List.of` is cleaner, and can slightly reduce the memory footprint for lists
> of one or two elements.
>
> Because `List.of` can only store non-null elements, I have only replaced a
> few usage.
>
> Can someone open an Issue on JBS for me?
### FileChooser
`List.of` and `List.copyOf` already check for a `null` argument and `null`
elements. This means that `validateArgs` only needs to check the `length` of
`extensions` and for an empty element. Since the only difference between the
constructors is taking an array vs. taking a list, once a list is created from
the array, the array constructor can call the list constructor.
I suggest the following refactoring:
public ExtensionFilter(String description, String... extensions) {
this(description, List.of(extensions));
}
public ExtensionFilter(String description, List extensions) {
var extensionsList = List.copyOf(extensions);
validateArgs(description, extensionsList);
this.description = description;
this.extensions = extensionsList;
}
Note that `List.copyOf` will not create another copy if it was called from the
array constructor that already created an unmodifiable `List.of`.
Now validation can be reduced to
private static void validateArgs(String description, List
extensions) {
Objects.requireNonNull(description, "Description must not be null");
if (description.isEmpty()) {
throw new IllegalArgumentException("Description must not be
empty");
}
if (extensions.isEmpty()) {
throw new IllegalArgumentException("At least one extension must
be defined");
}
for (String extension : extensions) {
if (extension.isEmpty()) {
throw new IllegalArgumentException("Extension must not be
empty");
}
}
}
Aditionally, please add empty lines after the class declarations of
`FileChooser` and `ExtensionFilter`.
Also please correct the typo in `getTracks`: "The returned List
**is** unmodifiable."
modules/javafx.graphics/src/main/java/com/sun/javafx/iio/common/ImageDescriptor.java
line 41:
> 39: this.extensions = List.of(extensions);
> 40: this.signatures = List.of(signatures);
> 41: this.mimeSubtypes = List.of(mimeSubtypes);
While this is not equivalent since changing the backing array will not change
the resulting list anymore, I would consider the old code a bug and this the
correct fix.
Note that in `FileChooser` care is taken to create a copy of the supplied array
before using it to create a list.
Additionally, care must be taken that all the callers don't have `null`
elements in the arrays they pass. I checked it and it's fine (and should
probably be disallowed, which is good now).
By the way, this class should be a `record`, and all its subtypes, which are
not really subtypes but just configured instances, should be modified
accordingly. This is out of scope though.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 100:
> 98: returnValue = null;
> 99: } else {
> 100: returnValue = List.copyOf(tracks);
This is fine because `addTrack` checks for `null` elements.
modules/javafx.media/src/main/java/com/sun/media/jfxmedia/Media.java line 103:
> 101: }
> 102: }
> 103: return returnValue;
This method can be reduced to
public List getTracks() {
synchronized (tracks) {
return tracks.isEmpty() ? null : List.copyOf(tracks);
}
}
though I find it highly questionable that it returns `null` for an empty list
instead of just an empty list. There are 2 use cases of this method and both
would do better with just an empty list.
-
Changes requested by nlisker (Reviewer).
PR: https://git.openjdk.org/jfx/pull/1012
Some classes could be sealed
Hi, NodeHelper.java has this: throw new UnsupportedOperationException( "Applications should not extend the " + nodeType + " class directly."); I think it's replaceable with selead classes. Am I right? The benefit will be compile time error instead of runtime. -- Thiago.
Re: CFV: New OpenJFX Committer: Hima Bindu Meda
Vote: YES Em qua., 1 de fev. de 2023 às 10:45, Kevin Rushforth < kevin.rushfo...@oracle.com> escreveu: > I hereby nominate Hima Bindu Meda [1] to OpenJFX Committer. > > Hima is a member of the JavaFX team at Oracle who has contributed 10 > commits [2] to OpenJFX. > > Votes are due by February 15, 2023 at 14:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.org/census#hmeda > > [2] > > https://github.com/openjdk/jfx/search?q=author-name%3A%22Hima+Bindu+Meda%22&s=author-date&type=commits > > [3] https://openjdk.org/census#openjfx > > [4] https://openjdk.org/bylaws#lazy-consensus > > [5] https://openjdk.org/projects#project-committer > >
Re: CFV: New OpenJFX Committer: Jay Bhaskar
Vote: YES Em qua., 1 de fev. de 2023 às 10:45, Kevin Rushforth < kevin.rushfo...@oracle.com> escreveu: > I hereby nominate Jay Bhaskar [1] to OpenJFX Committer. > > Jay is a member of the JavaFX team at Oracle who has contributed 11 > commits [2] to OpenJFX. > > Votes are due by February 15, 2023 at 14:00 UTC. > > Only current OpenJFX Committers [3] are eligible to vote on this > nomination. Votes must be cast in the open by replying to this mailing > list. > > For Lazy Consensus voting instructions, see [4]. Nomination to a project > Committer is described in [5]. > > Thanks. > > -- Kevin > > > [1] https://openjdk.org/census#jbhaskar > > [2] > > https://github.com/openjdk/jfx/search?q=author-name%3A%22Jay+Bhaskar%22&s=author-date&type=commits > https://github.com/openjdk/jfx/search?q=%22Jay+Bhaskar%22&type=commits > > [3] https://openjdk.org/census#openjfx > > [4] https://openjdk.org/bylaws#lazy-consensus > > [5] https://openjdk.org/projects#project-committer >
Re: CFV: New OpenJFX Committer: Hima Bindu Meda
Vote: YES -- Kevin On 2/1/2023 5:45 AM, Kevin Rushforth wrote: I hereby nominate Hima Bindu Meda [1] to OpenJFX Committer.
Re: CFV: New OpenJFX Committer: Jay Bhaskar
Vote: YES -- Kevin On 2/1/2023 5:45 AM, Kevin Rushforth wrote: I hereby nominate Jay Bhaskar [1] to OpenJFX Committer.
CFV: New OpenJFX Committer: Hima Bindu Meda
I hereby nominate Hima Bindu Meda [1] to OpenJFX Committer. Hima is a member of the JavaFX team at Oracle who has contributed 10 commits [2] to OpenJFX. Votes are due by February 15, 2023 at 14:00 UTC. Only current OpenJFX Committers [3] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [4]. Nomination to a project Committer is described in [5]. Thanks. -- Kevin [1] https://openjdk.org/census#hmeda [2] https://github.com/openjdk/jfx/search?q=author-name%3A%22Hima+Bindu+Meda%22&s=author-date&type=commits [3] https://openjdk.org/census#openjfx [4] https://openjdk.org/bylaws#lazy-consensus [5] https://openjdk.org/projects#project-committer
CFV: New OpenJFX Committer: Jay Bhaskar
I hereby nominate Jay Bhaskar [1] to OpenJFX Committer. Jay is a member of the JavaFX team at Oracle who has contributed 11 commits [2] to OpenJFX. Votes are due by February 15, 2023 at 14:00 UTC. Only current OpenJFX Committers [3] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [4]. Nomination to a project Committer is described in [5]. Thanks. -- Kevin [1] https://openjdk.org/census#jbhaskar [2] https://github.com/openjdk/jfx/search?q=author-name%3A%22Jay+Bhaskar%22&s=author-date&type=commits https://github.com/openjdk/jfx/search?q=%22Jay+Bhaskar%22&type=commits [3] https://openjdk.org/census#openjfx [4] https://openjdk.org/bylaws#lazy-consensus [5] https://openjdk.org/projects#project-committer
Integrated: JDK-8299977 : Update WebKit to 615.1
On Wed, 25 Jan 2023 19:51:02 GMT, Hima Bindu Meda wrote: > Update JavaFX WebKit to GTK WebKit 2.38 (615.1). > > Verified the updated version build, sanity tests and stability. > This does not cause any issues except one unit test failure. > Also, there are some artifacts observed while playing youtube > > The above issues are recorded and ignored using below bugs: > [JDK-8300954](https://bugs.openjdk.org/browse/JDK-8300954) > [JDK-8301022](https://bugs.openjdk.org/browse/JDK-8301022) This pull request has now been integrated. Changeset: 301bbd64 Author:Hima Bindu Meda Committer: Kevin Rushforth URL: https://git.openjdk.org/jfx/commit/301bbd6409d463c4f789e2b3dcb6b2ea200d7373 Stats: 197124 lines in 5579 files changed: 116283 ins; 42814 del; 38027 mod 8299977: Update WebKit to 615.1 Co-authored-by: Jay Bhaskar Reviewed-by: kcr, sykora - PR: https://git.openjdk.org/jfx/pull/1011
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 11:21:04 GMT, Glavo wrote: >> Filed: https://bugs.openjdk.org/browse/JDK-8301604. Please let me know if >> the issue is not reported correctly. > > @theaoqi Thank you very much! @Glavo If you can't access JBS, you can submit a report via bugreport.java.com. @theaoqi If you create a JBS issue, fill in the affected version and fix version (`tbd` is fine) fields. The subcomponent is also not only graphics because there are changes to media as well. Use `other` for overarching changes like these (e.g., https://bugs.openjdk.org/browse/JDK-8208761). I can sponsor this fix, but it will need some more changes. Just a drop-in replacement with `List.of` doesn't result in good code. I will review it shortly. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: JDK-8299977 : Update WebKit to 615.1 [v3]
On Wed, 1 Feb 2023 04:11:48 GMT, Hima Bindu Meda wrote: >> Update JavaFX WebKit to GTK WebKit 2.38 (615.1). >> >> Verified the updated version build, sanity tests and stability. >> This does not cause any issues except one unit test failure. >> Also, there are some artifacts observed while playing youtube >> >> The above issues are recorded and ignored using below bugs: >> [JDK-8300954](https://bugs.openjdk.org/browse/JDK-8300954) >> [JDK-8301022](https://bugs.openjdk.org/browse/JDK-8301022) > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Remove the log added for testing purpose Marked as reviewed by sykora (Author). - PR: https://git.openjdk.org/jfx/pull/1011
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote: > `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? For a refactoring / cleanup fix like this, the question is whether it is worth the effort to test and review it. I don't know that it is, but if you can convince a Reviewer to test and review it, and then sponsor the fix for you, I won't object. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: JDK-8299977 : Update WebKit to 615.1 [v2]
On Tue, 31 Jan 2023 18:49:21 GMT, Joeri Sykora wrote: >> Hima Bindu Meda has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add blank lines > > Building and testing succeeded on linux, mac and windows x86_64. @tiainen Can you re-review following Hima's removal of the print statement? - PR: https://git.openjdk.org/jfx/pull/1011
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]
On Wed, 1 Feb 2023 12:27:22 GMT, Nir Lisker wrote: >> I believe no code change is required as of now. Hence not making any changes >> now. > > I also think that `null` should throw unless otherwise specified. Since the null check is preexisting, I don't think we should make the change to throw NPE on null as part of a simple bug fix. If we decide to change this, I would want to see a spec change in the docs with an accompanying CSR, which should be done as a separate bug. - PR: https://git.openjdk.org/jfx/pull/1018
Re: RFR: JDK-8299977 : Update WebKit to 615.1 [v3]
On Wed, 1 Feb 2023 04:11:48 GMT, Hima Bindu Meda wrote: >> Update JavaFX WebKit to GTK WebKit 2.38 (615.1). >> >> Verified the updated version build, sanity tests and stability. >> This does not cause any issues except one unit test failure. >> Also, there are some artifacts observed while playing youtube >> >> The above issues are recorded and ignored using below bugs: >> [JDK-8300954](https://bugs.openjdk.org/browse/JDK-8300954) >> [JDK-8301022](https://bugs.openjdk.org/browse/JDK-8301022) > > Hima Bindu Meda has updated the pull request incrementally with one > additional commit since the last revision: > > Remove the log added for testing purpose Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.org/jfx/pull/1011
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]
On Wed, 1 Feb 2023 09:17:33 GMT, Karthik P K wrote: >> I think it is also pretty clear the original author intended to check >> `rows.length == 0` and made the mistake that it would be called with `rows >> == null` when there are no further indices specified, which is incorrect. > > I believe no code change is required as of now. Hence not making any changes > now. I also think that `null` should throw unless otherwise specified. - PR: https://git.openjdk.org/jfx/pull/1018
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Wed, 1 Feb 2023 11:03:32 GMT, Ao Qi wrote: >> `List.of` is cleaner, and can slightly reduce the memory footprint for lists >> of one or two elements. >> >> Because `List.of` can only store non-null elements, I have only replaced a >> few usage. >> >> Can someone open an Issue on JBS for me? > > Filed: https://bugs.openjdk.org/browse/JDK-8301604. Please let me know if the > issue is not reported correctly. @theaoqi Thank you very much! - PR: https://git.openjdk.org/jfx/pull/1012
RFR: 8301604: Replace Collections.unmodifiableList with List.of
`List.of` is cleaner, and can slightly reduce the memory footprint for lists of one or two elements. Because `List.of` can only store non-null elements, I have only replaced a few usage. Can someone open an Issue on JBS for me? - Commit messages: - cleanup Changes: https://git.openjdk.org/jfx/pull/1012/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1012&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8301604 Stats: 18 lines in 3 files changed: 0 ins; 9 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1012.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1012/head:pull/1012 PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8301604: Replace Collections.unmodifiableList with List.of
On Thu, 26 Jan 2023 05:30:56 GMT, Glavo wrote: > `List.of` is cleaner, and can slightly reduce the memory footprint for lists > of one or two elements. > > Because `List.of` can only store non-null elements, I have only replaced a > few usage. > > Can someone open an Issue on JBS for me? Filed: https://bugs.openjdk.org/browse/JDK-8301604. Please let me know if the issue is not reported correctly. - PR: https://git.openjdk.org/jfx/pull/1012
Re: RFR: 8251862: Wrong position of Popup windows at the intersection of 2 screens [v4]
On Tue, 31 Jan 2023 19:01:33 GMT, Kevin Rushforth wrote: >> On Windows platforms with more than one screen, a PopupWindow created for a >> Stage that straddles two windows will be drawn with an incorrect position >> and screen scale if the majority of the Stage is on one screen, and the >> popup is positioned on the other screen. In this case, the Stage is drawn >> using the screen scale of the screen that most of the window is on, while >> the popup is drawn using the scale of the screen that it is (typically >> entirely) on. >> >> The most common way this can happen is when you have two screens of a >> different scale with the secondary screen on the left or above the primary >> screen. If you position the Stage such that most of it is still on the >> primary screen (thus the Stage is drawn using the scale of the primary >> screen), with a menu, a control with a context menu, or a control with a >> Tooltip now on the secondary screen, the popup window for the menu or >> Tooltip will be drawn using the screen scale of the secondary window and >> thus won't be positioned or sized correctly relative to the menu bar, or >> control in the main window. >> >> The fix implemented by this PR is to always use the screen of the owner >> window, including the screen scales, when rendering a popup window. This >> matches the behavior of native Windows apps, such as Notepad. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year to 2023 I confirm that this fixes the reported issue. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/971
Re: RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v2]
On Wed, 18 Jan 2023 06:56:57 GMT, Karthik P K wrote: >> Ignore text condition was not considered in `computePrefHeight` method while >> calculating the Label height. >> >> Added `isIgnoreText` condition in `computePrefHeight` method while >> calculating Label height. >> >> Tests are present for all the ContentDisplay types. Modified tests which >> were failing because of the new height value getting calculated. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Use width while calling prefHeight method. Use graphicHeight instead of > multiple calls to prefHeight The fix looks good to me! - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/996
Re: RFR: 8088998: LineChart: duplicate child added exception when remove & add a series
On Mon, 30 Jan 2023 12:21:38 GMT, Karthik P K wrote: > While checking for duplicate series addition to the line chart, `setToRemove` > value was not considered before throwing exception. Hence code to handling > the case of adding the removed series was never run. > > Added condition to check `setToRemove` boolean value before throwing > exception. Made changes to call `setChart` method after calling > `seriesBeingRemovedIsAdded`. Otherwise chart will not be drawn for the > series, only points will be plotted. > > This issue is reproducible only when animation is enabled because timeline > will be still running when removed series is added back to the same chart. > Since timeline does not run in unit tests, added system test to validate the > fix. Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.org/jfx/pull/1015
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]
On Fri, 27 Jan 2023 14:51:59 GMT, John Hendrikx wrote:
>> Florian Kirmaier has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains eight commits:
>>
>> - JDK-8269907
>>Added missing changes after merge
>> - Merge remote-tracking branch 'origjfx/master' into
>> JDK-8269907-dirty-and-removed
>>
>># Conflicts:
>># modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>># modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>> - Merge remote-tracking branch 'origin/master'
>> - JDK-8269907
>>Removed the sync methods for the scene, because they don't work when peer
>> is null, and they are not necessary.
>> - JDK-8269907
>>Fixed rare bug, causing bounds to be out of sync.
>> - JDK-8269907
>>We now require the rendering lock when cleaning up dirty nodes. To do so,
>> we moved some code required for snapshot into a reusable method.
>> - JDK-8269907
>>The bug is now fixed in a new way. Toolkit now supports registering
>> CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.
>> - JDK-8269907
>>Fixing dirty nodes and parent removed, when a window is no longer
>> showing. This typically happens with context menus.
>
> I took another good look at this solution, and I would like to offer an
> alternative as I think this solution is more dealing with symptoms of the
> underlying problem.
>
> I think the underlying problem is:
>
> 1) `Parent` is tracking a `removed` list, which is a list of `Node`. However,
> it only requires the peer for the dirty area calculation. This is a classic
> case of not being specific enough with the information you need. If `Parent`
> is modified to store `NGNode` in its `removed` list, then the problem of
> `removed` keeping references to old nodes disappears as `NGNode` does not
> refer back to `Node`. Note: there is a test method currently in `Parent`
> that returns the list of removed nodes -- these tests would need to be
> modified to work with a list of `NGNode` or some other way should be found to
> check these cases.
>
> 2) `Scene` keeps tracking dirty nodes even when it is not visible. The list
> of dirty nodes could (before this fix) become as big as you want as it was
> never cleared as the pulse listener that synchronizes the nodes never runs
> when `peer == null` -- keep adding and removing new nodes while the scene is
> not shown, and the list of dirty nodes grows indefinitely. This only happens
> if the scene has been shown at least once before, as before that time special
> code kicks in that tries to avoid keeping track of all scene nodes in the
> dirty list.
>
> I think the better solution here would be to reset the scene to go back to
> its initial state (before it was shown) and sync all nodes when it becomes
> visible again. This can be achieved by setting the dirty node list to `null`
> to trigger the logic that normally only triggers on the first show.
> `addToDirtyList` should simply do nothing when `peer == null` .
>
> I believe the `syncPeer` call is smart enough not to update the peer in the
> situation where a scene is hidden and then reshown, even if `Scene` calls it
> again on all its nodes (`syncPeer` in `Node` will check
> `dirtyBits.isEmpty()`).
>
> I'd also highly recommend using `ArrayList` instead of a manual `Node[]
> dirtyNodes` in `Scene`.
@hjohn
The Parent is tracking a removed list, which is a list of Node. However, it
only requires the peer for the dirty area calculation. This is a classic case
of not being specific enough with the information you need. If Parent is
modified to store NGNode in its removed list, then the problem of removed
keeping references to old nodes disappears as NGNode does not refer back to
Node. Note: there is a test method currently in Parent that returns the list of
removed nodes -- these tests would need to be modified to work with a list of
NGNode or some other way should be found to check these cases.
This would just end in an leak of the NGNode, instead of the Node?
There are also some leaks related to NGNode, bug guess I'll have to fix the
more obvious bugs first.
Also, NGNode sometimes refers to the Node. There are a lot of issues in this
area.
## Resetting the Scene
This approach has one problem - it would change the complexity of
showing/hiding a window. This issue typically happened with Popups, which are
regularly shown/hidden. I guess changing the complexity to O()
might be ok - but I'm not sure about it.
I don't want to change the dirtyNodes to ArrayList in this PR, i don't want to
mix this kind of refactoring into the Bugfix-PR.
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line 492:
>
>> 490: }
>> 491:
>> 492: public void addCleanupListener(TKPulseListener listener) {
>
> This name is misleading. It adds a listener which gets auto removed when
> called (and hence w
Replace Collections.unmodifiableList with List.of
I opened a PR to clean up the code. Can someone help me open an Issue in JBS? https://github.com/openjdk/jfx/pull/1012 With best regards, Glavo
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]
On Fri, 27 Jan 2023 13:30:59 GMT, John Hendrikx wrote:
>> Florian Kirmaier has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains eight commits:
>>
>> - JDK-8269907
>>Added missing changes after merge
>> - Merge remote-tracking branch 'origjfx/master' into
>> JDK-8269907-dirty-and-removed
>>
>># Conflicts:
>># modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>># modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>> - Merge remote-tracking branch 'origin/master'
>> - JDK-8269907
>>Removed the sync methods for the scene, because they don't work when peer
>> is null, and they are not necessary.
>> - JDK-8269907
>>Fixed rare bug, causing bounds to be out of sync.
>> - JDK-8269907
>>We now require the rendering lock when cleaning up dirty nodes. To do so,
>> we moved some code required for snapshot into a reusable method.
>> - JDK-8269907
>>The bug is now fixed in a new way. Toolkit now supports registering
>> CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.
>> - JDK-8269907
>>Fixing dirty nodes and parent removed, when a window is no longer
>> showing. This typically happens with context menus.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 746:
>
>> 744: if(!cleanupAdded) {
>> 745: if((window.get() == null || !window.get().isShowing()) &&
>> dirtyNodesSize > 0) {
>> 746:
>> Toolkit.getToolkit().addCleanupListener(cleanupListener);
>
> This adds a listener to be run on the next pulse, yet no pulse is being
> requested. Probably something else will request a pulse, but seems like you
> may not want to rely on that. Specifically, `addToDirtyList` won't request a
> pulse if the stage is already closed (peer is `null`), and only then do you
> remove some nodes. It still works though in that case, probably because
> something else in JavaFX is requesting a pulse (nothing in `Scene` does
> though when peer is `null`).
I assume that these methods are just called all the time - independent of
whether there is an open window.
I would like to leave it like this - unless there is an issue and an idea on
how to do it differently.
-
PR: https://git.openjdk.org/jfx/pull/584
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]
On Thu, 26 Jan 2023 18:20:52 GMT, Michael Strauß wrote:
>> Florian Kirmaier has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains eight commits:
>>
>> - JDK-8269907
>>Added missing changes after merge
>> - Merge remote-tracking branch 'origjfx/master' into
>> JDK-8269907-dirty-and-removed
>>
>># Conflicts:
>># modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>># modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>> - Merge remote-tracking branch 'origin/master'
>> - JDK-8269907
>>Removed the sync methods for the scene, because they don't work when peer
>> is null, and they are not necessary.
>> - JDK-8269907
>>Fixed rare bug, causing bounds to be out of sync.
>> - JDK-8269907
>>We now require the rendering lock when cleaning up dirty nodes. To do so,
>> we moved some code required for snapshot into a reusable method.
>> - JDK-8269907
>>The bug is now fixed in a new way. Toolkit now supports registering
>> CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.
>> - JDK-8269907
>>Fixing dirty nodes and parent removed, when a window is no longer
>> showing. This typically happens with context menus.
>
> tests/system/src/test/java/test/javafx/scene/DirtyNodesLeakTest.java line 44:
>
>> 42: import static test.util.Util.TIMEOUT;
>> 43:
>> 44: public class DirtyNodesLeakTest {
>
> Since this tests dirty nodes of a `Scene`, maybe you could use a name like
> `Scene_dirtyNodesLeakTest`?
I like the name you've suggested. I've changed it now to your suggestion.
-
PR: https://git.openjdk.org/jfx/pull/584
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v9]
> After thinking about this issue for some time, I've now got a solution. > I know put the scene in the state it is, before is was shown, when the > dirtyNodes are unset, the whole scene is basically considered dirty. > This has the drawback of rerendering, whenever a window is "reshown", but it > restores sanity about memory behaviour, which should be considered more > important. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: JDK-8269907 renamed testclass, based on code review, readded acidentally removed import - Changes: - all: https://git.openjdk.org/jfx/pull/584/files - new: https://git.openjdk.org/jfx/pull/584/files/7e47e060..dfb34701 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=584&range=08 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=584&range=07-08 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/584.diff Fetch: git fetch https://git.openjdk.org/jfx pull/584/head:pull/584 PR: https://git.openjdk.org/jfx/pull/584
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]
On Thu, 26 Jan 2023 18:12:23 GMT, Michael Strauß wrote:
>> Florian Kirmaier has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains eight commits:
>>
>> - JDK-8269907
>>Added missing changes after merge
>> - Merge remote-tracking branch 'origjfx/master' into
>> JDK-8269907-dirty-and-removed
>>
>># Conflicts:
>># modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>># modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>> - Merge remote-tracking branch 'origin/master'
>> - JDK-8269907
>>Removed the sync methods for the scene, because they don't work when peer
>> is null, and they are not necessary.
>> - JDK-8269907
>>Fixed rare bug, causing bounds to be out of sync.
>> - JDK-8269907
>>We now require the rendering lock when cleaning up dirty nodes. To do so,
>> we moved some code required for snapshot into a reusable method.
>> - JDK-8269907
>>The bug is now fixed in a new way. Toolkit now supports registering
>> CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.
>> - JDK-8269907
>>Fixing dirty nodes and parent removed, when a window is no longer
>> showing. This typically happens with context menus.
>
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 745:
>
>> 743: private void checkCleanDirtyNodes() {
>> 744: if(!cleanupAdded) {
>> 745: if((window.get() == null || !window.get().isShowing()) &&
>> dirtyNodesSize > 0) {
>
> Minor: space after `if`.
> You could also reduce nesting by consolidating the two separate conditions.
I've merged it now to 1 if, with a space!
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2112:
>
>> 2110:
>> **/
>> 2111:
>> 2112: private void windowForSceneChanged(Window oldWindow, Window
>> window) {
>
> This change doesn't seem necessary.
I've reverted the variable name.
> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2129:
>
>> 2127: }
>> 2128:
>> 2129: private final ChangeListener sceneWindowShowingListener =
>> (p, o, n) -> {checkCleanDirtyNodes(); } ;
>
> Minor: should be no space before `;`.
> You could also remove the curly braces.
I've removed now the curly braces.
> tests/system/src/test/java/test/javafx/scene/DirtyNodesLeakTest.java line 35:
>
>> 33: import junit.framework.Assert;
>> 34: import org.junit.BeforeClass;
>> 35: import org.junit.Test;
>
> Since this is a new class, you should use the JUnit5 API.
I'm now using the junit5 api.
-
PR: https://git.openjdk.org/jfx/pull/584
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]
On Fri, 27 Jan 2023 10:51:00 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line
>> 420:
>>
>>> 418: new WeakHashMap<>();
>>> 419: final Map cleanupList =
>>> 420: new WeakHashMap<>();
>>
>> I wonder why we need `WeakHashMap` here. These maps are short-lived anyway,
>> since they're local variables.
>
> These should not be `WeakHashMap` at all; even if it is was necessary for
> some reason, there is no guarantee GC will run and potential keys that should
> have been removed may still be there in many situations. Using weak maps here
> only serves to make the process unpredictable, making it harder to find bugs
> if there ever is a situation where a key should have been removed.
>
> I would be in favor of changing these to normal maps.
I've changed it now to a normal HashMap.
>> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java line
>> 494:
>>
>>> 492: public void addCleanupListener(TKPulseListener listener) {
>>> 493: AccessControlContext acc = AccessController.getContext();
>>> 494: cleanupListeners.put(listener,acc);
>>
>> Access to `cleanupListeners` is not synchronized on `this` here, but it is
>> synchronized in L426.
>> You should either synchronize each and every access, or none of it.
>
> It should not be removed everywhere, it has to be synchronized (all the other
> listener lists are as well). This is probably because listeners can be added
> on different threads.
I've added a synchronized now.
-
PR: https://git.openjdk.org/jfx/pull/584
Re: RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v8]
> After thinking about this issue for some time, I've now got a solution. > I know put the scene in the state it is, before is was shown, when the > dirtyNodes are unset, the whole scene is basically considered dirty. > This has the drawback of rerendering, whenever a window is "reshown", but it > restores sanity about memory behaviour, which should be considered more > important. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: JDK-8269907 changes based on code review - Changes: - all: https://git.openjdk.org/jfx/pull/584/files - new: https://git.openjdk.org/jfx/pull/584/files/c5c5e080..7e47e060 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=584&range=07 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=584&range=06-07 Stats: 27 lines in 3 files changed: 3 ins; 1 del; 23 mod Patch: https://git.openjdk.org/jfx/pull/584.diff Fetch: git fetch https://git.openjdk.org/jfx pull/584/head:pull/584 PR: https://git.openjdk.org/jfx/pull/584
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0 [v2]
On Tue, 31 Jan 2023 19:41:20 GMT, John Hendrikx wrote: >> Well it's legal Java code, but that doesn't mean that leaving something >> `null` is allowed. At the very least it is undocumented behavior: >> >> /** >> * This method allows for one or more selections to be set at the >> same time. >> * It will ignore any value that is not within the valid range (i.e. >> greater >> * than or equal to zero, and less than the total number of items in the >> * underlying data model). Any duplication of indices will be ignored. >> * >> * If there is already one or more indices selected in this model, >> calling >> * this method will not clear these selections - to do so it is >> * necessary to first call clearSelection. >> * >> * The last valid value given will become the selected index / >> selected >> * item. >> * @param index the first index to select >> * @param indices zero or more additional indices to select >> */ >> public abstract void selectIndices(int index, int... indices); > > I think it is also pretty clear the original author intended to check > `rows.length == 0` and made the mistake that it would be called with `rows == > null` when there are no further indices specified, which is incorrect. I believe no code change is required as of now. Hence not making any changes now. - PR: https://git.openjdk.org/jfx/pull/1018
Re: RFR: 8138842: TableViewSelectionModel.selectIndices does not select index 0
On Tue, 31 Jan 2023 17:05:01 GMT, Nir Lisker wrote: > I recently looked at https://bugs.openjdk.org/browse/JDK-8120635. Is it > related? I managed to reproduce it, so I think it should be reopened. This is kind of similar issue but not related. This issue is because of the bug in `selectLast` method of MultipleSelectionModelBase class. I couldn't reproduce the issue with `select` and `selectFirst` methods as mentioned in the bug, I could reproduce only with `selectLast` method. - PR: https://git.openjdk.org/jfx/pull/1018
