-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
Introduce a new summary description field for jobs #4489
Changes from all commits
f3313d2
a1e34c9
9bf0605
8161545
1003cc6
523d296
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,6 +157,14 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run | |
*/ | ||
public static final long QUEUE_ID_UNKNOWN = -1; | ||
|
||
/** | ||
* Target size limit for truncated {@link #description}s in the Build History Widget. | ||
* Negative values will disable truncation, {@code 0} will enforce empty strings. | ||
* The value has no effect if a new {@link #summary} field is defined. | ||
* @since TODO | ||
*/ | ||
private static int TRUNCATED_DESCRIPTION_LIMIT = SystemProperties.getInteger("historyWidget.descriptionLimit", 100); | ||
|
||
protected transient final @Nonnull JobT project; | ||
|
||
/** | ||
|
@@ -221,10 +229,22 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run | |
protected volatile Result result; | ||
|
||
/** | ||
* Human-readable description. Can be null. | ||
* Human-readable description which is used on the main build page. | ||
* It can also be quite long, and it may use markup in a format defined by a {@link hudson.markup.MarkupFormatter}. | ||
* {@link #getTruncatedDescription()} may be used to retrieve a size-limited description, | ||
* but it implies some limitations. | ||
*/ | ||
@CheckForNull | ||
protected volatile String description; | ||
|
||
/** | ||
* A short human-readable summary which is used on the main page, build history widgets and other locations. | ||
* It may use markup in a format defined by the {@link hudson.markup.MarkupFormatter}. | ||
* The goal for this field is to keep the formatted text below the {@link #TRUNCATED_DESCRIPTION_LIMIT} symbols limit. | ||
*/ | ||
@CheckForNull | ||
protected volatile String summary; | ||
|
||
/** | ||
* Human-readable name of this build. Can be null. | ||
* If non-null, this text is displayed instead of "#NNN", which is the default. | ||
|
@@ -659,33 +679,56 @@ public final long getStartTimeInMillis() { | |
return startTime; | ||
} | ||
|
||
//TODO(oleg_nenashev): Do we want to export full description by default taking the size considerations? Looks like it should be another REST API or a flag | ||
@Exported | ||
@CheckForNull | ||
public String getDescription() { | ||
return description; | ||
} | ||
|
||
/** | ||
* Gets summary of the run. | ||
* This is a formatted string, {@link hudson.markup.MarkupFormatter} should be applied to it in the WebUI. | ||
* If the {@link #summary} field is {@code null}, the method will also try to retrieve a truncated {@link #description}. | ||
* @since TODO | ||
*/ | ||
@Exported | ||
@CheckForNull | ||
public String getSummary() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it have a suppressed warning for the use of a deprecated method? |
||
return summary != null ? summary : getTruncatedDescription(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you need to check if it's empty, or will it always be safely null here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer a strict data model here (good for getter performance), but I can add String truncation on the setter side |
||
} | ||
|
||
/** | ||
* Returns the length-limited description. | ||
* The method tries to take the HTML tags into account, but it is a best-effort attempt. | ||
* Also, the method will not work properly if a non-HTML {@link hudson.markup.MarkupFormatter} is used. | ||
* @return The length-limited description. | ||
* @deprecated truncated description uses arbitrary and unconfigurable limit of 100 symbols | ||
* @deprecated truncated description based on the {@link #TRUNCATED_DESCRIPTION_LIMIT} setting. | ||
*/ | ||
@Deprecated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be un deprecated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think no, because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I would prefer to not change the state here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mentioned it as this PR adds documentation for it: Probably shouldn't be suggesting to call a deprecated API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when compiling, we will have a warning because we are calling a deprecated api. I don't have solution, just worse mentioning that this is intentional, no? |
||
public @Nonnull String getTruncatedDescription() { | ||
final int maxDescrLength = 100; | ||
if (description == null || description.length() < maxDescrLength) { | ||
public @CheckForNull String getTruncatedDescription() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be stylistically better to do:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would. Just following the historical codestyle |
||
if (TRUNCATED_DESCRIPTION_LIMIT < 0) { // disabled | ||
return description; | ||
} | ||
if (TRUNCATED_DESCRIPTION_LIMIT == 0) { // Someone wants to suppress descriptions, why not? | ||
return ""; | ||
} | ||
|
||
final int maxDescrLength = TRUNCATED_DESCRIPTION_LIMIT; | ||
final String localDescription = description; | ||
if (localDescription == null || localDescription.length() < maxDescrLength) { | ||
return localDescription; | ||
} | ||
|
||
final String ending = "..."; | ||
final int sz = description.length(), maxTruncLength = maxDescrLength - ending.length(); | ||
final int sz = localDescription.length(), maxTruncLength = maxDescrLength - ending.length(); | ||
|
||
boolean inTag = false; | ||
int displayChars = 0; | ||
int lastTruncatablePoint = -1; | ||
|
||
for (int i=0; i<sz; i++) { | ||
char ch = description.charAt(i); | ||
char ch = localDescription.charAt(i); | ||
if(ch == '<') { | ||
inTag = true; | ||
} else if (ch == '>') { | ||
|
@@ -702,7 +745,7 @@ public String getDescription() { | |
} | ||
} | ||
|
||
String truncDesc = description; | ||
String truncDesc = localDescription; | ||
|
||
// Could not find a preferred truncatable index, force a trunc at maxTruncLength | ||
if (lastTruncatablePoint == -1) | ||
|
@@ -2330,6 +2373,18 @@ public void setDescription(String description) throws IOException { | |
this.description = description; | ||
save(); | ||
} | ||
|
||
/** | ||
* Sets a new summary | ||
* @param summary Summary to set. Use {@code null} to remove the summary. | ||
* @throws IOException Summary update error | ||
* @since TODO | ||
*/ | ||
public void setSummary(@CheckForNull String summary) throws IOException { | ||
checkPermission(UPDATE); | ||
this.summary = summary; | ||
save(); | ||
} | ||
|
||
/** | ||
* Accepts the new description. | ||
|
@@ -2340,6 +2395,17 @@ public synchronized void doSubmitDescription( StaplerRequest req, StaplerRespons | |
rsp.sendRedirect("."); // go to the top page | ||
} | ||
|
||
/** | ||
* Accepts the new summary. | ||
* @since TODO | ||
*/ | ||
@RequirePOST | ||
@Restricted(NoExternalUse.class) | ||
public synchronized void doSubmitSummary( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException { | ||
setSummary(req.getParameter("summary")); | ||
rsp.sendRedirect("."); // go to the top page | ||
} | ||
|
||
/** | ||
* @deprecated as of 1.292 | ||
* Use {@link #getEnvironment(TaskListener)} instead. | ||
|
@@ -2476,6 +2542,7 @@ public long getEstimatedDuration() { | |
protected void submit(JSONObject json) throws IOException { | ||
setDisplayName(Util.fixEmptyAndTrim(json.getString("displayName"))); | ||
setDescription(json.getString("description")); | ||
setSummary(json.optString("summary", null)); | ||
} | ||
|
||
public static final XStream XSTREAM = new XStream2(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether @harry-g and @jsoref use REST APIs for builds. The current state is a potentially big problem for long descriptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would anything change for REST API?
getDescription
always returned full text.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I do not touch it. But I believe that it is not ideal for users who are concerned about huge REST APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think we don't use it afaik.
But I would also discourage delivering the full description with the build object.
(Where it is only one build run, so less severe than the current job/build with/config page which load description from tons of builds.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just decrease visibility if you don't like it here. People should be requesting it via
?tree
anyway.More of a separate change though.