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

Introduce a new summary description field for jobs #4489

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 75 additions & 8 deletions core/src/main/java/hudson/model/Run.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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

Copy link

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

Copy link
Member

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.

@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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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();
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be un deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no, because getSummary is a proper API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would prefer to not change the state here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned it as this PR adds documentation for it:
https://github.com/jenkinsci/jenkins/pull/4489/files#diff-c4f9931d88bca347279b881007d71f0eR234

Probably shouldn't be suggesting to call a deprecated API?

Copy link
Member

Choose a reason for hiding this comment

The 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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be stylistically better to do:

@CheckForNull
public String ...

Copy link
Member Author

Choose a reason for hiding this comment

The 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 == '>') {
Expand All @@ -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)
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.getString("summary"));
}

public static final XStream XSTREAM = new XStream2();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,10 @@ THE SOFTWARE.
</div>
</j:if>
</div>
<j:if test="${!empty build.description}">
<j:set var="buildSummary" value="${build.summary}"/>
<j:if test="${!empty buildSummary}">
<div class="pane desc indent-multiline">
<j:out value="${app.markupFormatter.translate(build.description)}"/>
<j:out value="${app.markupFormatter.translate(buildSummary)}"/>
</div>
</j:if>
<div class="left-bar" />
Expand Down