8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list

[aghaisas]

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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8301002 to be approved
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

• JDK-8290863: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list
• JDK-8301002: Update the documentation of Virtualized controls to include the best practice of not using Nodes directly in the item list (CSR)

Reviewers

• Andy Goryachev (@andy-goryachev-oracle - Committer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/995/head:pull/995
$ git checkout pull/995

Update a local copy of the PR:
$ git checkout pull/995
$ git pull https://git.openjdk.org/jfx pull/995/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 995

View PR using the GUI difftool:
$ git pr show -t 995

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/995.diff

[bridgekeeper]

👋 Welcome back aghaisas! A progress list of the required criteria for merging this PR into jfx20 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[aghaisas]

/csr

[aghaisas]

/reviewers 2

[openjdk]

@aghaisas has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@aghaisas please create a CSR request for issue JDK-8290863 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[openjdk]

@aghaisas
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[mlbridge]

Webrevs

• 04: Full - Incremental (84423f5a)
• 03: Full - Incremental (7b441fbf)
• 02: Full - Incremental (b4335b68)
• 01: Full - Incremental (e46e31b4)
• 00: Full (4325a8e4)

[nlisker]

Instead of repeating this in every class, maybe writing it once in Cell and linking to it from these classes will be better?

[aghaisas]

Thanks @andy-goryachev-oracle, @kevinrushforth and @nlisker for your review. My responses are inline -

After thinking about it further, I agree with Andy. Having a bulleted list of what to avoid, and a code sample of the right way to do it, seems best.

I am convinced that we should avoid writing out the non-recommended code snippet in documentation. I will replace it with bulleted list as suggested.

Instead of repeating this in every class, maybe writing it once in Cell and linking to it from these classes will be better?

@kevinrushforth and I did think about this suggestion before deciding to put the warning explicitly in each of the virtualized controls. Each of ListView, TreeView, TableView and TreeTableView control documentation has a section about "Customizing Visuals" which has a link to Cell documentation.

• The documentation for Cell already has an example of custom cell factory and also a bulleted list of key design goals.
  Despite this we see multiple occurrences of Nodes being put directly in the item list of virtualized controls. Hence, it was decided to replicate the explicit warning similar to the one we already have in the documentation of ComboBox.
• I feel it is better to have an example of custom factory specific to each of the virtualized controls which we cannot put in generic Cell documentation.

[andy-goryachev-oracle]

Have only minor suggestion, the extra space will not make any difference as it won't be seen in the browser.
Will re-approve should you decide to fix them.

[andy-goryachev-oracle]

lg, thanks!

[kevinrushforth]

Overall it looks great. I left a few comments on the wording.

I presume all of the newly added examples compile?

[aghaisas]

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.

[kevinrushforth]

Looks good apart from one more missing "the". Once you add it, go ahead and create the CSR and move it to "Proposed".

[nlisker]

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.

[kevinrushforth]

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.

[aghaisas]

@nlisker, Can you please file an issue explaining how exactly you would like ListView documentation to be restructured?

[kevinrushforth]

Looks good.

[andy-goryachev-oracle]

LG

[nlisker]

@nlisker, Can you please file an issue explaining how exactly you would like ListView documentation to be restructured?

Yes, I'll probably work on it during RDP 2. It's not the only control I want to go over.

[openjdk]

@aghaisas This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 15 new commits pushed to the jfx20 branch:

• a4bc9d1: 8300013: Node.focusWithin doesn't account for nested focused nodes
• 05a67df: 8300664: Missing copyright header in ConditionalBinding.java file
• 1ccb586: Merge
• 5a70e4f: 8299628: BMP top-down images fail to load after JDK-8289336
• 98cff2f: Merge
• d4f89d8: Merge
• 328bc40: Merge
• 54187d5: Merge
• 4f26251: 8294779: Improve FX pages
• 3fde080: 8289336: Better platform image support
• ... and 5 more: https://git.openjdk.org/jfx/compare/8136b11fdc75aba64d8b77cf286097db3e12d6c9...jfx20

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the jfx20 branch, type /integrate in a new comment.

[aghaisas]

/integrate

[openjdk]

Going to push as commit 39d8747.
Since your change was applied there have been 15 commits pushed to the jfx20 branch:

• a4bc9d1: 8300013: Node.focusWithin doesn't account for nested focused nodes
• 05a67df: 8300664: Missing copyright header in ConditionalBinding.java file
• 1ccb586: Merge
• 5a70e4f: 8299628: BMP top-down images fail to load after JDK-8289336
• 98cff2f: Merge
• d4f89d8: Merge
• 328bc40: Merge
• 54187d5: Merge
• 4f26251: 8294779: Improve FX pages
• 3fde080: 8289336: Better platform image support
• ... and 5 more: https://git.openjdk.org/jfx/compare/8136b11fdc75aba64d8b77cf286097db3e12d6c9...jfx20

Your commit was automatically rebased without conflicts.

[openjdk]

@aghaisas Pushed as commit 39d8747.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.