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

RESOLVED FIXED

Status

Tree Management Graveyard
TBPL
--
enhancement
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.)
Created attachment 472127 [details] [diff] [review]
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.
Attachment #472127 - Flags: feedback?
Attachment #472127 - Flags: feedback? → feedback?(mstange)
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.
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 :).
Well there is always confusion about Array.map and jQuery.map in tbpl :(
(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.
Created attachment 483407 [details] [diff] [review]
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.
Assignee: nobody → jones.chris.g
Attachment #472127 - Attachment is obsolete: true
Attachment #483407 - Flags: review?
Attachment #472127 - Flags: feedback?(mstange)
Attachment #483407 - Flags: review? → review?(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...
Attachment #483407 - Flags: review?(mstange) → review+
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/eb0f6190d914
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(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?
Depends on: 611358
Now there is: bug 611358
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.