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

Allow customization of Job icons #9723

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

strangelookingnerd
Copy link
Contributor

@strangelookingnerd strangelookingnerd commented Sep 12, 2024

In a discussion with @jonesbusy in jenkinsci/custom-folder-icon-plugin#387 I stumbled upon custom-job-icon-plugin which by now has become deprecated / out-dated but brought up the interesting idea of being able to change the default icon of a Job from the BallColor to something else similar as custom-folder-icon-plugin already does it for AbstractFolder.

Since I really liked the idea and already have a use case myself I wanted to bring this to the core to lay the foundation.

This PR aims to introduce configurable icons for jobs and potentially also other types of items.

The first step is to add a new column to show the icon, similar to the status or weather icons. By default the column uses the icon and description describing the "type" of an item.

Screenshot:

image

The second step is to add some sort of configuration to Job that allows customization (tbd. / work in progress).

Full disclaimer, I am aware that this draft is still a little rough on the edges how ever to see if this goes anywhere I'd be grateful for some feedback.

Testing done

Only manual testing so far, tests to follow in case we want to follow up on this draft.

Proposed changelog entries

tbd.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

N/A

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

Maintainer checklist

@jonesbusy
Copy link
Contributor

Thanks! I really like the idea!

@timja timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise labels Sep 13, 2024
@timja timja requested a review from a team September 13, 2024 08:26
@strangelookingnerd strangelookingnerd force-pushed the feature/configurable_job_icon branch 2 times, most recently from 541cda9 to 6cfa2a5 Compare September 17, 2024 14:43
@strangelookingnerd strangelookingnerd marked this pull request as ready for review September 17, 2024 14:43
@strangelookingnerd strangelookingnerd force-pushed the feature/configurable_job_icon branch 3 times, most recently from 02f018a to dbbce1f Compare September 23, 2024 06:07
@daniel-beck daniel-beck added the needs-security-review Awaiting review by a security team member label Sep 24, 2024
@daniel-beck daniel-beck self-requested a review September 24, 2024 12:00
@daniel-beck
Copy link
Member

The complication here is that this is where jobs show their status – hence the "S" in the column title. While it seems useful to have a "type" indicator for jobs (e.g., using the icons from the "New Item" dialog) that extends beyond Folders (an item type with no natural status, at least not before ComputedFolder), and have that be customizable, this removes the build status information from view.

For that reason I am against this proposal as implemented. It seems to me that, to properly support this in a way that makes sense, we'd need another column as job type indicator first (with corresponding adaptation of the Folders plugin behavior IMO).

@strangelookingnerd
Copy link
Contributor Author

The complication here is that this is where jobs show their status – hence the "S" in the column title. While it seems useful to have a "type" indicator for jobs (e.g., using the icons from the "New Item" dialog) that extends beyond Folders (an item type with no natural status, at least not before ComputedFolder), and have that be customizable, this removes the build status information from view.

For that reason I am against this proposal as implemented. It seems to me that, to properly support this in a way that makes sense, we'd need another column as job type indicator first (with corresponding adaptation of the Folders plugin behavior IMO).

I mean, we could have Item or AbstractItem or TopLevelItem (?) have a icon field and this way also enable all sub-classes to inherit the feature. This would make large portions of the Folder implementation around icons obsolete I guess. The default implementation could simply use the icon from the "New Item" dialog and stand for the "type" as you already mentioned.

One thing I kind of disagree with is calling it "type" because that again - as the status - has an implication to it.
For example there is https://plugins.jenkins.io/custom-folder-icon/#plugin-content-build-status-folder-icon - a build status for Folders. Simply calling it "icon" would be sufficient in my opinion.

@daniel-beck
Copy link
Member

For example there is https://plugins.jenkins.io/custom-folder-icon/#plugin-content-build-status-folder-icon - a build status for Folders. Simply calling it "icon" would be sufficient in my opinion.

Right, the same problem applies to current organization folders, like e.g. on https://ci.jenkins.io/ that use the "Status" column for the organization icon. If we end up splitting the current status (except Folder with fixed icon) column into a "always status" and "always icon" column, that involves looking into how to adapt Folders accordingly.

@strangelookingnerd
Copy link
Contributor Author

So the first step would be to add a new column IconColumn in core views similar to StatusColumn.
Now the question is, what should be displayed there and where does it come from?

As said before, I'd imagine adding a icon field to AbstractItem. It could be based on org.jenkins.ui.icon.Icon and provided by an interface so other comsumers could opt-in e.g. SCMNavigator
With a proper default there would be no need to change anything on the implementing classes if they don't want to.

WDYT?

@daniel-beck
Copy link
Member

@strangelookingnerd That would be my suggestion, with the default being the item type icon from the New Item dialog. Obviously, that's just me and others may well see this differently (including future me once I've seen a prototype 😬 ).

@strangelookingnerd
Copy link
Contributor Author

I am sure that there will be some back an forth but I will try and look into creating a prototype and we see where that brings us. Setting this back to Draft for the time being.

@strangelookingnerd strangelookingnerd marked this pull request as draft September 25, 2024 19:26
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 12, 2024
Copy link
Contributor

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

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 12, 2024
@strangelookingnerd
Copy link
Contributor Author

strangelookingnerd commented Oct 13, 2024

@daniel-beck I updated the PR and implemented the first step towards a polished solution. Please let me know what you think.

I also had a go at adding a configuration for items in order to allow customization of icons that also enable providers for the icons. I may need a little more time to contemplate how to best solve it.

@daniel-beck
Copy link
Member

@strangelookingnerd Thanks, I'll try to look at this soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-security-review Awaiting review by a security team member rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants