[JENKINS-68250] Icon defaults to help.png, which was deleted in Jenkins 2.333

KalleOlaviNiemitalo commented 2 years ago

This issue is also tracked as JENKINS-68250.

Jenkins and plugins versions report

I have not reproduced this bug yet, but I think it will occur when Sidebar Link 2.1.0 is used with Jenkins 2.333 or later.

What Operating System are you using (both controller, and any agents involved in the problem)?

Any

Reproduction steps

Define a sidebar link in the global configuration, without specifying an icon.
Save the settings.
View the sidebar on the main page.

Expected Results

The sidebar link should have a default icon.

Actual Results

Icon won't load.

Anything else?

PR https://github.com/jenkinsci/sidebar-link-plugin/pull/25 made LinkAction use "static/efbf17e4/images/16x16/help.png" as the default icon. As of Sidebar Link 2.1.0, LinkAction still does so:

https://github.com/jenkinsci/sidebar-link-plugin/blob/fa77f63045894732918b5ade79bc867e429897bd/src/main/java/hudson/plugins/sidebar_link/LinkAction.java#L67-L69

However, https://github.com/jenkinsci/jenkins/pull/5778 deleted war/src/main/webapp/images/16x16/help.png from the Jenkins core. This change was released in Jenkins 2.333 (2022-01-31). The icon has been replaced with war/src/main/webapp/images/svgs/help.svg, which was moved to that directory in https://github.com/jenkinsci/jenkins/pull/5663 and released in Jenkins 2.308 (2021-08-24).

There is a migration guide in Icon path to icon class migration but it looks a bit difficult to apply in the Sidebar Link plugin. I think the best fix is to save an empty string as the icon file name in the persistent configuration and substitute a version-specific default when the link is rendered. This would require separating LinkAction.getIconFileName() to two methods: one that implements Action.getIconFileName() and another that is used for the configuration UI and CasC. I hope @Symbol can be used to retain compatibility with existing CasC jenkins.yaml files.

Alternatively, Sidebar Link could ship with its own copy of help.svg and always default to that.

KalleOlaviNiemitalo commented 2 years ago

I think the best fix is to save an empty string as the icon file name in the persistent configuration and substitute a version-specific default when the link is rendered.

This kind of render-time default would also help implement JENKINS-28830.

KalleOlaviNiemitalo commented 2 years ago

This would require separating LinkAction.getIconFileName() to two methods

Like there is LinkAction.getUrlName() for the Action interface and LinkAction.getUnprotectedUrlName() for the text box in links.jelly.

KalleOlaviNiemitalo commented 2 years ago

I hope @Symbol can be used to retain compatibility

Nope, @interface Symbol has @Target({TYPE}), so LinkAction cannot define @Symbol("iconFileName") public String getIconFileNameWithoutDefault()to make CasC call this method when it exports the current configuration to YAML. Instead, sidebar-link-plugin could perhaps implement Configurator for that purpose.

I get the feeling that the thing being configured in the UI and in CasC should not be a LinkAction itself but rather an instance of a separate class that would generate a LinkAction when requested. That way, it would be able to name its methods to match the property names used in CasC YAML files, without conflicting with how those names are used in LinkAction and in interface Action. But perhaps it is not possible to make such a change without breaking compatibility again.

KalleOlaviNiemitalo commented 2 years ago

I tried setting just "help" as the icon name, without any directory path or file extension, and Jenkins 2.319.3 rendered a question mark icon as SVG.

    <hudson.plugins.sidebar__link.LinkAction>
      <url>http://example.org/</url>
      <text>Example</text>
      <icon>help</icon>
    </hudson.plugins.sidebar__link.LinkAction>

<div class="task "><span class="task-link-wrapper "><a href="http://example.org/" title="Example" class="task-link "><span class="task-icon-link"><svg viewBox="0 0 24 24" focusable="false" class="svg-icon icon-help icon-md"><use href="/jenkins/static/1bfb45e3/images/material-icons/svg-sprite-action-symbol.svg#ic_help_24px"></use></svg></span><span class="task-link-text">Example</span></a></span></div>

That might work as a default icon, then. However:

I did not test with older or newer versions of Jenkins.
That this works at all seems to be a side effect of how actions.jelly chooses the icon. Icon names that are not file names should apparently be returned from IconSpec.getIconClassName() and not from Action.getIconFileName().
The form validation in the sidebar link configuration displayed an ugly warning about "Image does not exist". On the other hand, that warning would be avoided if the "help" default applied only during rendering and were never displayed on the configuration page.

Perhaps Sidebar Link could use something like org.jenkins.ui.icon.IconSet.icons.getIconByClassSpec("icon-help icon-md").getUrl() to get a default path for Action.getIconFileName(). That would depend on https://github.com/jenkinsci/jenkins/pull/5072 in Jenkins 2.269, I think. It seems easier to ship a copy of help.svg in the sidebar link plugin. Or even switch the default to a different icon altogether.

KalleOlaviNiemitalo commented 2 years ago

A comment in JENKINS-67844 mentions custom symbols intended for the sidebar. Of the existing symbols, help-circle.svg is most similar to the current help icon. However, I'm not sure a help icon is even appropriate as the default icon. Plugins cannot install custom symbols yet.

https://github.com/jenkinsci/jenkins/pull/6186 in Jenkins 2.335 adds support for symbols in Action.getIconFileName(), but that feature is not in any LTS release of Jenkins yet. It might be possible for sidebar-link-plugin to detect support at run time but that would just make it more complex to maintain.

KalleOlaviNiemitalo commented 2 years ago

@NotMyFault, you have been replacing icons in other plugins for Jenkins. Can you recommend what to do here? To summarize:

The Sidebar Link plugin lets the user type an icon file name for each link, and saves that string in within a GlobalConfiguration or NodeProperty or FolderProperty or JobProperty.
If the user does leaves the icon file name empty, then it defaults to "static/efbf17e4/images/16x16/help.png", and the plugin saves that. However, help.png was deleted in Jenkins 2.333.
The plugin defines a LinkAction class that implements Action.getIconFile() by returning the previously saved string. That is indirectly called from actions.jelly in core, perhaps from elsewhere too.
pom.xml currently sets 2.263.4 as jenkins.version. That version has war/images/help.svg but I'm not sure it can be directly used as Action.getIconFile().

Should the Sidebar Link plugin reference an icon provided by Jenkins, or should it carry its own icon?

If using an icon provided by Jenkins, then should that be done by returning a specific string from Action.getIconFile(), or by taking over with a LinkAction/action.jelly file?

The plugin should also be changed to stop saving the default icon file name as part of the per-link settings, but I think I'll figure out how to do that on my own.

NotMyFault commented 2 years ago

@NotMyFault, you have been replacing icons in other plugins for Jenkins. Can you recommend what to do here?

You can ship your own default icon for sure, but I would go with bumping the baseline, because that is a change you definitely do in a foreseeable future, which does then defeat the addition of a temporary custom icon just to keep an old baseline, which already is more than two years old, in this case.

jenkinsci / sidebar-link-plugin