On Wed, 19 Aug 2026 19:20:16 GMT, Andy Goryachev <[email protected]> wrote:
>> eduardsdv has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8389970: Add mnemonic registration even if ContentDisplay==GRAPHIC_ONLY
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 97:
>
>> 95: public void teardown() {
>> 96: if (stageLoader != null) {
>> 97: stageLoader.dispose();
>
> `stageLoader = null;`
>
> to avoid using stale stageLoader left over from an earlier run?
As far as I know, it is not necessary, because the Junit creates a new instance
of the test class for each test method call.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2127:
>
>> 2125: checkMnemonics();
>> 2126: }
>> 2127:
>
> I was trying to check whether mnemonic is registered with the scene, but this
> test fails with NPE before that:
>
>
> @Test
> public void withGraphic() {
> assumeFalse(PlatformUtil.isMac());
> label.setText("_test");
> label.setMnemonicParsing(true);
> label.setGraphic(new Label());
> label.setContentDisplay(ContentDisplay.GRAPHIC_ONLY);
> stageLoader = new StageLoader(label);
> Toolkit.getToolkit().firePulse(); // NPE
Interesting. I've added extra tests that permutate properties involved into
mnemonic processing and I got the error too.
I've reworked some methods and removed obsolete flags.
Some cases were not handled properly:
- the ``getCleanText()`` method did not update the ``MnemonicInfo`` if the text
was null
- presence of two methods to add/remove mnemonic registration. These methods
were called from different places, but not in pairs. This led to multiple
mnemonic registrations. I replaced them by ``updateMnemonicRegistration()``. It
concentrates all the knowledge, whether the mnemonic is already added, needs to
be replaced or removed.
- multiple flags made it difficult to oversee the state and the correct value
combinations. The 'MnemonicInfo' instance is now the only source of truth.
Now all tests running green again.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2166:
>
>> 2164:
>> 2165: // Mac does not support mnemonics
>> 2166: boolean expectedMnemonics = PlatformUtil.isMac() ? false :
>> label.isMnemonicParsing();
>
> could we also check whether mnemonic is actually registered with the scene,
> using `label.getScene().getMnemonic()`?
Done.
The scene is now always accessed via the ``label``.
See the ``label.getScene().getMnemonics()`` in ``checkMnemonics()`` method.
Function<String, Boolean> mnemonicRegistrationChecker = character ->
label.getScene().getMnemonics()
....
> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinTest.java
> line 2261:
>
>> 2259:
>> 2260: private StageLoader createStageLoader(Node node) {
>> 2261: if (stageLoader == null) {
>
> this works only if `stageLoader` is null'ed in `@AfterEach`
I've removed this method.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3823715467
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3823725476
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3823717693
PR Review Comment: https://git.openjdk.org/jfx/pull/2253#discussion_r3823719114
