Re: RFR: 8328577: Toolbar's overflow button overlaps the items [v5]
On Mon, 15 Apr 2024 15:42:27 GMT, eduardsdv wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java >> line 575: >> >>> 573: box.getStyleClass().addListener((ListChangeListener>> String>) change -> overflowBox.getStyleClass().setAll(change.getList())); >>> 574: overflowBox.getStylesheets().setAll(box.getStylesheets()); >>> 575: box.getStylesheets().addListener((ListChangeListener>> String>) change -> overflowBox.getStylesheets().setAll(change.getList())); >> >> this is interesting. >> >> isn't `overflowBox` a sibling of `box`, having the same parent, and >> therefore inheriting the same set of stylesheets from the parent `Scene`? > > In addition to those from the scene, each ``Parent`` can have its own > stylesheets. OK, you are correct - the `box` is technically visible via node lookup by the "content" style, so the app developer can attach a stylesheet to it (why they would do it is another question, I am sure there are many other places where doing so would break the code). And we are not creating leaks because both nodes are siblings. - PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1566017946
Re: RFR: 8328577: Toolbar's overflow button overlaps the items [v5]
On Mon, 15 Apr 2024 15:24:06 GMT, Andy Goryachev wrote: >> eduardsdv has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - JDK-8328577: Update comment >> - JDK-8328577: Bind style related properties > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java > line 575: > >> 573: box.getStyleClass().addListener((ListChangeListener> String>) change -> overflowBox.getStyleClass().setAll(change.getList())); >> 574: overflowBox.getStylesheets().setAll(box.getStylesheets()); >> 575: box.getStylesheets().addListener((ListChangeListener> String>) change -> overflowBox.getStylesheets().setAll(change.getList())); > > this is interesting. > > isn't `overflowBox` a sibling of `box`, having the same parent, and therefore > inheriting the same set of stylesheets from the parent `Scene`? In addition to those from the scene, each ``Parent`` can have its own stylesheets. - PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1566002787
Re: RFR: 8328577: Toolbar's overflow button overlaps the items [v5]
On Mon, 15 Apr 2024 15:10:42 GMT, Andy Goryachev wrote: >> eduardsdv has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - JDK-8328577: Update comment >> - JDK-8328577: Bind style related properties > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java > line 573: > >> 571: overflowBox.idProperty().bind(box.idProperty()); >> 572: overflowBox.getStyleClass().setAll(box.getStyleClass()); >> 573: box.getStyleClass().addListener((ListChangeListener> String>) change -> overflowBox.getStyleClass().setAll(change.getList())); > > I think what you need here (and below) is `Bindings.bindContent(List, > ObservableList)` Good suggestion - PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1565996530
Re: RFR: 8328577: Toolbar's overflow button overlaps the items [v5]
On Mon, 15 Apr 2024 12:38:15 GMT, eduardsdv wrote: >> This change fixes the calculation of which nodes go to the toolbar and which >> go to the overflow menu. >> It is now determined before the nodes are removed from the scene graph. >> This is important because the values returned by >> ``Node.prefWidth(..)``/``Node.prefHeight(..)`` may depend on whether the >> node is added to the scene graph. >> >> Furthermore I corrected the ``hasOveflow`` value passed to the >> ``organizeOverflow(double, boolean)`` in ``correctOverflow(double)``. > > eduardsdv has updated the pull request incrementally with two additional > commits since the last revision: > > - JDK-8328577: Update comment > - JDK-8328577: Bind style related properties modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java line 573: > 571: overflowBox.idProperty().bind(box.idProperty()); > 572: overflowBox.getStyleClass().setAll(box.getStyleClass()); > 573: box.getStyleClass().addListener((ListChangeListener String>) change -> overflowBox.getStyleClass().setAll(change.getList())); I think what you need here (and below) is `Bindings.bindContent(List, ObservableList)` modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java line 575: > 573: box.getStyleClass().addListener((ListChangeListener String>) change -> overflowBox.getStyleClass().setAll(change.getList())); > 574: overflowBox.getStylesheets().setAll(box.getStylesheets()); > 575: box.getStylesheets().addListener((ListChangeListener String>) change -> overflowBox.getStylesheets().setAll(change.getList())); this is interesting. isn't `overflowBox` a sibling of `box`, having the same parent, and therefore inheriting the same set of stylesheets from the parent `Scene`? - PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1565958505 PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1565977562
Re: RFR: 8328577: Toolbar's overflow button overlaps the items [v5]
> This change fixes the calculation of which nodes go to the toolbar and which > go to the overflow menu. > It is now determined before the nodes are removed from the scene graph. > This is important because the values returned by > ``Node.prefWidth(..)``/``Node.prefHeight(..)`` may depend on whether the node > is added to the scene graph. > > Furthermore I corrected the ``hasOveflow`` value passed to the > ``organizeOverflow(double, boolean)`` in ``correctOverflow(double)``. eduardsdv has updated the pull request incrementally with two additional commits since the last revision: - JDK-8328577: Update comment - JDK-8328577: Bind style related properties - Changes: - all: https://git.openjdk.org/jfx/pull/1434/files - new: https://git.openjdk.org/jfx/pull/1434/files/6478f38b..e63b0b9b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1434&range=04 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1434&range=03-04 Stats: 15 lines in 1 file changed: 12 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1434.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1434/head:pull/1434 PR: https://git.openjdk.org/jfx/pull/1434
