655887 - Use multiselecting for csets as well as builds

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Steve Fink [:sfink] [:s:]]

Bug 654528 did an initial change aimed at improving the workflow for starring multiple builds with the same cset. This bug continues.

I would like to be able to multi-select builds and select a cset or csets, then bring up the comment box to add a comment containing each of those csets' URLs to each of those builds.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Multiselect csets and builds to add comment

1. Allow csets to be 'selected'.
2. Change builds to have a 'selected' state instead of the previous 'commenting' state I was using.
3. When the comment box is opened, populate it with one cset URL for each selected cset, and associate it with all selected builds.
4. Allow the comment box to be opened even with no build active.

The use case this is intended for is when you have a set of builds that all failed for the same reason, and you want to comment them with the cset that either broke them or will fix them. The expected workflow is:

1. Ctrl-click to select the cset or csets
2. Use 'j' and 'k' to flip through the unstarred failures, hitting space on each one that you want to star.
3. 'c' to bring up the comment box.
4. After a quick eyeballing to make sure it looks ok, or possibly after adding some additional comments ("fixed by backout..."), press submit.

If you're happier with the mouse, replace step 2 with ctrl-clicking the relevant failures. If you want to see the basic log info, click on each failure, wait for the info to appear, then space to select that build iff desired. If you like drag & drop, start out with 'c' to open the comment box, then drag everything relevant into the box. If you select more than you wanted, ctrl-click (in the area outside of the box) to deselect (or for cset comments, you can just edit the comment to remove the cset).

Missing pieces and issues:
- No way to scroll through csets with the keyboard
- No automatic way to grab all failures resulting from a cset or push
- No way to select or drag a push, only csets
- If you select some csets and then edit the comment to remove them, they'll still be (harmlessly) selected

I think all of those can be considered followup bugs, if people want them.

Comment 2

[Arpad Borsos [:Swatinem]]

Comment on attachment 531205 [details] [diff] [review]
Multiselect csets and builds to add comment

Review of attachment 531205 [details] [diff] [review]:
-----------------------------------------------------------------

We are using underscores to designate “private” functions, that hasn’t worked for quite some time.
But don’t bother, I wanted to move the UI part of AddCommentUI into UI for quite some time (and move the comment submission part into its own class).

::: js/AddCommentUI.js
@@ +59,5 @@
>      });
> +
> +    // Defeat the keep-text-on-reload feature, because it results in
> +    // comments containing changesets that are no longer selected.
> +    $("#logNoteText").get(0).value = "";

$("#logNoteText").val("") seems nicer somehow. ;-)

@@ +277,5 @@
> +    else
> +      return prev + text + "\n";
> +  },
> +
> +  addRevToComment: function AddCommentUI__addRevToComment(rev) {

Just one underscore to designate the “namespace”. The same for remove below.

@@ +289,5 @@
> +    // Remove the deselected revision if and only if it shows up on a line by
> +    // itself.
> +    var box = $("#logNoteText").get(0);
> +    var url = UserInterface._changesetURL(rev);
> +    var offset = box.value.search(url + "\n");

Is .search a synonym for indexOf? We use the latter, so I think it is more clearer.

::: js/UserInterface.js
@@ +137,4 @@
>      $(".revlink").draggable({ helper: 'clone' });
> +
> +    $(".revlink").unbind('click.selectedRev');
> +    $(".revlink").bind('click.selectedRev', function (event) {

Since when can you add a classname to the event name in jquery? Also, why do you need the unbind before?

@@ +797,5 @@
>      AddCommentUI.updateUI();
> +    return selected;
> +  },
> +
> +  _toggleSelectedRev: function UserInterface__toggleSelectedRev(rev,force) {

Space between arguments.

@@ +801,5 @@
> +  _toggleSelectedRev: function UserInterface__toggleSelectedRev(rev,force) {
> +    var selected;
> +
> +    if (typeof(force) === "undefined")
> +      force = !(rev in this._selectedRevs);

I like this better:
var add = force ? true : !(rev in this._selectedRevs);

@@ +814,5 @@
> +      selected = false;
> +    }
> +    AddCommentUI.updateUI();
> +    this._markSelected();
> +    return selected;

Also, return add; here, since it is the same.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to comment #2)
> Comment on attachment 531205 [details] [diff] [review] [review]
> Multiselect csets and builds to add comment
> 
> Review of attachment 531205 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> We are using underscores to designate “private” functions, that hasn’t
> worked for quite some time.
> But don’t bother, I wanted to move the UI part of AddCommentUI into UI for
> quite some time (and move the comment submission part into its own class).

Yes, I wasn't sure when to use a leading underscore, because AddCommentUI was using UserInterface._activeResult all over the place. So I thought maybe they were meant to be friends, or ...?

Maybe I'll wait for that refactoring before I start on what I intended next, which was to use the selected builds with the self-service API.

> 
> ::: js/AddCommentUI.js
> @@ +59,5 @@
> >      });
> > +
> > +    // Defeat the keep-text-on-reload feature, because it results in
> > +    // comments containing changesets that are no longer selected.
> > +    $("#logNoteText").get(0).value = "";
> 
> $("#logNoteText").val("") seems nicer somehow. ;-)

I agree. I copied the .get(0).value pattern from elsewhere in this file, but now I've switched nearly everything over to .val(). The .get(0) bothers me.

I've done enough jquery now that I shouldn't have forgotten .val().

> 
> @@ +277,5 @@
> > +    else
> > +      return prev + text + "\n";
> > +  },
> > +
> > +  addRevToComment: function AddCommentUI__addRevToComment(rev) {
> 
> Just one underscore to designate the “namespace”. The same for remove below.

Ok, done.

> @@ +289,5 @@
> > +    // Remove the deselected revision if and only if it shows up on a line by
> > +    // itself.
> > +    var box = $("#logNoteText").get(0);
> > +    var url = UserInterface._changesetURL(rev);
> > +    var offset = box.value.search(url + "\n");
> 
> Is .search a synonym for indexOf? We use the latter, so I think it is more
> clearer.

Whoops. Someday I'll internalize JS enough to stop coding by Google search.

.search is the regex equivalent of .indexOf. I don't want regexes here (or in other similar places.)

Fixed.

> ::: js/UserInterface.js
> @@ +137,4 @@
> >      $(".revlink").draggable({ helper: 'clone' });
> > +
> > +    $(".revlink").unbind('click.selectedRev');
> > +    $(".revlink").bind('click.selectedRev', function (event) {
> 
> Since when can you add a classname to the event name in jquery? Also, why do
> you need the unbind before?

The two are related. If I don't unbind, jquery will now (as of 1.4.2) accumulate click handlers (even if the function is === identical!) I had to track down why it was running so slowly. Fortunately, I had a bug where my action was not idempotent as intended, but added an extra newline each time; otherwise, I never would have noticed.

Because this binding code runs multiple times (eg once per push update, but even on the initial load it runs 3 or 4 times), I need to cancel the previous binding. One way to do that is to keep the bound function somewhere and pass it to unbind(). Another way is to unbind with just the name of the handler ('click'). That'll erase all previous bindings. To keep from interfering with other bindings that some other code is using, you can optionally suffix with a namespace. (It reads backwards to me, but whatever.)

http://api.jquery.com/bind/

...and in fact, when testing the updates to my patch, I found that I made the same mistake with the other click handler. For some reason, I was getting an odd number of handlers installed before, and now I'm getting an even number. So toggling appeared to be working, and now it doesn't. :-)

> 
> @@ +797,5 @@
> >      AddCommentUI.updateUI();
> > +    return selected;
> > +  },
> > +
> > +  _toggleSelectedRev: function UserInterface__toggleSelectedRev(rev,force) {
> 
> Space between arguments.

ok

> @@ +801,5 @@
> > +  _toggleSelectedRev: function UserInterface__toggleSelectedRev(rev,force) {
> > +    var selected;
> > +
> > +    if (typeof(force) === "undefined")
> > +      force = !(rev in this._selectedRevs);
> 
> I like this better:
> var add = force ? true : !(rev in this._selectedRevs);

Not the same. If force is false and !this._selectedRevs[rev], yours would set add to true.

Updating |force| is messy, I agree. How about

var add;
if (typeof(force) != "undefined")
  add = force;
else
  add = !(rev in this._selectedRevs);

> 
> @@ +814,5 @@
> > +      selected = false;
> > +    }
> > +    AddCommentUI.updateUI();
> > +    this._markSelected();
> > +    return selected;
> 
> Also, return add; here, since it is the same.

Right you are!

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Multiselect csets and builds to add comment

Comment 5

[Arpad Borsos [:Swatinem]]

Comment on attachment 531383 [details] [diff] [review]
Multiselect csets and builds to add comment

Review of attachment 531383 [details] [diff] [review]:
-----------------------------------------------------------------

Nevermind the underscores, that refactoring will happen, someday in the distant future.
Also thanks for enlightening me about the jquery .bind() semantics, cleaning up those event handlers is also a todo item for the future, I remember similar things were put as onclick="" handlers into the tags for performance reasons some time ago.

::: js/UserInterface.js
@@ -666,5 @@
>          }
>  
>          self._setActiveResult(unstarred[result].runID, true);
>          AddCommentUI.clearAutoStarBugs();
> -        reutrn false;

I wonder how this could have worked before. :-D

@@ +795,5 @@
> +    } else {
> +      this._selectedBuilds[id] = true;
> +      selected = true;
> +    }
> +    window.console.log("selected[" + id + "] = " + selected);

remove this console.log

@@ +808,3 @@
>      else
> +      add = !(rev in this._selectedRevs);
> +

So you want to be able to force remove it, I didn’t get that, sorry.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Multiselect csets and builds to add comment

(In reply to comment #5)
> Comment on attachment 531383 [details] [diff] [review] [review]
> Multiselect csets and builds to add comment
> 
> Review of attachment 531383 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Nevermind the underscores, that refactoring will happen, someday in the
> distant future.
> Also thanks for enlightening me about the jquery .bind() semantics, cleaning
> up those event handlers is also a todo item for the future, I remember
> similar things were put as onclick="" handlers into the tags for performance
> reasons some time ago.
> 
> ::: js/UserInterface.js
> @@ -666,5 @@
> >          }
> >  
> >          self._setActiveResult(unstarred[result].runID, true);
> >          AddCommentUI.clearAutoStarBugs();
> > -        reutrn false;
> 
> I wonder how this could have worked before. :-D

That was from something earlier in my patch stack. I've updated the earlier patch.

> @@ +795,5 @@
> > +    } else {
> > +      this._selectedBuilds[id] = true;
> > +      selected = true;
> > +    }
> > +    window.console.log("selected[" + id + "] = " + selected);
> 
> remove this console.log

Doh!

And now, to cause you more trouble, I also changed the behavior of selecting a cset. First, as in bug 655879 it now uses only the hash, not the full url. But the part that requires re-review is that I changed it to remove the entire line when deselecting a changeset with the comment box open, using a happy fun regex. I also changed addRevToComment to do its add-new-line logic inline.

(You can probably just look at addRevToComment and removeRevFromComment.)

Thanks!

Comment 7

[Arpad Borsos [:Swatinem]]

Comment on attachment 531760 [details] [diff] [review]
Multiselect csets and builds to add comment

Review of attachment 531760 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/AddCommentUI.js
@@ +286,5 @@
> +
> +  removeRevFromComment: function AddCommentUI_removeRevFromComment(rev) {
> +    // Remove whole line containing the given rev, if it exists
> +    var box = $("#logNoteText");
> +    box.val(box.val().replace(new RegExp(".*" + rev + ".*\n?", ""), ""));

Doesn’t the .* remove too much? You want to remove the whole line, but I’m not sure if the .* matches other lines too.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to comment #7)
> Comment on attachment 531760 [details] [diff] [review] [review]
> Multiselect csets and builds to add comment
> 
> Review of attachment 531760 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: js/AddCommentUI.js
> @@ +286,5 @@
> > +
> > +  removeRevFromComment: function AddCommentUI_removeRevFromComment(rev) {
> > +    // Remove whole line containing the given rev, if it exists
> > +    var box = $("#logNoteText");
> > +    box.val(box.val().replace(new RegExp(".*" + rev + ".*\n?", ""), ""));
> 
> Doesn’t the .* remove too much? You want to remove the whole line, but I’m
> not sure if the .* matches other lines too.

...which is exactly why my first cut was "[^\n]*" + rev + "[^\n]\n?", but then I read in the docs that . doesn't match newline, so I switched to reduce the line noise ratio. I'll tack on a comment since neither of us was sure.

Comment 9

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/42964668d16b

Comment 10

[Dão Gottwald [:dao]]

This breaks the rather basic functionality of ctrl+clicking changeset links to open them in new tabs. Can we revert this?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(ctrl+click on changesets was filed as bug 661677)