Skip to content

Commit

Permalink
[FIXED JENKINS-14495] Hetero lists not working correctly after adding…
Browse files Browse the repository at this point in the history
… elements.

Unlike [JENKINS-14514] this is a true fix rather than a workaround (now removed), and is more general.
cjo9900 discovered that behaviors were being redundantly registered (as of 1.474 the monolithic JS is broken up);
this caused some behaviors to be run repeatedly on the same elements, breaking reasonable expectations of some behaviors.
The ideal fix would be to change Behavior.register to be idempotent: for example, key it by selector, then maintain a set of distinct behavior functions for each.
Unfortunately some adjuncts directly call Behavior.list.unshift, bypassing register(...), which would be tricky to intercept (would need to make a mock of Array).
The known one cases are in core, but it is possible plugin adjuncts do this too, in which case it would be incompatible to (say) change the Array<Map<String,Behavior>> to a Map<String,Array<Behavior>>.
Instead, permitting redundant registrations as before, and just silently skipping all but the first at runtime when applying behaviors.
Beware that since adjuncts are loaded from multiple places, different JS function objects are registered each time, so a naive set of behavior functions does not work;
have to identify functions by their toString in order to ensure that each is run only once.
(Currently once _per selector_, conceivably >1x per element; could if necessary be refined to make sure a given behavior is only run once on a given element during one call to applySubtree even if the element matches multiple selectors.)
  • Loading branch information
jglick committed Aug 3, 2012
1 parent 8349d0d commit dbb100d
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 7 deletions.
2 changes: 1 addition & 1 deletion changelog.html
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
<ul class=image>
<li class='major bug'>
Regressions in add/delete buttons starting in 1.474.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-14434">issue 14434</a> and <a href="https://issues.jenkins-ci.org/browse/JENKINS-14514">issue 14514</a>)
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-14434">issue 14434</a> and <a href="https://issues.jenkins-ci.org/browse/JENKINS-14495">issue 14495</a>)
<li class=rfe>
Collapse nonempty tool installation sections by default in <code>/configure</code>.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-14538">issue 14538</a>)
Expand Down
3 changes: 0 additions & 3 deletions core/src/main/resources/lib/form/repeatable/repeatable.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ var repeatableSupport = {
// do the initialization
init : function(container,master,insertionPoint) {
this.container = $(container);
if (this.container.tag != null) {
return;
}
this.container.tag = this;
master = $(master);
this.blockHTML = master.innerHTML;
Expand Down
16 changes: 13 additions & 3 deletions war/src/main/webapp/scripts/behavior.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,22 @@ var Behaviour = {
* this semantics is preserved.
*/
applySubtree : function(startNode,includeSelf) {
var behaviorsBySelector = {};
Behaviour.list._each(function(sheet) {
for (var selector in sheet){
function apply(n) {
var list = findElementsBySelector(n,selector,includeSelf);
if (list.length>0) // just to simplify setting of a breakpoint.
list._each(sheet[selector]);
var behavior = sheet[selector];
var behaviors = behaviorsBySelector[selector];
if (behaviors == null) {
behaviors = [];
behaviorsBySelector[selector] = behaviors;
}
if (behaviors.indexOf(behavior.toString()) == -1) {
behaviors.push(behavior.toString());
var list = findElementsBySelector(n,selector,includeSelf);
if (list.length>0) // just to simplify setting of a breakpoint.
list._each(sheet[selector]);
}
}

if (startNode instanceof Array) {
Expand Down

0 comments on commit dbb100d

Please sign in to comment.