Re: RFC: new property in ToggleGroup
On 1/20/2023 2:57 PM, Andy Goryachev wrote: I just want to add one thing - the initial state of RadioMenuItems added to a menu is unselected, even if they belong to a ToggleGroup. One can say that having all unselected radio buttons in a toggle group makes no sense, or perhaps it depends on the application requirements - though I cannot find an example where it might be needed. Yes, it's initially unselected unless the app makes an explicit selection. HTML radio buttons work the same way [1]. Some apps use that to indicate that nothing was selected, but then once you select it there will always be something selected. So either we allow two different policies by adding a property to the ToggleGroup, or we proclaim that adding a radio button to a toggle group must have a side effect of one (first) button to be automatically selected, otherwise the whole group is in inconsistent state (or the app developer must write some code to select one). I wouldn't want to change the default behavior, since I can imagine some apps relying on being able to tell if the user has ever selected any of the choices. Having two properties would be one solution, presuming we think that we need to provide a way for the app to indicate that it wants us to enforce the invariant of ensuring that the app can't ever get the control in a state where nothing is selected. Although, since it would be an opt-in, the app could just as easily set the default itself as opposed to setting this new property. -- Kevin [1] https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_input_type_radio -andy *From: *openjfx-dev on behalf of Kevin Rushforth *Date: *Friday, 2023/01/20 at 12:27 *To: *openjfx-dev@openjdk.org *Subject: *Re: RFC: new property in ToggleGroup How common a UI feature is being able to deselect the selected item in a ToggleGroup via the UI such that no item is selected? I don't normally see that in various apps or toolkits that I am familiar with. What I do see is that either a default item is selected or no item is selected initially (which is the one and only time that there will be no item selected), but in both case, once you make a selection, there is no way via the UI to deselect the current item. Absent a compelling need, I think the current behavior (once the fix for JDK-8237505 is integrated) is sufficient. What do other developers think? -- Kevin On 1/20/2023 11:31 AM, Andy Goryachev wrote: Dear colleagues: In the context of a recent PR https://bugs.openjdk.org/browse/JDK-8237505 https://github.com/openjdk/jfx/pull/1002 https://stackoverflow.com/questions/57911107/javafx-togglegroup-not-functioning-properly-with-accelerators-radiomenuitem where a number of RadioMenuItems belonging to a toggle group are added to the menu, we might want to add a new property to the ToggleGroup which controls whether all items in a group can be deselected. If this property is set, a selected radio menu item can be deselected via either keyboard accelerator or a mouse click. If not, then not only this operation cannot be performed, but also the first item added to said ToggleGroup gets automatically selected. This should allow for more flexibility in creating menus with RadioMenuItems, but eliminate some boilerplate code required in such cases. The new logic would also affect any Toggle, such as ToggleButton. What do you think? Thank you. -andy
Re: Small changes to Gradle build scripts
It does look cleaner, and I guess every little bit helps. If you want to
file a cleanup enhancement, we can take a look, but it won't be a very
high priority.
-- Kevin
On 1/20/2023 6:51 AM, Scott Palmer wrote:
Are small simplifying changes to the gradle scripts welcome?
E.g. instead of:
def inStream = new java.io.BufferedReader(new
java.io.InputStreamReader(new java.lang.ProcessBuilder(JAVA,
"-fullversion").start().getErrorStream()));
try {
String v = inStream.readLine().trim();
if (v != null) {
int ib = v.indexOf("full version \"");
if (ib != -1) {
String str = v.substring(ib);
String ver = str.substring(str.indexOf("\"") + 1, str.size() - 1);
defineProperty("jdkRuntimeVersion", ver)
def jdkVersionInfo = parseJavaVersion(ver)
defineProperty("jdkVersion", jdkVersionInfo[0])
defineProperty("jdkBuildNumber", jdkVersionInfo[1])
// Define global properties based on the version of Java
// For example, we could define a "jdk18OrLater" property as
// follows that could then be used to implement conditional build
// logic based on whether we were running on JDK 18 or later,
// should the need arise.
// def status = compareJdkVersion(jdkVersion, "18")
// ext.jdk18OrLater = (status >= 0)
}
}
} finally {
inStream.close();
}
this:
def verMatch = [JAVA, "-fullversion"].execute().err.text =~ /full
version "([^"]+)/
if (verMatch.find()) {
String ver = verMatch.group(1);
defineProperty("jdkRuntimeVersion", ver)
def jdkVersionInfo = parseJavaVersion(ver)
defineProperty("jdkVersion", jdkVersionInfo[0])
defineProperty("jdkBuildNumber", jdkVersionInfo[1])
// Define global properties based on the version of Java
// For example, we could define a "jdk18OrLater" property as
// follows that could then be used to implement conditional build
// logic based on whether we were running on JDK 18 or later,
// should the need arise.
// def status = compareJdkVersion(jdkVersion, "18")
// ext.jdk18OrLater = (status >= 0)
}
or instead of:
ext.HAS_JAVAFX_MODULES = false;
def inStream2 = new java.io.BufferedReader(new
java.io.InputStreamReader(new java.lang.ProcessBuilder(JAVA,
"--list-modules").start().getInputStream()));
try {
String v;
while ((v = inStream2.readLine()) != null) {
v = v.trim();
if (v.startsWith("javafx.base")) ext.HAS_JAVAFX_MODULES = true;
}
} finally {
inStream2.close();
}
this:
ext.HAS_JAVAFX_MODULES = [JAVA,
'--list-modules'].execute().text.contains('javafx.base')
... much simpler and seems to work just as well.
They don't do much more than improve readability and reduce the line
count though, so I'm not sure if anyone cares.
Scott
Re: RFR: 8275033: Drag and drop a file produces NullPointerException Cannot read field "dragboard"
On Fri, 20 Jan 2023 20:35:07 GMT, Andy Goryachev wrote:
>> When running on Wayland the `GDK_DRAG_LEAVE` is sent just after
>> `GDK_DROP_START`. The leave event causes java to set `dragGesture` to
>> `null`. On Xorg this event is not sent.
>>
>> The fix just ignores the leave event if a drop occurred.
>
> modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp line 141:
>
>> 139: jobjectArray mimes;
>> 140: gint dx, dy;
>> 141: } enter_ctx = {NULL, FALSE, FALSE, NULL, 0, 0};
>
> it's funny how it's initialized here and also in reset_enter_ctx(), and just
> by coincidence both produce the same result. This will break if initial
> values are other than NULL, 0, FALSE.
Indeed. During my review, before I looked more closely, I was all ready to post
a comment to the effect that it was never set back to FALSE. Then I saw the
`memset` in `reset_enter_ctx` that you referred to. Not the way I would have
initially designed it -- relying on '0' being the desired default value for all
fields (including pointers) -- but given that it is that way, this doesn't make
the problem any worse.
-
PR: https://git.openjdk.org/jfx/pull/1005
Re: RFC: new property in ToggleGroup
I just want to add one thing - the initial state of RadioMenuItems added to a menu is unselected, even if they belong to a ToggleGroup. One can say that having all unselected radio buttons in a toggle group makes no sense, or perhaps it depends on the application requirements - though I cannot find an example where it might be needed. So either we allow two different policies by adding a property to the ToggleGroup, or we proclaim that adding a radio button to a toggle group must have a side effect of one (first) button to be automatically selected, otherwise the whole group is in inconsistent state (or the app developer must write some code to select one). -andy From: openjfx-dev on behalf of Kevin Rushforth Date: Friday, 2023/01/20 at 12:27 To: openjfx-dev@openjdk.org Subject: Re: RFC: new property in ToggleGroup How common a UI feature is being able to deselect the selected item in a ToggleGroup via the UI such that no item is selected? I don't normally see that in various apps or toolkits that I am familiar with. What I do see is that either a default item is selected or no item is selected initially (which is the one and only time that there will be no item selected), but in both case, once you make a selection, there is no way via the UI to deselect the current item. Absent a compelling need, I think the current behavior (once the fix for JDK-8237505 is integrated) is sufficient. What do other developers think? -- Kevin On 1/20/2023 11:31 AM, Andy Goryachev wrote: Dear colleagues: In the context of a recent PR https://bugs.openjdk.org/browse/JDK-8237505 https://github.com/openjdk/jfx/pull/1002 https://stackoverflow.com/questions/57911107/javafx-togglegroup-not-functioning-properly-with-accelerators-radiomenuitem where a number of RadioMenuItems belonging to a toggle group are added to the menu, we might want to add a new property to the ToggleGroup which controls whether all items in a group can be deselected. If this property is set, a selected radio menu item can be deselected via either keyboard accelerator or a mouse click. If not, then not only this operation cannot be performed, but also the first item added to said ToggleGroup gets automatically selected. This should allow for more flexibility in creating menus with RadioMenuItems, but eliminate some boilerplate code required in such cases. The new logic would also affect any Toggle, such as ToggleButton. What do you think? Thank you. -andy
Re: RFR: 8275033: Drag and drop a file produces NullPointerException Cannot read field "dragboard"
On Thu, 19 Jan 2023 16:16:18 GMT, Thiago Milczarek Sayao
wrote:
> When running on Wayland the `GDK_DRAG_LEAVE` is sent just after
> `GDK_DROP_START`. The leave event causes java to set `dragGesture` to `null`.
> On Xorg this event is not sent.
>
> The fix just ignores the leave event if a drop occurred.
Tested with FxDock on Xorg - no ill effects.
modules/javafx.graphics/src/main/native-glass/gtk/glass_dnd.cpp line 141:
> 139: jobjectArray mimes;
> 140: gint dx, dy;
> 141: } enter_ctx = {NULL, FALSE, FALSE, NULL, 0, 0};
it's funny how it's initialized here and also in reset_enter_ctx(), and just by
coincidence both produce the same result. This will break if initial values
are other than NULL, 0, FALSE.
-
Marked as reviewed by angorya (Committer).
PR: https://git.openjdk.org/jfx/pull/1005
Re: RFC: new property in ToggleGroup
How common a UI feature is being able to deselect the selected item in a ToggleGroup via the UI such that no item is selected? I don't normally see that in various apps or toolkits that I am familiar with. What I do see is that either a default item is selected or no item is selected initially (which is the one and only time that there will be no item selected), but in both case, once you make a selection, there is no way via the UI to deselect the current item. Absent a compelling need, I think the current behavior (once the fix for JDK-8237505 is integrated) is sufficient. What do other developers think? -- Kevin On 1/20/2023 11:31 AM, Andy Goryachev wrote: Dear colleagues: In the context of a recent PR https://bugs.openjdk.org/browse/JDK-8237505 https://github.com/openjdk/jfx/pull/1002 https://stackoverflow.com/questions/57911107/javafx-togglegroup-not-functioning-properly-with-accelerators-radiomenuitem where a number of RadioMenuItems belonging to a toggle group are added to the menu, we might want to add a new property to the ToggleGroup which controls whether all items in a group can be deselected. If this property is set, a selected radio menu item can be deselected via either keyboard accelerator or a mouse click. If not, then not only this operation cannot be performed, but also the first item added to said ToggleGroup gets automatically selected. This should allow for more flexibility in creating menus with RadioMenuItems, but eliminate some boilerplate code required in such cases. The new logic would also affect any Toggle, such as ToggleButton. What do you think? Thank you. -andy
RFC: new property in ToggleGroup
Dear colleagues: In the context of a recent PR https://bugs.openjdk.org/browse/JDK-8237505 https://github.com/openjdk/jfx/pull/1002 https://stackoverflow.com/questions/57911107/javafx-togglegroup-not-functioning-properly-with-accelerators-radiomenuitem where a number of RadioMenuItems belonging to a toggle group are added to the menu, we might want to add a new property to the ToggleGroup which controls whether all items in a group can be deselected. If this property is set, a selected radio menu item can be deselected via either keyboard accelerator or a mouse click. If not, then not only this operation cannot be performed, but also the first item added to said ToggleGroup gets automatically selected. This should allow for more flexibility in creating menus with RadioMenuItems, but eliminate some boilerplate code required in such cases. The new logic would also affect any Toggle, such as ToggleButton. What do you think? Thank you. -andy
Re: RFR: 8237505: RadioMenuItem in ToggleGroup: deselected on accelerator
On Thu, 19 Jan 2023 12:54:55 GMT, Karthik P K wrote:
> No check was present in `RadioMenuItem` accelerator to see if it is in a
> `ToggleGroup` or not.
>
> Made changes to check if `RadioMenuItem` is part of `ToggleGroup` or not. If
> it is part of a `ToggleGroup`, then it can not be cleared using accelerator.
>
> Added test to validate the change.
Tested with the MonkeyTester app.
I'd like to have a discussion on a new ToggleGroup's property (i'll send an
email to the mailing list).
This looks good, given the decision to not to allow deselection by an
accelerator.
But I wonder if the right approach is to have a property in the ToggleGroup
which determines whether to allow all items to be deselected. If set, the
radio menu items should be allowed to be deselected by both keyboard and mouse;
if not set, the first item added to a group should be automatically selected,
and radio menu items would not get deselected by the mouse or keyboard.
I think it might qualify as a separate enhancement (and a welcome one, since it
saves some boilerplate code).
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
line 178:
> 176: if (!menuitem.isDisable()) {
> 177: if (menuitem instanceof RadioMenuItem) {
> 178:
> if(((RadioMenuItem)menuitem).getToggleGroup() == null){
minor: this group insists on adding spaces after "if" and before "{"
-
Marked as reviewed by angorya (Committer).
PR: https://git.openjdk.org/jfx/pull/1002
Integrated: 8137244: Empty Tree/TableView with CONSTRAINED_RESIZE_POLICY is not properly resized
On Tue, 10 Jan 2023 19:34:33 GMT, Andy Goryachev wrote: > STEPS TO FOLLOW TO REPRODUCE THE PROBLEM : > 1. Add data to the tree/table with a constrained resize policy > 2. Clear the table > 3. Try to resize the tree/table > Expected: > - the tree/table columns get resized > Observed: > - columns are not resized > > Caused by an incomplete fix for RT-14855 / JDK-8113886 This pull request has now been integrated. Changeset: be89e3e3 Author:Andy Goryachev URL: https://git.openjdk.org/jfx/commit/be89e3e3c6c3a4479ee16b6e805a4a10fba6f7ab Stats: 111 lines in 5 files changed: 106 ins; 2 del; 3 mod 8137244: Empty Tree/TableView with CONSTRAINED_RESIZE_POLICY is not properly resized Reviewed-by: aghaisas - PR: https://git.openjdk.org/jfx/pull/991
Re: RFR: 8275033: Drag and drop a file produces NullPointerException Cannot read field "dragboard"
On Thu, 19 Jan 2023 16:16:18 GMT, Thiago Milczarek Sayao wrote: > When running on Wayland the `GDK_DRAG_LEAVE` is sent just after > `GDK_DROP_START`. The leave event causes java to set `dragGesture` to `null`. > On Xorg this event is not sent. > > The fix just ignores the leave event if a drop occurred. Looks good. I reproduced the bug on Wayland, and can confirm that this fixes the problem @andy-goryachev-oracle Can you also review this? - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.org/jfx/pull/1005
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v4]
On Fri, 20 Jan 2023 14:44:51 GMT, Nir Lisker wrote: > Looking at the current `ListView` docs, I think it's not very informative. > The main paragraph talks about generics in Java: This would be better done as a separate follow-up issue. - PR: https://git.openjdk.org/jfx/pull/995
Small changes to Gradle build scripts
Are small simplifying changes to the gradle scripts welcome?
E.g. instead of:
def inStream = new java.io.BufferedReader(new java.io.InputStreamReader(new
java.lang.ProcessBuilder(JAVA, "-fullversion").start().getErrorStream()));
try {
String v = inStream.readLine().trim();
if (v != null) {
int ib = v.indexOf("full version \"");
if (ib != -1) {
String str = v.substring(ib);
String ver = str.substring(str.indexOf("\"") + 1, str.size() - 1);
defineProperty("jdkRuntimeVersion", ver)
def jdkVersionInfo = parseJavaVersion(ver)
defineProperty("jdkVersion", jdkVersionInfo[0])
defineProperty("jdkBuildNumber", jdkVersionInfo[1])
// Define global properties based on the version of Java
// For example, we could define a "jdk18OrLater" property as
// follows that could then be used to implement conditional build
// logic based on whether we were running on JDK 18 or later,
// should the need arise.
// def status = compareJdkVersion(jdkVersion, "18")
// ext.jdk18OrLater = (status >= 0)
}
}
} finally {
inStream.close();
}
this:
def verMatch = [JAVA, "-fullversion"].execute().err.text =~ /full version
"([^"]+)/
if (verMatch.find()) {
String ver = verMatch.group(1);
defineProperty("jdkRuntimeVersion", ver)
def jdkVersionInfo = parseJavaVersion(ver)
defineProperty("jdkVersion", jdkVersionInfo[0])
defineProperty("jdkBuildNumber", jdkVersionInfo[1])
// Define global properties based on the version of Java
// For example, we could define a "jdk18OrLater" property as
// follows that could then be used to implement conditional build
// logic based on whether we were running on JDK 18 or later,
// should the need arise.
// def status = compareJdkVersion(jdkVersion, "18")
// ext.jdk18OrLater = (status >= 0)
}
or instead of:
ext.HAS_JAVAFX_MODULES = false;
def inStream2 = new java.io.BufferedReader(new java.io.InputStreamReader(new
java.lang.ProcessBuilder(JAVA, "--list-modules").start().getInputStream()));
try {
String v;
while ((v = inStream2.readLine()) != null) {
v = v.trim();
if (v.startsWith("javafx.base")) ext.HAS_JAVAFX_MODULES = true;
}
} finally {
inStream2.close();
}
this:
ext.HAS_JAVAFX_MODULES = [JAVA, '--list-modules'].execute().text.contains(
'javafx.base')
... much simpler and seems to work just as well.
They don't do much more than improve readability and reduce the line count
though, so I'm not sure if anyone cares.
Scott
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v3]
On Wed, 18 Jan 2023 09:25:08 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.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove extra spaces
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
151:
> 149: *
> 150: * Warning: Nodes should not be inserted directly into the items
> list
> 151: * ListView allows for the items list to contain elements of any type,
> including
`ListView` should be in `@code`
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
152:
> 150: * Warning: Nodes should not be inserted directly into the items
> list
> 151: * ListView allows for the items list to contain elements of any type,
> including
> 152: * {@link Node} instances. Putting nodes into
`Node` is already linked above so there's no need for it, but it's fine to
leave as is.
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
163:
> 161: * Avoid creating new {@link Node}s in custom {@link
> #cellFactoryProperty() cell factory} {@code updateItem} method.
> 162: *
> 163: * The following minimal example shows how to create a custom cell
> factory for {@code ListView} containing {@link Node}s:
No need to link `Node` here again.
-
PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v4]
On Fri, 20 Jan 2023 11:16:04 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. > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > Additional review fixes Left some minor inline comments that are relevant for the other classes here as well. Looking at the current `ListView` docs, I think it's not very informative. The main paragraph talks about generics in Java: > A ListView is able to have its generic type set to represent the type of data > in the backing model. Doing this has the benefit of making various methods in > the ListView, as well as the supporting classes (mentioned below), type-safe. > In addition, making use of the generic type supports substantially simplified > development of applications making use of ListView, as all modern IDEs are > able to auto-complete far more successfully with the additional type > information. I think that all of that should be removed, and your explanation on how to actually use `ListView` should be added instead. While it's tied to the "Customizing Visuals" section, the points of `ListView` is for it contain data/model instances which are interpreted as a visual `Node` by the `ListView`, so it should belong in the main paragraph. I can come up with a redistribution of the paragraphs if you want and if you don't consider this out of scope. - PR: https://git.openjdk.org/jfx/pull/995
[jfx19] Integrated: 8300771: Create release notes for 19.0.2.1
On Fri, 20 Jan 2023 12:51:04 GMT, Abhinay Agarwal wrote: > Add release notes for JavaFX 19.0.2.1 This pull request has now been integrated. Changeset: 34ca36f5 Author:Abhinay Agarwal Committer: Johan Vos URL: https://git.openjdk.org/jfx/commit/34ca36f59ae101e40cab08c041446020713f64d1 Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod 8300771: Create release notes for 19.0.2.1 Reviewed-by: kcr, jvos - PR: https://git.openjdk.org/jfx/pull/1006
Re: [jfx19] RFR: 8300771: Create release notes for 19.0.2.1
On Fri, 20 Jan 2023 12:51:04 GMT, Abhinay Agarwal wrote: > Add release notes for JavaFX 19.0.2.1 Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.org/jfx/pull/1006
Re: [jfx19] RFR: 8300771: Create release notes for 19.0.2.1
On Fri, 20 Jan 2023 12:51:04 GMT, Abhinay Agarwal wrote: > Add release notes for JavaFX 19.0.2.1 Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.org/jfx/pull/1006
[jfx19] RFR: 8300771: Create release notes for 19.0.2.1
Add release notes for JavaFX 19.0.2.1 - Commit messages: - 8300771: Create release notes for 19.0.2.1 Changes: https://git.openjdk.org/jfx/pull/1006/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1006=00 Issue: https://bugs.openjdk.org/browse/JDK-8300771 Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1006.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1006/head:pull/1006 PR: https://git.openjdk.org/jfx/pull/1006
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v4]
On Fri, 20 Jan 2023 11:16:04 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.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Additional review fixes
Looks good apart from one more missing "the". Once you add it, go ahead and
create the CSR and move it to "Proposed".
modules/javafx.controls/src/main/java/javafx/scene/control/ComboBox.java line
147:
> 145: * a custom {@link #cellFactoryProperty() cell factory}.
> 146: *
> 147: * The following minimal example shows how to create a custom cell
> factory for {@code ComboBox} containing {@link Node}s:
Can you also add the final note, after the example, about creating the
Rectangle in the instance init block or constructor?
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
191:
> 189: * This example has an anonymous custom {@code ListCell} class in
> the custom cell factory.
> 190: * Note that the {@code Rectangle} ({@code Node}) object needs to be
> created in the instance initialization block
> 191: * or the constructor of custom {@code ListCell} class and updated/used
> in its {@code updateItem} method.
of _the_ custom ...
-
PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v4]
> 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. Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision: Additional review fixes - Changes: - all: https://git.openjdk.org/jfx/pull/995/files - new: https://git.openjdk.org/jfx/pull/995/files/b4335b68..7b441fbf Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=995=03 - incr: https://webrevs.openjdk.org/?repo=jfx=995=02-03 Stats: 25 lines in 5 files changed: 1 ins; 2 del; 22 mod Patch: https://git.openjdk.org/jfx/pull/995.diff Fetch: git fetch https://git.openjdk.org/jfx pull/995/head:pull/995 PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v3]
On Wed, 18 Jan 2023 09:25:08 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. > > Ajit Ghaisas has updated the pull request incrementally with one additional > commit since the last revision: > > Remove extra spaces > Overall it looks great. I left a few comments on the wording. > > I presume all of the newly added examples compile? Yes. They do compile. - PR: https://git.openjdk.org/jfx/pull/995
Re: [jfx20] RFR: 8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list [v3]
On Wed, 18 Jan 2023 23:20:10 GMT, Kevin Rushforth wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove extra spaces
>
> modules/javafx.controls/src/main/java/javafx/scene/control/ComboBox.java line
> 126:
>
>> 124: * necessary to specify a custom {@link StringConverter}.
>> 125: *
>> 126: * Warning: Nodes should not be inserted directly into the ComboBox
>> items list
>
> Can you also add the bulleted list of "Important points to note" to
> `ComboBox`?
Added the bulleted list.
> I think it would read better to move this bullet above the previous so that
> the two "avoid"s are next to each other.
The current flow of 3 bullets seems natural to me.
The first point says - what to avoid?
The second point says - what is our recommendation.
The third point says - what to avoid in our recommendation.
> Also, that should either be "a custom cell factory updateItem method" (adding
> the missing article) or maybe "the updateItem method of a custom cell factory"
Fixed.
> modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
> 191:
>
>> 189: * This example has an anonymous custom {@code ListCell} class in
>> the custom cell factory.
>> 190: * Note that the {@code Rectangle} ({@code Node}) object needs to be
>> created in the custom {@code ListCell} class
>> 191: * or in its constructor and updated/used in its {@code updateItem}
>> method.
>
> I might just say "created in the constructor of the custom ListCell" and
> leave it at that. Saying "in the ... class or ... constructor" might be
> confusing, since the node is an instance field. The example uses an instance
> initialization block, which you could mention if you want to be more precise
> than just saying "in the constructor".
Reworded the description to include "instance initialisation block"
-
PR: https://git.openjdk.org/jfx/pull/995
Re: RFR: 8137244: Empty Tree/TableView with CONSTRAINED_RESIZE_POLICY is not properly resized
On Tue, 10 Jan 2023 19:34:33 GMT, Andy Goryachev wrote: > STEPS TO FOLLOW TO REPRODUCE THE PROBLEM : > 1. Add data to the tree/table with a constrained resize policy > 2. Clear the table > 3. Try to resize the tree/table > Expected: > - the tree/table columns get resized > Observed: > - columns are not resized > > Caused by an incomplete fix for RT-14855 / JDK-8113886 The fix looks good to me! - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.org/jfx/pull/991
