8371067: RichTextArea: requestLayout by inline node doesn't reach VFlow #1975
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| // https://github.com/andy-goryachev/AppFramework/blob/1e9f2197ce510a77ec5f719a2cb7112b0b6cf7be/src/goryachev/fx/FX.java#L1081 | ||
| // with the author's permission | ||
| /** returns a parent of the specified type, or null. if node is an instance of the specified class, returns node */ | ||
| public static <T> T getAncestorOfClass(Class<T> c, Node node) { |
There was a problem hiding this comment.
I would really recommend to not do anything like that. Normally, the layouting system, especially when requesting a layout should work and bubble up correctly. I wonder why this is needed. And if we found a special case, if we can solve it better.
Background: In the past projects, I often saw code like that and it turned out that this was never needed. It is often less readable and hurts the performance a bit.
We also improve coupling between components, which I would not recommend as well. Especially since subclasses can change a lot and it would be nice if everything still work out of the box.
There was a problem hiding this comment.
Thank you @Maran23 for looking into this PR!
My experience is exactly opposite - I do use it often. The Node (or JComponent) hierarchy is a hierarchy, explicitly retaining the pointers to the each member's parent.
A specific member can be a child of a certain Parent, direct or otherwise, by design, and this method allows to get to that parent easily.
Let me ask you this - what is the alternative? Maintain a duplicate pointer?
Also, keep in mind this is not public API, it's a utility.
There was a problem hiding this comment.
What I mean is:
Everything is part of the RichTextArea. Requesting a layout from a node inside should request a layout for each parent, also RichTextArea. So you should normally know that on the RichTextArea level, which also manages the VFlow. So it can do the corresponding actions, just by the node requesting the layout.
If we take a look at other more complex Controls, they do the following:
VirtualFlow.requestCellLayout-> sets a flaglayoutis requested, and due to the flag, we know what to do
Maybe something that could be done here as well? Could also be done by Properties perhaps.
A specific member can be a child of a certain Parent, direct or otherwise, by design, and this method allows to get to that parent easily.
...
Also, keep in mind this is not public API, it's a utility.
Yes, we can always get the parent hierarchy. But that does not mean we should.
Making assumptions about the hierarchy will make subclasses and customizations (e.g. in the Skin) worse. If we extend RichTextArea and use another Node then VFlow, then we can expect TextCells not to work anymore?
In JavaFX, as you can also see in the codebase and other controls, retrieving an ancestor somewhere in the scene graph is pretty much never done or needed.
I did not have a look on this particular issue, but what I want to suggest is to take another look at the problem and how to solve it. So we don't need to rely on finding a specific node that might be somewhere in the scene graph.
There was a problem hiding this comment.
If I remember correctly from what I've traced, is that requestLayout from the embedded node is propagating upwards until it reaches TextCell which extends a BorderPane. At this point requestLayout in Parent invokes markDirtyLayout with the forceParentLayout parameter/flag being false and the propagation stops.
There was a problem hiding this comment.
I know why. Because the TextCell is not managed.
Therefore, this will be set:
jfx/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java
Lines 1331 to 1334 in f87448e
Indeed, the TextCell will be handled as layout root, therefore not propagating any layout request further up.
So the real problem is, that the TextCell is not managed.
Otherwise the layout would be propagated to the RichTextArea, which can do the corresponding actions. So I would suggest looking into that.
There was a problem hiding this comment.
Let me repeat what I mean:
- If you call:
setManaged(false), you will effectively tell JavaFX: I'm the layout root, I can handle ALL layout changes from my children without my parents. - This is not the case here, so making the
TextCellunmanaged while still retrieving a parent (VFlow) fights this design. - Which leads me to the conclusion: Maybe this should not be unmanaged, since it does not fulfill the required preconditions
- Checking the
VirtualFlowas an example, it will not set anything to unmanaged
Lets check some existing examples of unmanaged nodes:
XYChart. See how this was done by design, the consequences were known:
jfx/modules/javafx.controls/src/main/java/javafx/scene/chart/XYChart.java
Lines 523 to 525 in 33abf5d
ScrollPaneSkin has actually the same case as you. This is how they dealt with it:
This could be a solution for your problem. Since they also set the parent viewRect as unmanaged, the layout request will never hit the ScrollPane. But with that solution the child of the viewRect, called viewContent does request the ScrollPane layout. A clever solution without anything like ancestor searching. What do you think?
There was a problem hiding this comment.
The documentation on Node.managed property states
Defines whether or not this node's layout will be managed by its parent.
The layout of TextCell is not managed by its parent (VFlow.content), by design.
The VFlow is the entity that does the layout, for a reason. It's a complicated thing, and multiple moving parts are connected (one example: it performs multiple layout cycles in the same layout pass when scrollbars appear/disappear during the layout pass, to avoid jumping and flicker - something we occasionally see with ScrollPane and ListView, see Note 1).
Notes
- I occasionally see the continuous flicker/layout when resizing the list of pages in the latest Monkey Tester, but so far I was unable to capture the exact conditions to create a reproducible test case (this is unrelated to this PR)
There was a problem hiding this comment.
Did you read what I wrote above? I made multiple suggestions as how we can deal with the situation, even without making things managed again.
To reiterate, what is the problem?
- We now have a dependency from
TextCelltoVFlow. Not explicitly, but rather hidden, by searching all parents (which is also costly when doing that often). I would prefer a clear separation of concerns - This makes subclassing/extending harder. What happens, if I want to write my own
VFlow, but still use everything else, includingTextCell? What if I want to useTextCellfor a component, that has noVFlow? What happens if we find aVFlowfrom another component even? Because we used theTextCellsomewhere else? - Other components made it clear how to use
managedandunamangednodes. And how to bubble up a request still. Why do we want to make an exception here? This is the first time we need to search the parent hierarchy - Why not e.g. binding to the
needsLayoutPropertyfrom aTextCellfrom withinVFlowand just request the layout when set?
Also another point: The test is green for me with the changes from: #1945
So I would like to know, if maybe there was a bug even?
There was a problem hiding this comment.
Thank you for looking into this and offering your suggestions! The thing is, the RTA is a bit more complex than the situation you describe, due to the presence of other requirements (such as side areas that house line numbers and possibly other paragraph decorators).
The design of the skin is such that only VFlow does the layout. The VFlow is the sole actor that deals with its complicated content. Without a very good reason, this design is unlikely to change. Furthermore, the VFlow is still an implementation detail, so one can't just subclass the skin (this is true for other controls as well). Neither VFlow nor TextCell nor RichUtils are public API.
Yes, the test is green with #1945 but it fails in master. And yes, the bug is still there even with #1945 , as can be seen with the monkey tester:
expected:
observed:
There was a problem hiding this comment.
While looking at the test, discovered an issue with the SimpleViewOnlyStyledModel:
https://bugs.openjdk.org/browse/JDK-8372438
thanks!
| */ | ||
| public void add(Node node) { | ||
| flow().getChildren().add(node); | ||
| embedsNode = true; |
There was a problem hiding this comment.
Is the intention that if the TextCell has no children (neither Text nor embedded nodes) that getAncestorOfClass is bypassed ?
Or is this to indicate that getAncestorOfClass should only be invoked if there are any embedded nodes (non Text ones) present ?
If the latter then note that add(Node) is called for both the adding of Text and embedded nodes and maybe you intended to overload the add method with add(Text) ?
There was a problem hiding this comment.
All this is by design. VFlow is the component that lays out the text cells.
There was a problem hiding this comment.
Umm, I find this code ambiguous - so just trying to clarify which of the two options presented you intend ?
If it's the first then maybe an alternative variable/flag name should be considered ?
If it's the second then embedsNode is always set true as soon as any content is added which means the first option is what is actually occurring ?
There was a problem hiding this comment.
The expectation is that most text cells contain TextFlow, with the VFlow managing the layout. Once VFlow determines the target width, it queries the TextFlow for its preferred height to determine the actual size.
For paragraphs that contains Regions - either inline or as whole paragraphs - this logic needs to be augmented to propagate the requestLayout() flag up the hierarchy. We don't want to do this for pure text cells, but we must do it for embedded nodes.
addNode(Node) handles the inline node case, RichParagraph.of(Supplier<Region>) does the "full-width" case.
so to answer your question - if I understand the issue - the embedsNode flag was added to enable telling VFlow that it needs to reflow because some embedded node asked for it. Pure text cells don't need this signaling enabled.
Did I answer your question (satisfactorily :-) ?
There was a problem hiding this comment.
I think so ....
Pure text cells don't need this signaling enabled.
If this is what is intended, then from how I read the code that is not what is happening because add(Node) is called even for Text only cells. See:
There was a problem hiding this comment.
you are right, thank you so much!
There was a problem hiding this comment.
Thanks again @Jugen for pointing this out. Sorry, it took a while for me to understand what's going on.
| VFlow vf = RichUtils.getAncestorOfClass(VFlow.class, this); | ||
| if (vf != null) { | ||
| vf.requestLayout(); | ||
| } |
There was a problem hiding this comment.
Would setNeedsLayout(true); maybe work here instead ?
There was a problem hiding this comment.
no, we need to signal this to VFlow who does the layout.
|
/reviewers 2 |
|
@andy-goryachev-oracle |
3 participants
Requesting
VFlowre-layout when signaled by aNodeembedded inTextCell.NOTES
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1975/head:pull/1975$ git checkout pull/1975Update a local copy of the PR:
$ git checkout pull/1975$ git pull https://git.openjdk.org/jfx.git pull/1975/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1975View PR using the GUI difftool:
$ git pr show -t 1975Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1975.diff
Using Webrev
Link to Webrev Comment