Use display name in directory browser breadcrumbs#25924
Use display name in directory browser breadcrumbs#25924MarkEWaite merged 4 commits intojenkinsci:masterfrom
Conversation
|
Please include before and after screenshots for any ui change |
daniel-beck
left a comment
There was a problem hiding this comment.
Please remove unrelated changes from this PR.
696a163 to
c31574e
Compare
|
Thanks for pointing it out! I’ve added the UI screenshots and will include before/after images in future PRs. |
|
you've only added after and the text before |
|
I’ve added the before screenshot as well. |
| <form> | ||
| <a href="${topPath}">${it.owner.name}</a> / | ||
| <j:choose> | ||
| <j:when test="${request2.originalRequestURI.contains('/artifact/')}"> |
There was a problem hiding this comment.
This makes absolutely no sense.
If a job is called artifact, so the URL is /job/artifact/whatever, we go into this branch?
Also, why would we not do this for workspaces?
| <a href="${rootURL}/${it.owner.url}artifact/">${%Root}</a> / | ||
| </j:when> | ||
| <j:otherwise> | ||
| <a href="${topPath}">${it.owner.name}</a> / |
There was a problem hiding this comment.
Actually a link is already added. It's just that it.owner.name return nothing or is null, hence the link is not usable. So all that needs to be done is checking if it.owner.name is null or empty and then insert ${%Root} instead.
| <a href="${rootURL}/${it.owner.url}artifact/">${%Root}</a> / | ||
| </j:when> | ||
| <j:otherwise> | ||
| <a href="${topPath}">${it.owner.name}</a> / |
There was a problem hiding this comment.
Or as the owner is a ModelObject use it.owner.displayName as that is the only method that a ModelObject has.
Things you should check:
- visit the workspace of a FreestyleProject
- visit the artifacts attached from the job directly (uses
lastSuccessfulBuild) - visit the artifacts attached from the run itself
- visit the workspace(s) of a pipeline run
|
Thanks @daniel-beck and @mawinter69 for the feedback. I see the issue with checking request2.originalRequestURI — it would indeed break if a job was named "artifact". Based on @mawinter69's suggestion, I'll update the approach to avoid the URL check entirely. Instead, I will check if it.owner.name is null or empty. I will also verify this behavior across: I'll push the changes shortly. |
|
We removed the URL check entirely. Instead, we updated the logic in dir.jelly to: 1 :- Check if it.owner.name is empty. Is Further Work Required? |
mawinter69
left a comment
There was a problem hiding this comment.
Please also update screenshots
| <form> | ||
| <a href="${topPath}">${it.owner.name}</a> / | ||
| <a href="${topPath}"> | ||
| <j:choose> |
There was a problem hiding this comment.
Just always use it.owner.displayname without a choose. No need to check for name alone, when you don't use it anyway.
|
I had changed the code as per the requirement and also added the latest screenshot @daniel-beck @mawinter69
|
|
@daniel-beck @mawinter69 any update in the PR . Do this required any further change or not ? |
|
Hi @daniel-beck , @timja and @mawinter69, I’ve now:
All checks are passing, and the branch is ready to merge once approved. Could you please let me know if any further changes are needed, or if this is ready for final review? |
|
Can you update the screenshots and the original PR description at the top to reflect the current state. It doesn't fit anymore to what was changed and what was tested |
mawinter69
left a comment
There was a problem hiding this comment.
Otherwise it looks good I think
|
Thanks for the suggestion! |
timja
left a comment
There was a problem hiding this comment.
/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!
|
Please restore the PR template making sure a changelog entry is included: |
|
@timja just following up on this PR since it is already approved. Let me know if anything else is needed from my side before merging. |
|
I had changed the PR description . |
Not until 15 minutes ago. I hope you've just mistaken had and have. |
|
/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! |
Sorry for the grammatical mistake. I updated the PR description a few minutes ago . |
MarkEWaite
left a comment
There was a problem hiding this comment.
I tested interactively and found no issues. Thanks!
* Update dir.jelly logic for navigation * Fix the navigation * Change as per requirement * Change jelly


Fixes #16765
Testing done
Verified locally. The breadcrumb now correctly shows the display name instead of the internal name, which looks much better for jobs with custom display names.
UI Screenshots
After the change
Before the change
Proposed changelog entries
Proposed changelog category
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@timja @mawinter69 @daniel-beck @MarkEWaite