Expose `org.jenkins.ui.symbol.Symbol` #6659

Vlatombe · 2022-06-14T11:29:42Z

In some cases, plugins need to generate markup with symbols (for example when generating FormValidation with html markup). This is adding a way for them to build such markup.

This also improves coverage for this method.

Exposes the existing IconSet#getSymbol for plugins by adding a builder
pattern, moving it to its own package and class to separate the
Symbol framework from the Icon one.
Added coverage to the existing method including symbol lookup from
plugins

See JENKINS-XXXXX.

Proposed changelog entries

Developer: allow plugins to generate symbol markup from Java code.
...

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

* Exposes the existing IconSet#getSymbol for plugins by adding a builder pattern and moving it to its own package and class to separate the symbol framework from the Icon one. * Added coverage to the existing method including symbol lookup from plugins

test/src/test/java/org/jenkins/ui/symbol/SymbolJenkinsTest.java

timja

Nice! Needed something like this in credentials plugin.

There's a fork of IconSet basically. For the menu's, only way I got it working was calling the restricted code but I left it out of the change set in the end

NotMyFault

Nice one

daniel-beck · 2022-06-14T15:37:23Z

To minimize problems integrating next week's core security fixes into jenkinsci/master, I'm on-holding this PR.

daniel-beck · 2022-06-22T18:41:04Z

Un-on-holding since the release is now out.

core/src/main/java/org/jenkins/ui/symbol/Symbol.java

timja · 2022-06-23T14:15:25Z

core/src/main/java/org/jenkins/ui/symbol/Symbol.java

+                     .replaceAll("(class=\").*?(\")", "")
+                     .replaceAll("(tooltip=\").*?(\")", "")
+                     .replaceAll("(id=\").*?(\")", "")
+                     .replace("stroke:#000", "stroke:currentColor");


@NotMyFault might this be the root cause of this PR instead? #6686

replace instead of replaceAll?

The dot of the price tag has no stroke defined, this replacement pattern wouldn't apply at all.

replace replaces all, it's just not a regex. (Although the first arg seems to assume it's never #000000? Is that not a potential problem?)

I see that 3 core symbols have stroke:#000 (lock-closed.svg, play.svg and power.svg)

And it appears twice in lock-closed.svg and power.svg so this could be updated. Not sure what is best between just fixing the svg files compared to changing this code.

The dot of the price tag has no stroke defined, this replacement pattern wouldn't apply at all.

ah my bad.

it could be yes although I guess it's an assumption based on what ionicon's does

Looks like everything is fine so far. However having some logic to validate symbols at build time would be interesting.

What comes to my mind:

Check that each path has at least one of fill,stroke defined and set to either none or currentColor. Could be defined either through attribute or through style.

github-actions · 2022-07-15T22:43:56Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

Vlatombe · 2022-07-20T13:10:31Z

Could I get a review from @jenkinsci/core-security-review ? This has been hanging for a while now.

github-actions · 2022-09-02T16:43:48Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

…public

basil · 2022-09-02T17:34:44Z

As far as I can tell the only pending task left on this PR is a validation of the proposed new API with a consumer. We have identified one possible consumer, the Credentials plugin. Given Vincent's limited availability, we would welcome any community contributions to adapt the Credentials plugin to the new API proposed in this PR.

timja · 2022-09-05T22:14:56Z

I've had a look at the credentials plugin and I'm not sure if this API is useful for it.

It's after an Icon not a symbol.

and fixing the issue there with symbols in context menus not working will likely require more refactoring in core around context menus and symbols.

I can't see a way with the current API to make it work right now.

basil · 2022-09-13T15:17:45Z

I can't see a way with the current API to make it work right now.

Is that a use case that we would expect this API to work with? If so, that would be an indication to me that the API needs further refinement to support this use case before this PR can be merged.

timja · 2022-09-13T15:36:08Z

Is that a use case that we would expect this API to work with

Unsure, the credentials plugin context menu code is not easy to reason with. I or someone else will need more time in there to try unravel it.

I think this API is a net positive, it would make that sort of thing easier to implement in the future.

basil · 2022-09-13T15:40:12Z

Unsure

Fair enough. I don't think we should move forward with this PR until we become sure one way or another. Committing to an API that doesn't necessarily satisfy all use cases could be a liability in the future, since changing an API to accommodate new use cases while retaining compatibility once there are existing consumers is much harder (and in some cases impossible) compared to getting the design right at the outset.

core/src/test/java/org/jenkins/ui/symbol/SymbolTest.java

uhafner · 2022-09-23T18:26:52Z

test/src/test/java/org/jenkins/ui/symbol/SymbolJenkinsTest.java

+                                           .withName("science")
+                                           .withPluginName("missing-plugin")


This would not work out of the box in my use case. I have a given symbol as full referenced string with plugin included, e.g. "symbol-solid/triangle-exclamation plugin-font-awesome-api". I can split this manually before and after the space but it would be simpler if another builder method would be available that takes the whole string. What do you think, does this make sense?

I tried to use the new API in jenkinsci/prism-api-plugin#60. However, it seems that I only get a cross as icon. It would be really helpful if we can add some logging why an icon is not found :-(

It seems I need to strip the plugin- and symbol- prefixes as well.

Ok, I got it working now:

name = "symbol-solid/triangle-exclamation plugin-font-awesome-api"; String[] elements = StringUtils.split(name); if (elements.length == 2) { String symbol = Symbol.get(new SymbolRequest.Builder() .withName(StringUtils.removeStart(elements[0], "symbol-")) .withPluginName(StringUtils.removeStart(elements[1], "plugin-")) .withClasses(ICON_MD) .build()); return new UnescapedText(symbol); }

From my perspective it would be helpful, if there would be another builder method (something like withId or withIdentifier), that takes the whole symbol identifier and splits it accordingly.

uhafner · 2022-11-22T20:49:23Z

@Vlatombe are you planning to finish this PR in the near future?

Vlatombe · 2022-11-23T09:25:02Z

@uhafner I pushed a new method based on what you commented earlier. Let me know if it satisfies your use case.

github-actions · 2022-11-25T22:25:29Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

uhafner · 2022-11-26T21:37:14Z

@uhafner I pushed a new method based on what you commented earlier. Let me know if it satisfies your use case.

Yes, this is exactly what I need. It seems that the incremental has not been published so I cannot test, but from the code and test case it looks good!

uhafner · 2022-11-29T22:39:11Z

I tested the new implementation, it works as expected! (See https://github.com/jenkinsci/prism-api-plugin/blob/599a3307af0e4e6263db95cee1ed79f7ae541131/src/main/java/io/jenkins/plugins/prism/SourcePrinter.java#L105-L113)

🎉

timja · 2022-12-14T08:58:27Z

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

Vlatombe requested review from a team and janfaracik June 14, 2022 11:32

timja reviewed Jun 14, 2022

View reviewed changes

test/src/test/java/org/jenkins/ui/symbol/SymbolJenkinsTest.java Outdated Show resolved Hide resolved

test/src/test/java/org/jenkins/ui/symbol/SymbolJenkinsTest.java Outdated Show resolved Hide resolved

timja requested a review from a team June 14, 2022 14:33

timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Jun 14, 2022

timja approved these changes Jun 14, 2022

View reviewed changes

NotMyFault approved these changes Jun 14, 2022

View reviewed changes

daniel-beck added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Jun 14, 2022

daniel-beck removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Jun 22, 2022

daniel-beck reviewed Jun 22, 2022

View reviewed changes

core/src/main/java/org/jenkins/ui/symbol/Symbol.java Outdated Show resolved Hide resolved

Vlatombe added 3 commits June 23, 2022 14:03

Fixes JENKINS-68805 and pick up jenkinsci#6685

f3146da

Merge branch 'master' into make-symbol-helper-public

b4db987

Clean up

f92ccaa

daniel-beck reviewed Jun 23, 2022

View reviewed changes

core/src/main/java/org/jenkins/ui/symbol/Symbol.java Outdated Show resolved Hide resolved

Vlatombe added 3 commits June 23, 2022 14:25

Rename method since the intent slightly changed

a86eb39

Simplify

9c6d7ca

Merge branch 'master' into make-symbol-helper-public

52f184e

timja reviewed Jun 23, 2022

View reviewed changes

timja added the needs-security-review Awaiting review by a security team member label Jun 23, 2022

timja requested a review from a team June 23, 2022 21:29

NotMyFault added the web-ui The PR includes WebUI changes which may need special expertise label Jun 26, 2022

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 15, 2022

Merge branch 'master' into make-symbol-helper-public

676c4e8

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 20, 2022

Make this compile

828a1b0

Merge remote-tracking branch 'origin/master' into make-symbol-helper-…

b5570c0

…public

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 2, 2022

timja mentioned this pull request Sep 4, 2022

Modernize UI jenkinsci/credentials-plugin#364

Merged

3 tasks

uhafner mentioned this pull request Sep 22, 2022

Prepare source view for symbols in markers jenkinsci/prism-api-plugin#59

Merged

uhafner reviewed Sep 22, 2022

View reviewed changes

core/src/test/java/org/jenkins/ui/symbol/SymbolTest.java Show resolved Hide resolved

Add assertions for the symbol content used for testing

f41e7fd

uhafner reviewed Sep 23, 2022

View reviewed changes

uhafner mentioned this pull request Sep 23, 2022

Try to use new symbol API jenkinsci/prism-api-plugin#60

Merged

uhafner mentioned this pull request Nov 22, 2022

Use Font Awesome icons as symbols rather than loading them as sprites jenkinsci/font-awesome-api-plugin#187

Merged

3 tasks

Vlatombe added 3 commits November 23, 2022 09:22

Merge branch 'master' into make-symbol-helper-public

bcfe104

Read science svg from filesystem

695e7be

Add withRaw to parse a raw selector

1fa0f3f

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Nov 25, 2022

Merge branch 'master' into make-symbol-helper-public

5526933

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 14, 2022

timja approved these changes Dec 14, 2022

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 14, 2022

NotMyFault merged commit 53e59bb into jenkinsci:master Dec 20, 2022

Uh oh!

Expose org.jenkins.ui.symbol.Symbol #6659

Expose org.jenkins.ui.symbol.Symbol #6659

Uh oh!

Conversation

Vlatombe commented Jun 14, 2022 • edited by gitpod-io bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Uh oh!

Uh oh!

Uh oh!

timja left a comment

Choose a reason for hiding this comment

Uh oh!

NotMyFault left a comment

Choose a reason for hiding this comment

Uh oh!

daniel-beck commented Jun 14, 2022

Uh oh!

daniel-beck commented Jun 22, 2022

Uh oh!

Uh oh!

Uh oh!

timja Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotMyFault Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

daniel-beck Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vlatombe Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timja Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

Vlatombe Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 15, 2022

Uh oh!

Vlatombe commented Jul 20, 2022

Uh oh!

github-actions bot commented Sep 2, 2022

Uh oh!

basil commented Sep 2, 2022

Uh oh!

timja commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basil commented Sep 13, 2022

Uh oh!

timja commented Sep 13, 2022

Uh oh!

basil commented Sep 13, 2022

Uh oh!

Uh oh!

uhafner Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

uhafner Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhafner Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhafner commented Nov 22, 2022

Uh oh!

Expose `org.jenkins.ui.symbol.Symbol` #6659

Expose `org.jenkins.ui.symbol.Symbol` #6659

Vlatombe commented Jun 14, 2022 •

edited by gitpod-io bot

Loading

timja Jun 23, 2022 •

edited

Loading

daniel-beck Jun 23, 2022 •

edited

Loading

Vlatombe Jun 23, 2022 •

edited

Loading

Vlatombe Jun 23, 2022 •

edited

Loading

timja commented Sep 5, 2022 •

edited

Loading

uhafner Sep 23, 2022 •

edited

Loading

uhafner Sep 23, 2022 •

edited

Loading