You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by GitBox <gi...@apache.org> on 2022/09/30 19:02:05 UTC
[GitHub] [jmeter] weisJ opened a new pull request, #5715: Update darklaf to version 3.0.2
weisJ opened a new pull request, #5715:
URL: https://github.com/apache/jmeter/pull/5715
## 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](https://github.com/weisJ/darklaf/releases).
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:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] My code follows the [code style][style-guide] of this project.
- [ ] I have updated the documentation accordingly.
[style-guide]: https://wiki.apache.org/jmeter/CodeStyleGuidelines
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1264577640
Would you please delete `/src/licenses/licenses/svgSalamander` directory as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1534883293
> 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?
<img width="1066" alt="jmeter focus issues" src="https://user-images.githubusercontent.com/213894/236233889-6fd08c2d-777d-44f1-8826-47d8d63a92ef.png">
>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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1595117111
@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.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] codecov-commenter commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1264136677
# [Codecov](https://codecov.io/gh/apache/jmeter/pull/5715?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5715](https://codecov.io/gh/apache/jmeter/pull/5715?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c7c743d) into [master](https://codecov.io/gh/apache/jmeter/commit/4caddd8d33364f3bb0f6447f5e3409d6db4a6a35?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4caddd8) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/jmeter/pull/5715?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #5715 +/- ##
=========================================
Coverage 55.24% 55.24%
- Complexity 10387 10388 +1
=========================================
Files 1062 1062
Lines 65777 65776 -1
Branches 7536 7536
=========================================
+ Hits 36339 36341 +2
+ Misses 26838 26836 -2
+ Partials 2600 2599 -1
```
| [Impacted Files](https://codecov.io/gh/apache/jmeter/pull/5715?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/main/java/org/apache/jmeter/SplashScreen.java](https://codecov.io/gh/apache/jmeter/pull/5715/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2NvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2ptZXRlci9TcGxhc2hTY3JlZW4uamF2YQ==) | `58.97% <ø> (ø)` | |
| [...java/org/apache/jmeter/gui/util/JMeterToolBar.java](https://codecov.io/gh/apache/jmeter/pull/5715/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2NvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2ptZXRlci9ndWkvdXRpbC9KTWV0ZXJUb29sQmFyLmphdmE=) | `66.16% <ø> (ø)` | |
| [...g/apache/jmeter/gui/action/LookAndFeelCommand.java](https://codecov.io/gh/apache/jmeter/pull/5715/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2NvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2ptZXRlci9ndWkvYWN0aW9uL0xvb2tBbmRGZWVsQ29tbWFuZC5qYXZh) | `48.86% <100.00%> (+0.54%)` | :arrow_up: |
| [...isualizers/backend/influxdb/HttpMetricsSender.java](https://codecov.io/gh/apache/jmeter/pull/5715/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2NvbXBvbmVudHMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2ptZXRlci92aXN1YWxpemVycy9iYWNrZW5kL2luZmx1eGRiL0h0dHBNZXRyaWNzU2VuZGVyLmphdmE=) | `80.20% <0.00%> (+2.08%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/jmeter/pull/5715?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/jmeter/pull/5715?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4caddd8...c7c743d](https://codecov.io/gh/apache/jmeter/pull/5715?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] weisJ commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
weisJ commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1264342946
Yes it will still work. Besides the changes in packacke 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1534856564
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1265070670
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1528717280
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:
<img width="627" alt="tiny buttons on save confirmation dialog" src="https://user-images.githubusercontent.com/213894/235292895-117e7d43-1d54-46ba-a09c-be68df8a585d.png">
@weisJ , I have not checked what could be the reason, however, does this behaviour ring a bell for you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1295788720
@weisJ , just wondering, is this ready to go re darklaf?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] weisJ commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "weisJ (via GitHub)" <gi...@apache.org>.
weisJ commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1529149652
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] weisJ commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
weisJ commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1264599549
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "vlsi (via GitHub)" <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1594652698
@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(..)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] vlsi commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
vlsi commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1264331353
Nice, thank you.
Well, I used SVG icon for the splash screen in https://github.com/apache/jmeter/blob/4caddd8d33364f3bb0f6447f5e3409d6db4a6a35/src/core/src/main/java/org/apache/jmeter/SplashScreen.java#L78
Would it still work without svgSalamander?
If that works, I guess we can remove svgSalamander dependency, and its license override.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] weisJ commented on pull request #5715: Update darklaf to version 3.0.2
Posted by GitBox <gi...@apache.org>.
weisJ commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1304327759
All fixes I’m currently working on only regard some erroneous warning messages or the svg implementation. Both don’t apply to JMeter, so the current darklaf version should be good to go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] weisJ commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "weisJ (via GitHub)" <gi...@apache.org>.
weisJ commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1529139958
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jmeter] weisJ commented on pull request #5715: Update darklaf to version 3.0.2
Posted by "weisJ (via GitHub)" <gi...@apache.org>.
weisJ commented on PR #5715:
URL: https://github.com/apache/jmeter/pull/5715#issuecomment-1535307918
> Could you please clarify which classloader do you mean?
I am referring to the `DynamicClassLoader` here
https://github.com/apache/jmeter/blob/b0692252777dbf473b713fe40b38a1d3dfdc3b9b/src/launcher/src/main/java/org/apache/jmeter/NewDriver.java#L55
> 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 https://github.com/mikaelgrev/miglayout/pull/94 which should fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jmeter.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org