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 Tue, 24 Jan 2023 09:59:48 GMT, Ajit Ghaisas wrote:
>> 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.
>
> The first occurrence of `{@link Node}` is on line 152. I have kept the first
> occurrence and replaced others with `{@code Node}`
It's linked in the type hierarchy, but I'm fine with keeping an inline link.
-
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 Thu, 19 Jan 2023 05:24:06 GMT, Nir Lisker 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/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.
The first occurrence of `{@link Node}` is on line 152. I have kept the first
occurrence and replaced others with `{@code Node}`
-
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
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 [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: [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?
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`?
modules/javafx.controls/src/main/java/javafx/scene/control/ListView.java line
161:
> 159: * provide a custom {@link #cellFactoryProperty() cell factory} to
> create the nodes for a
> 160: * given cell and update them on demand using the data stored in the
> item for that cell.
> 161: * Avoid creating new {@link Node}s in custom {@link
> #cellFactoryProperty() cell factory} {@code updateItem} method.
I think it would read better to move this bullet above the previous so that the
two "avoid"s are next to each other.
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"
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".
-
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 lg, thanks! - Marked as reviewed by angorya (Committer). 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]
> 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 - Changes: - all: https://git.openjdk.org/jfx/pull/995/files - new: https://git.openjdk.org/jfx/pull/995/files/e46e31b4..b4335b68 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=995&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=995&range=01-02 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 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
