[FIXED JENKINS-38960] Deprecate getIconFilePathPattern and switch to IconSpec #2590

stephenc · 2016-10-13T11:44:09Z

@jenkinsci/code-reviewers @reviewbybees

/cc @michaelneale I'm wondering if BO is using the now deprecated information

/cc @recena You added the original method, I cannot find in jelly where it is used... and hints/tips as to where else I need to apply fix-up (already fixing some of the SCM-API usage)

…IconSpec

ghost · 2016-10-13T12:32:23Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

…entially - Such a pity that we don't auto-populate the CSS with the image sources for all the icons

stephenc · 2016-10-13T13:28:05Z

Here is a screenshot showing the New Item page after the change:

Points to note:

The Freestyle Project icon has the css style icon-freestyle-project which will enable theme plugins to override the icon via CSS
The icon src url is using the correct static/*hash*/ prefix to ensure that image resources are cached correctly
The Pipeline descriptor does not have an Icon registered, so we fall back to legacy rendering using the iconFilePathPattern

rsandell · 2016-10-13T13:34:57Z

🐝

stephenc · 2016-10-14T14:20:11Z

Test failure is unrelated @reviewbybees done

oleg-nenashev

IMHO it requires some tests of the FreestyleProject logic at least

oleg-nenashev · 2016-10-15T15:43:28Z

core/src/main/java/hudson/model/TopLevelItemDescriptor.java

@@ -189,6 +192,7 @@ public String getCategoryId() {
     * @return A string or null if it is not defined.
     *
     * @since 2.0
+     * @deprecated prefer {@link #getIconClassName()}


🐜 Also must have the @Deprecated annotation

oleg-nenashev · 2016-10-15T15:43:43Z

core/src/main/java/hudson/model/TopLevelItemDescriptor.java

@@ -203,6 +207,7 @@ public String getIconFilePathPattern() {
     * @return A string or null if it is not defined.
     *
     * @since 2.0
+     * @deprecated prefer {@link #getIconClassName()}


stephenc · 2016-10-17T08:55:17Z

@oleg-nenashev I have added the annotations.

w.r.t. adding a test case, I do not think that it should block merging this:

There are no tests for the add-item.js code in the first place, there should be a separate issue to add tests for that javascript code, blocking this issue on creating a whole test framework is invalid IMHO
There changes to Freestyle project are just returning a string constant and registering some icons. I see no value in spending time writing tests for code that obviously behaves as written and does not contain any logic.
The logic in TopLevelItemDescriptor's default implementation of getIconClassName() is essentially a lookup that falls back to null on failure. Either it gets a match or it doesn't. If it gets a match then the matching icon will be displayed (which essentially is adding a layer of indirection onto the icon path... but has the side-effect that the icon gets a css class to allow for themes). To my reading of the code, it is trivial and does not merit adding another test that starts up a Jenkins instance (because you need a Jenkins instance to test the logic). Plugins should not be relying on the default implementation, rather they should be overriding and returning a string constant. I do not see this code as needing a test, especially when none of the other code in TopLevelItemDescriptor has tests. Create a JIRA to create some tests for TopLevelItemDescriptor if you want more tests there, but blocking this PR on that is not appropriate
The logic in View.doItemCategories() is trivial and does not include any significant branches.

stephenc · 2016-10-18T17:47:25Z

@oleg-nenashev you are currently blocking

recena · 2016-10-19T10:25:44Z

👎 I cannot be agreed. I prefer the current way to describe a graphical resource. Please, find here other example.

recena · 2016-10-19T10:29:45Z

TopLevelItemDescriptor should not specify how the graphical resource will be rendered. Only what type of graphical resource it wants to use.

stephenc · 2016-10-19T11:42:10Z

@recena I will create a PR against the Maven plugin so. This pattern stuff should never have been done that way. The whole point of using the IconSet stuff was to remove the reliance on hardcoded paths... nevermind that the paths you are generating with that way are not cachable by the browser.

recena · 2016-10-19T11:47:54Z

Well, you should create a PR for several plugins, the same way as I did.

This pattern stuff should never have been done that way.

Well I have a different PoV.

nevermind that the paths you are generating with that way are not cachable by the browser.

The information generated by View.doItemCategories() is exposed through a JSON.

🐛 for deprecating methods with few months of live

stephenc · 2016-10-19T12:21:08Z

@recena the method you chosen should never have been exposed in the first place. It basically broke the entire reason for existence of the icon module.

I might disagree with how the Icon module does things, but the Icon module is the way we are supposed to do things until somebody files changes against it to switch to another way.

I have been filing PRs against all the plugins that I find, e.g. jenkinsci/maven-plugin#84, jenkinsci/workflow-multibranch-plugin#38, etc. Additionally, any code that uses the old way will continue to work. Plugins only need to implement IconSpec and add the method in order for their Icon to be used, if they don't then we'll fall back to the non-icon spec way.

Have you a personal interest in the method that I am deprecating?

recena · 2016-10-19T14:09:07Z

@stephenc As I said before...

the method you chosen should never have been exposed in the first place.

This is your opinion. Not mine.

recena · 2016-10-19T14:12:09Z

@stephenc What do you think if we include getIconFilePathPattern() or something like that on IconSpec?

stephenc · 2016-10-19T14:48:33Z

@recena that is a question you should direct at the icon-module maintainer. Please stop blocking this PR because you have issues with the icon-module. At this point in time the icon module is the way that icons are supposed to be referenced. This PR fixes the issue

stephenc · 2016-10-19T14:49:42Z

@recena I will say that an getIconFilePathPattern() method prevents the theme plugins from replacing the icons with their own variants, which as I understand it, would basically be a veto on your proposal... but see what the icon-module maintainer says

recena · 2016-10-20T12:07:54Z

I'm totally disagree with the idea behind of this PR but it seems clear it is very difficult changing some things in Jenkins.

recena · 2016-10-20T12:08:44Z

🐝 but I'll continue with 👎

stephenc · 2016-10-20T12:09:09Z

@reviewbybees done

recena · 2016-10-20T12:11:38Z

@stephenc These statements (assertive) do not help to change or to improvement.

stephenc · 2016-10-20T12:17:10Z

@recena we may both have objections to IconSpec and its friends, but a decision was taken that IconSpec is the "one true way" to handle icons as it enables a consistent API for theme plugins to reskin...

But until that decision has been reversed by the community/governance board, that decision stands. While that decision stands, code that does not follow IconSpec is a bug and needs to be fixed.

I encourage you to petition the community to reverse the IconSpec stuff and I will join you in that effort... but until the community reverses the IconSpec decision, we need to fix the bugs like this one... just as I was overruled on the @CheckForNull annotation consolidation... we need not be happy about the change making things "worse" from our personal perspective... but this change makes all the icons at least follow the current IconSpec way... which is the same argument that was given to me on the @CheckForNull annotation consolidation

oleg-nenashev · 2016-10-20T13:32:31Z

👎 for merging without waiting for a response from people, who requested changes and bugged the PR

oleg-nenashev · 2016-10-20T13:34:28Z

There was a negative community feedback, which has not been outvoted by @jenkinsci/code-reviewers. IMHO it's a violation of the community process, which may lead to CoC compliance discussion in such way. Please do not do it

oleg-nenashev · 2016-10-20T13:36:28Z

@recena @jenkinsci/code-reviewers Should this PR be reverted? Or is there a chance to get an agreement before the weekly?

stephenc added 2 commits October 13, 2016 12:39

[FIXED JENKINS-38960] Deprecate getIconFilePathPattern and switch to …

bd834fe

…IconSpec

Fix javadoc

780fdf1

[JENKINS-38960] The new item page should use the IconClassName prefer…

4a97222

…entially - Such a pity that we don't auto-populate the CSS with the image sources for all the icons

oleg-nenashev requested changes Oct 15, 2016

View reviewed changes

oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging needs-testcase Test automation is required for this pull request labels Oct 15, 2016

[JENKINS-38960] Forgot to mark methods as @Deprecated as well

95781a6

stephenc removed needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging needs-testcase Test automation is required for this pull request labels Oct 17, 2016

stephenc merged commit 764c49f into jenkinsci:master Oct 20, 2016

stephenc deleted the jenkins-38960 branch October 20, 2016 12:18

oleg-nenashev added a commit that referenced this pull request Oct 23, 2016

Changelog: Noting#2595, #2492, #2590 and #2586

12236a8

stephenc removed the needs-more-reviews Complex change, which would benefit from more eyes label Nov 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED JENKINS-38960] Deprecate getIconFilePathPattern and switch to IconSpec #2590

[FIXED JENKINS-38960] Deprecate getIconFilePathPattern and switch to IconSpec #2590

stephenc commented Oct 13, 2016

ghost commented Oct 13, 2016

stephenc commented Oct 13, 2016

rsandell commented Oct 13, 2016

stephenc commented Oct 14, 2016

oleg-nenashev left a comment

oleg-nenashev Oct 15, 2016

oleg-nenashev Oct 15, 2016

stephenc commented Oct 17, 2016

stephenc commented Oct 18, 2016

recena commented Oct 19, 2016

recena commented Oct 19, 2016

stephenc commented Oct 19, 2016

recena commented Oct 19, 2016 •

edited

Loading

stephenc commented Oct 19, 2016

recena commented Oct 19, 2016

recena commented Oct 19, 2016

stephenc commented Oct 19, 2016

stephenc commented Oct 19, 2016

recena commented Oct 20, 2016

recena commented Oct 20, 2016

stephenc commented Oct 20, 2016

recena commented Oct 20, 2016

stephenc commented Oct 20, 2016

oleg-nenashev commented Oct 20, 2016

oleg-nenashev commented Oct 20, 2016 •

edited

Loading

oleg-nenashev commented Oct 20, 2016

[FIXED JENKINS-38960] Deprecate getIconFilePathPattern and switch to IconSpec #2590

[FIXED JENKINS-38960] Deprecate getIconFilePathPattern and switch to IconSpec #2590

Conversation

stephenc commented Oct 13, 2016

ghost commented Oct 13, 2016

stephenc commented Oct 13, 2016

rsandell commented Oct 13, 2016

stephenc commented Oct 14, 2016

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev Oct 15, 2016

Choose a reason for hiding this comment

oleg-nenashev Oct 15, 2016

Choose a reason for hiding this comment

stephenc commented Oct 17, 2016

stephenc commented Oct 18, 2016

recena commented Oct 19, 2016

recena commented Oct 19, 2016

stephenc commented Oct 19, 2016

recena commented Oct 19, 2016 • edited Loading

stephenc commented Oct 19, 2016

recena commented Oct 19, 2016

recena commented Oct 19, 2016

stephenc commented Oct 19, 2016

stephenc commented Oct 19, 2016

recena commented Oct 20, 2016

recena commented Oct 20, 2016

stephenc commented Oct 20, 2016

recena commented Oct 20, 2016

stephenc commented Oct 20, 2016

oleg-nenashev commented Oct 20, 2016

oleg-nenashev commented Oct 20, 2016 • edited Loading

oleg-nenashev commented Oct 20, 2016

recena commented Oct 19, 2016 •

edited

Loading

oleg-nenashev commented Oct 20, 2016 •

edited

Loading