593472 - TBPL can make it easier to paste large numbers of cset URLs into a bug

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pretty often nowadays we fire off pushes with a bunch of csets.  (I think roc holds the record at ~50.)  It's a PITA to copy and paste each URL individually.  It would be neat if I could click a button or something on a push header that popped up a box with all the push's cset URLs in copy-able format.

(Not all csets in a push will always be for the same bug, but it's easy to only copy the ones one wants.)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Throw an alert containing all changesets for a push in copy-able form

Yeah ... alert() ;).  This does what I want it to, but I don't know how it should fit into the TPBL UI.

Comment 2

[Arpad Borsos [:Swatinem]]

Comment on attachment 472127 [details] [diff] [review]
Throw an alert containing all changesets for a push in copy-able form

>+      var pushedCsets = push.patches.reduce(
>+        function (acc, patch) { return patchURL(patch) +'\\n'+ acc; },
>+        '');

Using reduce for this use case is really interesting. But as far as I know, it’s firefox specific. Would be better to use more portable (or jquery) functions, also to stay in line with the rest of tbpl.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

So [array].map is "more standard" than [array].reduce?  (TBPL uses the former; not sure whose implementation, FF's or jquery's.)  Interesting, one learns something new every day :).

Comment 4

[Arpad Borsos [:Swatinem]]

Well there is always confusion about Array.map and jQuery.map in tbpl :(

Comment 5

[Markus Stange [:mstange]]

(In reply to comment #2)
> Using reduce for this use case is really interesting. But as far as I know,
> it’s firefox specific.

Safari 5 supports it, that's standard enough for me. I don't see a reason not to use reduce, except that I think it's not the right tool for the job in this case (see below).
I agree that we should make tbpl consistent regarding map usage, but that's for another bug.

About the patch:

+      var pushedCsets = push.patches.reduce(
+        function (acc, patch) { return patchURL(patch) +'\\n'+ acc; },
+        '');

push.patches.map(patchURL).join('\n') is shorter :)

I think we should build the list only when the button is clicked. Parsing innerHTML is expensive, so we should keep it as short as possible.

One way to that would be to make Data._getPushForRev public (i.e. remove the underscore) and then do <button onclick="UserInterface.listChangesetsForPush('<insert push.toprev here>')"...>.
(Note that there's no "javascript:" necessary in event handler attributes.)

As for the UI, could you change "copy cset URLs" to "List changeset URLs" and remove the brackets around the button? Maybe add a little margin-left to it, too.
Instead of an alert I'd prefer a textarea, but I can implement that myself in a follow-up bug.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Throw an alert containing all changesets for a push in copy-able form

(In reply to comment #5)
> About the patch:
> 
> +      var pushedCsets = push.patches.reduce(
> +        function (acc, patch) { return patchURL(patch) +'\\n'+ acc; },
> +        '');
> 
> push.patches.map(patchURL).join('\n') is shorter :)
> 

Also doesn't give the same result :).  I used reduce here because I wanted to reverse the order of the revs.  Tradition when pasting into bugzilla is first-cset-to-last, but they're stored last-to-first in this array.  This new patch uses map then reverse, and I added a comment.

> I think we should build the list only when the button is clicked. Parsing
> innerHTML is expensive, so we should keep it as short as possible.
> 

OK.  (Although I don't want to believe you! ;) .)

> One way to that would be to make Data._getPushForRev public (i.e. remove the
> underscore) and then do <button
> onclick="UserInterface.listChangesetsForPush('<insert push.toprev here>')"...>.
> (Note that there's no "javascript:" necessary in event handler attributes.)
> 

Done.

> As for the UI, could you change "copy cset URLs" to "List changeset URLs" and
> remove the brackets around the button? Maybe add a little margin-left to it,
> too.

Done.

> Instead of an alert I'd prefer a textarea, but I can implement that myself in a
> follow-up bug.

Yes, the alert() is meant to be a placeholder.

I left the bug unassigned in case someone else wanted to take on the task of a "real" UI, but if that's going to a followup, I'll reassign this -->me.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 483407 [details] [diff] [review]
Throw an alert containing all changesets for a push in copy-able form

Ugh, sorry for the delay here...

Comment 8

[Markus Stange [:mstange]]

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

Comment 9

[Nochum Sossonko [:Natch]]

(In reply to comment #5)
> Instead of an alert I'd prefer a textarea, but I can implement that myself in a
> follow-up bug.

Is there a followup for that?

Comment 10

[Markus Stange [:mstange]]

Now there is: bug 611358