Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
KalleOlaviNiemitalo opened this issue Feb 18, 2022 · 7 comments · Fixed by #43
Closed

Comments

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Feb 18, 2022

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

  1. Define a sidebar link in the global configuration, without specifying an icon.
  2. Save the settings.
  3. 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 #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:

if(StringUtils.isBlank(iconFileName)) {
iconFileName = "static/efbf17e4/images/16x16/help.png";
}

However, jenkinsci/jenkins#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 jenkinsci/jenkins#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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

KalleOlaviNiemitalo commented Feb 20, 2022

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
Copy link
Contributor Author

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 jenkinsci/jenkins#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
Copy link
Contributor Author

KalleOlaviNiemitalo commented Feb 23, 2022

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.

jenkinsci/jenkins#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
Copy link
Contributor Author

KalleOlaviNiemitalo commented Feb 24, 2022

@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
Copy link
Member

@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.

@KalleOlaviNiemitalo KalleOlaviNiemitalo changed the title Icon defaults to help.png, which was deleted in Jenkins 2.333 [JENKINS-68250] Icon defaults to help.png, which was deleted in Jenkins 2.333 Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants