On Wed, 30 Nov 2022 21:24:02 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8294589: cleanup > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java > line 374: > >> 372: >> 373: // When the parent window looses focus - menu selection >> should be cleared >> 374: >> sceneListenerHelper.addChangeListener(scene.windowProperty(), true, (sr, >> oldw, w) -> { > > Suggestion: > > sceneListenerHelper.addChangeListener(scene.windowProperty(), > true, w -> { my version does not create extra object(s). > modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java > line 381: > >> 379: >> 380: if (w != null) { >> 381: windowFocusHelper = >> sceneListenerHelper.addChangeListener(w.focusedProperty(), true, (s, p, >> focused) -> { > > Suggestion: > > windowFocusHelper = > sceneListenerHelper.addChangeListener(w.focusedProperty(), true, focused -> { my version does not create extra object(s). > modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java > line 387: > >> 385: }); >> 386: } >> 387: }); > > The goal seems to be to subscribe to changes on scene->window->focused. > > This can be done with `flatMap` / `orElse`. > > Suggestion: > > // When the parent window looses focus - menu selection > should be cleared > sceneListenerHelper.addChangeListener( > scene.windowProperty() > .flatMap(Window::focusedProperty) > .orElse(false), > true, > focused -> { > if (!focused) { > unSelectMenus(); > } > } > ); > > > You could even do this at the top level I think: > > lh.addChangeListener( > control.sceneProperty() > .flatMap(Scene::windowProperty) > .flatMap(Window::focusedProperty) > .orElse(false), > true, > focused -> { > if (!focused) { > unSelectMenus(); > } > } > ); > > Also available is to make use of `Subscription`: > > Subscription sub = > Subscription.subscribe(scene.windowProperty().flatMap(Window::focusedProperty).orElse(false), > focused -> { > if (!focused) { > unSelectMenus(); > } > }); > > The subscription can then be integrated with `ListenerHelper` again (or even > more directly if it accepted `Subscription`): > > lh.addDisconnectable(sub::unsubscribe); Good suggestions. It is basically the existing code - I'd prefer to keep it as is, since re-writing it might introduce regression. ------------- PR: https://git.openjdk.org/jfx/pull/906