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

[FIXED JENKINS-42443] Make f:select display spinner during AJAX requests #2771

Merged
merged 4 commits into from Mar 9, 2017

Conversation

@stephenc
Copy link
Member

commented Mar 2, 2017

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 2, 2017

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.

@stephenc stephenc force-pushed the stephenc:jenkins-42443 branch from 3e06368 to ed329d1 Mar 2, 2017

@daniel-beck
Copy link
Member

left a comment

This looks like something that could profit from tests, and maybe a ui-samples addition.

/*
* The MIT License
*
* Copyright (c) 2004-2009, Sun Microsystems, Inc.

This comment has been minimized.

Copy link
@daniel-beck
Behaviour.applySubtree(status);
// deleting values can result in the data loss, so let's not do that unless instructed
var header = rsp.getResponseHeader('X-Jenkins-Select-Error');
if (header && "clear".toUpperCase() === header.toUpperCase()) {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 2, 2017

Member

i18n issues possible?

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 2, 2017

Author Member

toLocale___Case is the variants that would have i18n issues.

But looking at this again, I can remove one call by just changing the header value to lowercase

}
Behaviour.applySubtree(status);
// deleting values can result in the data loss, so let's not do that unless instructed
var header = rsp.getResponseHeader('X-Jenkins-Select-Error');

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 2, 2017

Member

Is this HTTP2 compatible?

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 2, 2017

Author Member

XMLHttpRequest.getResponseHeader() is supposed to be case insensitive

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 3, 2017

Member

I'm fine with that. Once we start following HTTP2 rules, we will need to either make Stapler to do it for us OR to perform a bulk update in the core and plugins. I prefer the first option

This comment has been minimized.

Copy link
@jglick

jglick Mar 3, 2017

Member

I am probably being dumb here but what is the issue exactly?

import static hudson.Util.join;

/**
* Represents the result of the form field fill failure.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 2, 2017

Member

To clarify, FormFillFailure is to doFillWhatever as FormValidation is to doCheckWhatever? This didn't exist before?

@jglick
Copy link
Member

left a comment

I think you forgot to add select-ajax-pending to CSS.

I would encourage you to actually prototype rewriting jenkinsci/github-branch-source-plugin#125 using a snapshot baseline to verify that this works.

* <p>
* Use this with caution, so that anonymous users do not gain too much insights into the state of the system,
* as error stack trace often reveals a lot of information. Consider if a check operation needs to be exposed
* to everyone or just those who have higher access to job/hudson/etc.

This comment has been minimized.

Copy link
@jglick

jglick Mar 2, 2017

Member

Is this copied-and-pasted by any chance?

}

private static FormFillFailure _error(FormValidation.Kind kind, Throwable e, String message) {
if (e==null) return _errorWithMarkup(Util.escape(message),kind);

This comment has been minimized.

Copy link
@jglick

jglick Mar 2, 2017

Member

Clearly it is…

}
// 1x16 spacer needed for IE since it doesn't support min-height
return "<div class="+ getKind().name().toLowerCase(Locale.ENGLISH) +"><img src='"+
req.getContextPath()+ Jenkins.RESOURCE_PATH+"/images/none.gif' height=16 width=1>"+

This comment has been minimized.

Copy link
@jglick

jglick Mar 2, 2017

Member

Share code with FormValidation please.

return selectionCleared;
}

public FormFillFailure withSelectionCleared() {

This comment has been minimized.

Copy link
@jglick

jglick Mar 2, 2017

Member

missing Javadoc

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

spinner.gif seems to be missing too.

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

spinner.gif seems to be missing too.

Nope it is present already (which is why the css was added to the main css and not an adjunct)

@stephenc

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

@oleg-nenashev
Copy link
Member

left a comment

The error retrieval code dies not seem to be related to the PR title. Maybe you want to create a separate issue for Fill error handling.

I am also not sure we want to follow the single-message design. We already had this issue with FormValidation, where we had to rework the implementation to support multiple validation messages.

I think the best approach would be to reuse the existing FormValidation approach and maybe classes.

* Use one of the factory methods to create an instance, then throw it from your <tt>doFillXyz</tt>
* method.
*
* @since FIXME

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 3, 2017

Member

Please use @since TODO for the consistency. Otherwise awe may miss it during the bulkupdate

}
Behaviour.applySubtree(status);
// deleting values can result in the data loss, so let's not do that unless instructed
var header = rsp.getResponseHeader('X-Jenkins-Select-Error');

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 3, 2017

Member

I'm fine with that. Once we start following HTTP2 rules, we will need to either make Stapler to do it for us OR to perform a bulk update in the core and plugins. I prefer the first option

@jglick
jglick approved these changes Mar 3, 2017
@stephenc

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2017

No objections in 6 days... merging

@stephenc stephenc merged commit c3636eb into jenkinsci:master Mar 9, 2017

1 of 2 checks passed

continuous-integration/jenkins/pr-head This commit has test failures
Details
Jenkins This pull request looks good
Details

@stephenc stephenc deleted the stephenc:jenkins-42443 branch Mar 9, 2017

@oleg-nenashev oleg-nenashev referenced this pull request Mar 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.