Update darklaf to version 3.0.2

[weisJ]

Description

Update darklaf to version 3.0.2. The versions since then include a bunch of bug fixes and improvements. It is now fully compliant with the java module system. Also the svg renderer has been replaced with a different implementation that allows for better support in the future. For a full list of changes see the release notes.

This means that darklaf doesn't need svgSalamander anymore. I'm not sure how to go about this. Should I remove the dependency on svgSalamander or leave it in?

Checklist:

• My code follows the code style of this project.
• I have updated the documentation accordingly.

[codecov-commenter]

Codecov Report

Merging #5715 (65043c0) into master (4caddd8) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #5715      +/-   ##
============================================
- Coverage     55.24%   55.24%   -0.01%     
+ Complexity    10387    10386       -1     
============================================
  Files          1062     1062              
  Lines         65777    65776       -1     
  Branches       7536     7536              
============================================
- Hits          36339    36337       -2     
  Misses        26838    26838              
- Partials       2600     2601       +1     

Impacted Files	Coverage Δ
.../src/main/java/org/apache/jmeter/SplashScreen.java	58.97% <ø> (ø)
...java/org/apache/jmeter/gui/util/JMeterToolBar.java	66.16% <ø> (ø)
...g/apache/jmeter/gui/action/LookAndFeelCommand.java	48.86% <100.00%> (+0.54%)	⬆️
...a/org/apache/jmeter/timers/PoissonRandomTimer.java	72.97% <0.00%> (-5.41%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4caddd8...65043c0. Read the comment docs.

[vlsi]

Nice, thank you.

Well, I used SVG icon for the splash screen in

jmeter/src/core/src/main/java/org/apache/jmeter/SplashScreen.java

Line 78 in 4caddd8

Icon icon = new ThemedSVGIcon(svgUri, 521, 177);

Would it still work without svgSalamander?

If that works, I guess we can remove svgSalamander dependency, and its license override.

[weisJ]

Yes it will still work. Besides the changes in package locations the API surface has stayed the same. I checked and my new svg implementation is capable of rendering all svg files in the repository (not to 100% as some filters aren't supported, but those also weren't rendered before).

[vlsi]

Would you please delete /src/licenses/licenses/svgSalamander directory as well?

[weisJ]

Done

[vlsi]

Thank you.

I just found there's compile-time SVG -> Swing calls converter by @kirill-grouchnikov https://github.com/kirill-grouchnikov/radiance/tree/aa3a780a7e52de59070176e0183074c8d0efb3c2/tools/svg-transcoder/src/main/java/org/pushingpixels/radiance/tools/svgtranscoder/api (see https://github.com/kirill-grouchnikov/radiance/blob/sunshine/docs/tools/svg-transcoder/svg-transcoder.md)

Moving "SVG parsing" to compile time looks clever, however, I'm not sure it would improve much for the current state of JMeter: I guess the number of icons is small, so the parsing is probably fast.

[vlsi]

@weisJ , just wondering, is this ready to go re darklaf?

[weisJ]

All fixes I’m currently working on only regard some erroneous warning messages or the svg implementation. Both problems don’t apply to JMeter, so the current darklaf version should be good to go.

[vlsi]

I have rebased the PR, however, it looks like this upgrade breaks dialogs.

For instance, if run JMeter (e.g. ./gradlew runGui), add something to the test plan, and then try to close JMeter, the buttons for yes/no/cancel are tiny.
That is Darklaf-IntelliJ theme, however buttons are tiny in all Darklaf themes for me. macOS 13.2.1, Java 11.0.13

@weisJ , I have not checked what could be the reason, however, does this behaviour ring a bell for you?

[weisJ]

The issue here seems to be the usage of a custom ClassLoader. The resource bundles containing those strings are loaded by UIDefaults#getResourceCache, which uses the system classloader for non java.dekstop bundles. I'm not sure how to go about this.

[weisJ]

On another note. MigLayout supports so called visual paddings (e.g. shadows etc.) which are taken to be outside components for layoutpurposes. I am providing those values with darklaf but it results in a minor annoyance.
Miglayout completely ignores the visual paddings for layout purposes: This means that it doesn't check that the "padding-area" sits completely inside the parten container. This results in focus borders being cut off on some edges (righ and bottom to be precise). The simplest solution would be to disable visual padding with all MigLayout instances. Any thoughts?

[vlsi]

The issue here seems to be the usage of a custom ClassLoader

Could you please clarify which classloader do you mean?

The resource bundles containing those strings are loaded by UIDefaults#getResourceCache, which uses the system classloader for non java.dekstop bundles

I thought the button labels "yes/no/cancel" should be loaded from the system bundles. The dialog uses a regular JOptionPane.YES_NO_CANCEL_OPTION, so do not understand why custom classloaders break it.

[vlsi]

MigLayout supports so called visual paddings (e.g. shadows etc.) which are taken to be outside components for layoutpurposes

Do you mean ill focus around comments is caused by improper handling of the padding?

Miglayout completely ignores the visual paddings for layout purposes

First you say Miglayout supports the visual paddings, then you say it does not. I'm puzzled.

The simplest solution would be to disable visual padding with all MigLayout instances. Any thoughts?

Could you clarify what do you mean?
By the way, there are recent releases of miglayout. Have you tried them?
https://github.com/mikaelgrev/miglayout

Have you evaluated if fixing miglayout is an option?

[weisJ]

Could you please clarify which classloader do you mean?

I am referring to the DynamicClassLoader here

jmeter/src/launcher/src/main/java/org/apache/jmeter/NewDriver.java

Line 55 in b069225

private static final DynamicClassLoader loader;

I thought the button labels "yes/no/cancel" should be loaded from the system bundles. The dialog uses a regular JOptionPane.YES_NO_CANCEL_OPTION, so do not understand why custom classloaders break it.

Usually they are and before strong encapsulation was fully enforced third party LaFs were able to load them aswell, but with newer Java versions that is no longer possible. Hence the resource bundle as the replicated and packaged separately. Sadly due to the implementation of UIDefaults these are always loaded using the system classloader:

https://github.com/openjdk/jdk/blob/197d0cc6031cb470f1bd7678796593ff1bf440ca/src/java.desktop/share/classes/javax/swing/UIDefaults.java#L318

I have now opted for duplicating the resource cache, which fixes this issue.

---

First you say Miglayout supports the visual paddings, then you say it does not. I'm puzzled.

I'm sorry I have reworded my comment a few times and it came out rather scrabled. Let me clarify.

Miglayout applies the adjustments for visualPaddings after it has already determined the location for the component (which of course takes these paddings into account). However a constraint of fillx will always result in the component to always have the right edge before the paddings are compensated for. This results in the child components bounding box extending outside of its parent, hence the clipped off focus ring.

Newer versions of MigLayout seem to have the same problem. I have submitted a PR mikaelgrev/miglayout#94 which should fix it.

[vlsi]

@weisJ , it looks like Java lacks ability to attach a resource bundle: https://bugs.openjdk.org/browse/JDK-4834404

They say

addResourceBundle was intended for internal use only, therefor it doesn't
correctly resolve non-system resource bundles

So it looks like the ways out could be:

a) manually load the resource bundles and add all values from them to UIDefaults
b) move darklaf.jar (or a smaller jar with resources) to a system classpath

Technically speaking, I think we should stop using DynamicClassLoader as it makes the code hard to reason, and it makes the loading tricky.
I believe, the only reason for having DynamicClassLoader is:

1. ability to load plugins specified in the UI (extra testplan classpath field)
2. ability to use a small launcher.jar without supplying full classpath on the command line

We can probably fix it, however, it would take time, so I incline to an approach that would load bundles and pass the values to UIDefault.put(..).

[vlsi]

@weisJ , the workaround with "putting all the custom bundle values to defaults" works for cases like Actions.cut, however, it does not heal JOptionPane.YES_NO_CANCEL_OPTION.

I believe the reason is that:

• Darklaf uses putAll instead of delegation in https://github.com/weisJ/darklaf/blob/0622af33ba287b33f415e062158dfe4cc7b0d763/core/src/main/java/com/github/weisj/darklaf/DarkLaf.java#L164
  In other words, in the ideal case, darklaf should use delegation to the defaults like "try searching the element locally, or delegate to the other UIDefaults if not found".
• Java uses custom scheme for resolving OptionPane.yesButtonText. It detects that resource bundle does not have exact OptionPane.yesButtonText property, however, it attempts loading OptionPane.yesButtonTextAndMnemonics instead: https://github.com/openjdk/jdk/blob/197d0cc6031cb470f1bd7678796593ff1bf440ca/src/java.desktop/share/classes/javax/swing/UIDefaults.java#L1351
  unfortunately, the automatic attempt for yesButtonText -> yesButtonTextAndMnemonics happens only for resource ids loaded from resource bundle, which, as you know, are loaded only from the system classloader.

I would suggest:

1. Stop re-distributing "jdk" bundles in Darklaf
2. Replace defaults.putAll(baseDefaults); with something like MultiUIDefaults so you delegate to baseDefaults rather than get raw values from there

I believe it would fix JOptionPane.YES_NO_CANCEL_OPTION behaviour and it would simplify Darklaf code as you will no longer need to re-distribute labels for Java default bundles.

I've created weisJ/darklaf#341 for that.

WDYT?