647078 - Drag & drop to add builds to a comment

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

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

When mass-commenting a set of builds, it would be nice to be able to drag builds into the comment box.

Comment 1

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

Attachment: drag and drop to add builds to comment box

I probably wouldn't have implemented this had I known that you could already add builds to the comment box by clicking on them and using the "add the comment to this build, too" link. But I only discovered that while implementing this feature, and I still think drag & drop is nicer.

Neither is discoverable at all, though. The comment box looks modal, so it's surprising you can do anything at all with the page while it's up.

The one thing I know is questionable is that I set the 'active' attribute on selected builds. It seems to nicely go away when dismissing the comment box, but I don't know if that'll break anything assuming there's only one. I think it should be ok because there's still only one UserInterface._activeResult.

Comment 2

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

Attachment: add "(drag builds here)" doc string

While I'm at it, this should fix the discoverability. I saw complaints about that in bug 578982 as well.

Comment 3

[Arpad Borsos [:Swatinem]]

Looks good :-)

Comment 4

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

Comment on attachment 523480 [details] [diff] [review]
drag and drop to add builds to comment box

Since you've already looked at it...

(I think philor has been using it for a while on his instance)

Comment 5

[Phil Ringnalda (:philor)]

The only thing I've ever noticed is that once when I was doing a multiple starting from one where I was using a suggestion and expecting it to comment on the bug, I'm pretty sure it only commented on the bug about the first one, but for all I know that's true of using the "add the comment to this build too" link - I almost never do multiple builds for anything other than "backed out in ...".

Comment 6

[Arpad Borsos [:Swatinem]]

Comment on attachment 523480 [details] [diff] [review]
drag and drop to add builds to comment box

Review of attachment 523480 [details] [diff] [review]:

::: index.html
@@ -72,4 +72,5 @@
 
 <script type="text/javascript" src="http://www.google.com/jsapi"></script>
 <script type="text/javascript" src="js/jquery.js"></script>
 <script type="text/javascript" src="js/jquery-ui-draggable.js"></script>
+<script type="text/javascript" src="js/jquery-ui-droppable.js"></script>

Just a note to myself to update jquery and jq-ui to the latest versions.

::: js/AddCommentUI.js
@@ +40,5 @@
+    $("#addNotePopup").droppable({
+        greedy: true,
+        drop: function(ev, ui) {
+            var dt = ev.originalEvent.dataTransfer;
+            var data = Controller.getData();

dt and data are both unused.

@@ +42,5 @@
+        drop: function(ev, ui) {
+            var dt = ev.originalEvent.dataTransfer;
+            var data = Controller.getData();
+            var id = ui.draggable.attr('resultID');
+            ui.draggable.attr('active', true);

You should probably use UI._setActiveResult for that, AFTER the if below.

Comment 7

[Arpad Borsos [:Swatinem]]

Comment on attachment 523483 [details] [diff] [review]
add "(drag builds here)" doc string

Review of attachment 523483 [details] [diff] [review]:

Good :-)

Comment 8

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

(In reply to comment #6)
> Comment on attachment 523480 [details] [diff] [review]
> ::: js/AddCommentUI.js
> @@ +40,5 @@
> +    $("#addNotePopup").droppable({
> +        greedy: true,
> +        drop: function(ev, ui) {
> +            var dt = ev.originalEvent.dataTransfer;
> +            var data = Controller.getData();
> 
> dt and data are both unused.

Oh, right. Those were notes to myself, having never done drag&drop before. Forgot to remove them.

> @@ +42,5 @@
> +        drop: function(ev, ui) {
> +            var dt = ev.originalEvent.dataTransfer;
> +            var data = Controller.getData();
> +            var id = ui.draggable.attr('resultID');
> +            ui.draggable.attr('active', true);
> 
> You should probably use UI._setActiveResult for that, AFTER the if below.

It's been a while, but I kind of remember that this was something of a hack: I want *all* the builds I drag from to be highlighted, because otherwise I forget which ones I've done so far and miss some or drag some more than once. So I want multiple builds to be highlighted, though I don't really want them to be "active" in any other sense. This happened to work, and also deactivated all of them when you activated something else, but probably isn't really the right way to do it. (I am using the "active" CSS class to now mean something weaker than UI._setActiveResult, which is an overloading of the word "active".)

What do you think? Is it ok as-is because it kinda sorta works? Or should I rename the CSS class to "selected" or "highlighted" and have _setActiveResult control that class instead of/in addition to "active"?

Comment 9

[Arpad Borsos [:Swatinem]]

(In reply to comment #8)
> > @@ +42,5 @@
> > +        drop: function(ev, ui) {
> > +            var dt = ev.originalEvent.dataTransfer;
> > +            var data = Controller.getData();
> > +            var id = ui.draggable.attr('resultID');
> > +            ui.draggable.attr('active', true);
> > 
> > You should probably use UI._setActiveResult for that, AFTER the if below.
> 
> It's been a while, but I kind of remember that this was something of a hack: I
> want *all* the builds I drag from to be highlighted, because otherwise I forget
> which ones I've done so far and miss some or drag some more than once. So I
> want multiple builds to be highlighted, though I don't really want them to be
> "active" in any other sense. This happened to work, and also deactivated all of
> them when you activated something else, but probably isn't really the right way
> to do it. (I am using the "active" CSS class to now mean something weaker than
> UI._setActiveResult, which is an overloading of the word "active".)
> 
> What do you think? Is it ok as-is because it kinda sorta works? Or should I
> rename the CSS class to "selected" or "highlighted" and have _setActiveResult
> control that class instead of/in addition to "active"?

I’m fine landing it as-is, although I would much appreciate a followup to create a clear distinction between "active and showing in the bottom bar" and "indicating it is one of more runs selected for commenting".

As we are discussing this, I guess the drag action does not trigger the onclick _setActiveResult chain?

Comment 10

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

(In reply to comment #9)
> I’m fine landing it as-is,

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/02d5e3edb2d3
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/8bb8a01b2eb2

> As we are discussing this, I guess the drag action does not trigger the onclick
> _setActiveResult chain?

Correct, it does not. From a UX point of view, I'd rather it didn't; dragging is supposed to feel lightweight.

> although I would much appreciate a followup to
> create a clear distinction between "active and showing in the bottom bar" and
> "indicating it is one of more runs selected for commenting".

Good idea. Bug 654512. If you try, you can get weird behavior from the CSS class sharing right now. Now the question is to figure out what it should look like, since they're somewhat independent...