437711 - Use new Drag'n'Drop API in Calendar

Status: RESOLVED FIXED

(Calendar :: Internal Components, task, P1)

Description

[Simon Paquet [:sipaq]]

As nsISupportsArray is obsolete and isn't supported by the frozen API very
well, we should drop it on trunk.

Comment 1

[Daniel Boelzle [:dbo]]

I think we're lacking of frozen alternatives. Using nsIDragService requires passing an nsISupportsArray, even on trunk:
http://mxr.mozilla.org/seamonkey/source/widget/public/nsIDragService.idl#66

I think what's more important (but already done for trunk) is that we link our native code against the frozen core libs. At least in js code, we could reasonably well work around interface changes; frozen interfaces would be favourable though.

Comment 2

[Simon Paquet [:sipaq]]

Daniel, Philipp,

what is our status here? Is this FIXED on the trunk?

Comment 3

[Daniel Boelzle [:dbo]]

still used:
<http://mxr.mozilla.org/mozilla-central/source/widget/public/nsIDragService.idl#68>

Comment 4

[Martin Schröder [:mschroeder]]

(In reply to comment #3)
> still used:
> <http://mxr.mozilla.org/mozilla-central/source/widget/public/nsIDragService.idl#68>

Is it possible to avoid this API at all? In Gecko 1.9.1 we can use the API described on https://developer.mozilla.org/En/DragDrop/Drag_and_Drop. But I don't know if this could replace the direct use of 'invokeDragSession'.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

While I haven't found out how to set non-string data in the drag session (other than using index-specific methods), the new dnd api seems to allow us to not use nsITransferable, so we could get around using invokeDragSession.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

I knew how to do it afterall, just needed some help on understanding the interface docs: 

http://mxr.mozilla.org/comm-central/source/mozilla/dom/public/idl/events/nsIDOMDataTransfer.idl#225


we can do:

let dt = event.dataTransfer;
dt.mozSetDataAt("application/x-moz-cal-item", item, 0);
dt.setData("text/calendar", item.icalString);
dt.setData("text/unicode", item.icalString);

in the dragstart event, which will cause the backend code to invoke the drag session. To retrieve the data we can then do the following in the "drop" event. Removing shadows can then be done in dragend handlers as we already do.

let item = dt.mozGetDataAt(""application/x-moz-cal-item", 0)
             .QueryInterface(Ci.calIItemBase);

This could greatly simplify drag handling.

Comment 7

[Simon Paquet [:sipaq]]

Martin, are you still working on this?

Comment 8

[Martin Schröder [:mschroeder]]

(In reply to comment #7)
> Martin, are you still working on this?

I have some basic D'n'D code which needs some cleanup before I can hand it over to someone to continue the work.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Hey Martin, I'd also take up un-cleaned up code per email if you'd like to share :-)

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Martin, any updates? Do you still have any bits of code?

Comment 11

[Martin Schröder [:mschroeder]]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Martin, any updates? Do you still have any bits of code?

Philipp, I found a backup of my code and a copy of the plan on how to rewrite drag'n'drop handling. I will attach the useful pieces Friday night after checking if they could be helpful.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Hey Martin, are those bits still useful?

Comment 13

[Martin Schröder [:mschroeder]]

(In reply to Philipp Kewisch [:Fallen] from comment #12)
> Hey Martin, are those bits still useful?

Sorry, I missed to get back to you. The code I recovered is not useful (and also not understandable). My plan was to create a jsm for calendar drag'n'drop handling because it takes place in various parts of the user interface but needs consistent handling and processing.

Comment 14

[Eric Rahm [:erahm]]

I'm pretty sure bug 1317871 will cover the part of this that blocks bug 394167. :aceman, does that sound right?

Comment 15

[:aceman]

I do not see references to nsISupportsArray in the mentioned nsIDragService.idl and nsIDOMDataTransfer.idl files. Eric, maybe you could have put the calendar parts of the patch here.

Comment 16

[Eric Rahm [:erahm]]

(In reply to :aceman from comment #15)
> I do not see references to nsISupportsArray in the mentioned
> nsIDragService.idl and nsIDOMDataTransfer.idl files. Eric, maybe you could
> have put the calendar parts of the patch here.

Okay, I'm going to remove this as a blocker for the nuke nsISupportsArray bugs. I suppose folks might still want to use the modern API, so I'm not going to close/dup this bug.

Comment 17

[Magnus Melin [:mkmelin]]

https://developer.mozilla.org/en-US/docs/Web/API/HTML_Drag_and_Drop_API

Comment 18

[Khushil Mistry [:khushil324]]

I found that drag and drop function is not working for the calendar sidebar i.e. today pane. And this is the case for both trunk and ESR 68. Do you know any bugs filed for this?

Comment 19

[Magnus Melin [:mkmelin]]

Bug 419343 and bug 536526 are the ones I find from a quick search. (I don't know what is expected wrt dnd and the today pane.)

Comment 20

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_improve-dnd-today-pane-0.patch

Comment 21

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_use-new-dnd-calendar-0.patch

Apply this patch over Bug-437711_improve-dnd-today-pane-0.patch.

In the Calendar, we can not attach any file in the attachment for the event or task. We can only attach the “URI” in the attachments as iCal/CalDav supports that only. So right now, If you drop any file, it will take a URI of the file. I and Paul had a discussion on the Matrix. We found out that we can attach any file as inline binary encoded content according to iCal/CalDav documentation: https://tools.ietf.org/html/rfc2445#section-4.8.1.1. I am going to open a bug for this and carry on the discussion there.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 9131494 [details] [diff] [review]
Bug-437711_improve-dnd-today-pane-0.patch

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

::: calendar/base/content/calendar-dnd-listener.js
@@ +216,5 @@
> +    let dataFlavor, data;
> +    for (let i = 0; i < dt.mozItemCount; i++) {
> +      if (i === this.MAX_EVENT_SUPPORT) {
> +        return;
> +      }

you could just put this in the for clause.
I'm not sure it should be a constant. You could just do a local const and add an explanation
const MAX = 8; // Let's say we want to handle max 8 items.
for (let i = 0; i < dt.mozItemCount && i < MAX; i++) {

@@ +287,5 @@
>      }
>  
> +    if (!transData) {
> +      return;
> +    }

doesn't look like this would happen?
transData is an object

@@ +301,5 @@
>          this.onDropItems(parser.getItems().concat(parser.getParentlessItems()));
>          break;
>        }
>        case "text/unicode": {
> +        let separator = transData.toString().indexOf("\n");

should this be transData.data?

@@ +485,5 @@
> +  /**
> +   * calCalendarButtonDNDObserver::onDropURL
> +   *
> +   * Gets called in case we're dropping a URL
> +   * on the 'calendar mode'-button.

please use the full 80 (or since calendar, 100) ch width

@@ +487,5 @@
> +   *
> +   * Gets called in case we're dropping a URL
> +   * on the 'calendar mode'-button.
> +   *
> +   * @param URL     The URL to handle.

for these, please add type

Comment 23

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_improve-dnd-today-pane-1.patch

Comment 24

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_use-new-dnd-calendar-1.patch

Comment 25

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #22)

should this be transData.data?

It will be the same. I have just changed the variable name and used the previously writen code.

Comment 26

[Paul Morris [:pmorris]]

Comment on attachment 9131494 [details] [diff] [review]
Bug-437711_improve-dnd-today-pane-0.patch

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

Looking good.  I have some comments on the code.  Sorry it's on an earlier version of the patch.  I had started my review on this version before the latest.  I think my comments still apply to the latest.

::: calendar/base/content/calendar-dnd-listener.js
@@ +191,5 @@
>  }
>  
>  calDNDBaseObserver.prototype = {
>    // initialize this class's members
>    initBase() {},

While we're here let's remove this `initBase` function (since it does nothing), the comment above it, and everywhere it is called by sub-classes.

@@ +192,5 @@
>  
>  calDNDBaseObserver.prototype = {
>    // initialize this class's members
>    initBase() {},
> +  MAX_EVENT_SUPPORT: 8,

MAX_EVENTS_SUPPORTED or just MAX_EVENTS might be clearer?  [Edit: I see mkmelin's comment about moving it, and that makes sense to me.]

@@ +211,5 @@
>  
> +  onDrop(event, transferData, dragSession) {
> +    let flavours = ["text/x-moz-message", "text/x-moz-address", "text/x-moz-url"];
> +
> +    let dt = event.dataTransfer;

I'd use "dataTransfer" or maybe just "transfer" rather than "dt", for readability.

@@ +212,5 @@
> +  onDrop(event, transferData, dragSession) {
> +    let flavours = ["text/x-moz-message", "text/x-moz-address", "text/x-moz-url"];
> +
> +    let dt = event.dataTransfer;
> +    let dataFlavor, data;

Let's be consistent about "flavour" vs "flavor", although I see the current code already is not consistent...  hmpf, well, if we can make it consistent at least within these functions while we're here, let's do that.  (Either spelling is fine with me, as long as we stick with one here.)

@@ +260,5 @@
> +    }
> +
> +    if (dataFlavor) {
> +      return;
> +    }

I think this whole new section above here, it would be good to pull it out into its own function since it handles one case and the code below is a different case and there's very little that is shared between them.  As it is this function is getting long and unwieldy.

@@ +287,5 @@
>      }
>  
> +    if (!transData) {
> +      return;
> +    }

I'd move this early return up above the `if (bestFlavor.value...` block just above here.  That block of code doesn't need to run if we're returning early.

@@ +305,5 @@
> +        let separator = transData.toString().indexOf("\n");
> +        if (separator != -1) {
> +          transData = transData.substr(0, separator);
> +        }
> +        let droppedUrl = transData;

This bit would be clearer without re-assigning transData, for example:
```
let separator = transData.toString().indexOf("\n");
let droppedUrl = (separator != -1) ? transData.substr(0, separator) : transData;
```
Or more concise:
```
let droppedUrl = transData.toString().split("\n")[0];
```

@@ +485,5 @@
> +  /**
> +   * calCalendarButtonDNDObserver::onDropURL
> +   *
> +   * Gets called in case we're dropping a URL
> +   * on the 'calendar mode'-button.

Might be clearer to say 'the "open calendar tab" button.'?

@@ +489,5 @@
> +   * on the 'calendar mode'-button.
> +   *
> +   * @param URL     The URL to handle.
> +   */
> +  onDropURL(URL) {

I'd use lowercase "url" for this parameter, and let's go with "uri" to match "attachment.uri" (below).

@@ +517,5 @@
> +    attendee.role = "REQ-PARTICIPANT";
> +    attendee.participationStatus = "NEEDS-ACTION";
> +    let attendees = [];
> +    let j = 0;
> +    let addAttendee = address => {

No need to manually track the index with `j` here.  The second argument to a `forEach` function is the index, so you can do:
```
let addAttendee = (address, index) => {
```
and then use index as your j.

@@ +520,5 @@
> +    let j = 0;
> +    let addAttendee = address => {
> +      if (address.name.length == 0) {
> +        return;
> +      }

A more functional programming way to do this is to first `filter` and then `map`.  Something like:
```
let attendees = parsedInput
  .filter(address => address.name.length > 0)
  .map((address, index) => {
    // ...convert address to attendee...
    return attendee;
});
```

@@ +543,5 @@
> +    newItem.calendar = getSelectedCalendar();
> +    cal.dtz.setDefaultStartEndHour(newItem);
> +    cal.alarms.setDefaultValues(newItem);
> +    for (let attendee of attendees) {
> +      newItem.addAttendee(attendee.clone());

Do we really need to `.clone()` here?  Seems like it would be unnecessary.

@@ +599,5 @@
> +  /**
> +   * calTaskButtonDNDObserver::onDropURL
> +   *
> +   * Gets called in case we're dropping a URL
> +   * on the 'task mode'-button.

'the "open tasks tab" button' seems clearer to me.

Comment 27

[Paul Morris [:pmorris]]

Comment on attachment 9131803 [details] [diff] [review]
Bug-437711_use-new-dnd-calendar-1.patch

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

These changes look good to me.  It's great that we can drop the old legacy code and use the new approach.  Thanks for separating this out into two patches!  Makes review easier and more effective.  I'll await the next version of the other patch and then test the functionality.

::: mail/base/content/nsDragAndDrop.js
@@ -602,5 @@
> -
> -      throw new Error("Drop of " + aDraggedText + " denied.");
> -    }
> -  },
> -};

Yay for being able to drop this legacy code!

Comment 28

[Paul Morris [:pmorris]]

Comment on attachment 9131802 [details] [diff] [review]
Bug-437711_improve-dnd-today-pane-1.patch

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

Great to have type declarations in the doc strings. I wonder if we could be more specific than {Object} in some of these cases, but haven't looked closely and something is much better than nothing.  For more comments see review of previous version of the patch.

Comment 29

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_use-new-dnd-calendar-2.patch

Comment 30

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_improve-dnd-today-pane-2.patch

Comment 31

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #26)

Let's be consistent about "flavour" vs "flavor", although I see the current
code already is not consistent... hmpf, well, if we can make it consistent
at least within these functions while we're here, let's do that. (Either
spelling is fine with me, as long as we stick with one here.)

It will create confusion in these patches. I am planning to submit a 3rd patch with these changes.

Comment 32

[Paul Morris [:pmorris]]

Comment on attachment 9132499 [details] [diff] [review]
Bug-437711_improve-dnd-today-pane-2.patch

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

Changes look good, thanks.  

I tested it out by dragging: 1. email messages from within TB, 2. links from those email messages, 3. files from an OS window, 4, url string from a text editor outside of TB.  When I dragged these items to the today pane it worked as expected for 1, 2, and 3 (the new event or task dialog opened with the items included).  Number 4 didn't work for me anywhere.  I'm not positive that 4 is even expected to work?  When dragging anything to the calendar and task buttons it did not work consistently for me.  It seems these are not regressions but things that also weren't working before (right?) so let's work on them in a follow-up bug or bugs.

r+ with a few minor comments addressed.

::: calendar/base/content/calendar-dnd-listener.js
@@ +279,5 @@
>    onDragExit(aEvent, aDragSession) {},
>  
>    onDropItems(aItems) {},
>    onDropMessage(aMessage) {},
> +  onDropEventData(dataTransfer) {

Please add a doc string for this new function, to make it clearer what cases it handles.

@@ +306,5 @@
> +          break;
> +        case "text/x-moz-url": {
> +          data = data.toString().split("\n")[0];
> +          if (!data) {
> +            return;

If you return 'false' here you won't need to disable eslint below.

@@ +325,2 @@
>      }
> +    // eslint-disable-next-line consistent-return

You can avoid this line, as mentioned above.

@@ +499,5 @@
> +    attendee.participationStatus = "NEEDS-ACTION";
> +    let attendees = parsedInput
> +      .filter(address => address.name.length > 0)
> +      .map((address, index) => {
> +        // ...convert address to attendee...

Please omit the "..." and capitalize, etc., like:
// Convert address to attendee.

Comment 33

[Paul Morris [:pmorris]]

Comment on attachment 9132498 [details] [diff] [review]
Bug-437711_use-new-dnd-calendar-2.patch

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

No changes here right?  Just rebased I assume?  I looked but didn't notice anything that changed from the previous version.

Comment 34

[Paul Morris [:pmorris]]

(In reply to Khushil Mistry [:khushil324] from comment #31)

It will create confusion in these patches. I am planning to submit a 3rd patch with these changes.

Makes sense to me, thanks.

Comment 35

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #32)

I tested it out by dragging: 1. email messages from within TB, 2. links from
those email messages, 3. files from an OS window, 4, url string from a text
editor outside of TB. When I dragged these items to the today pane it
worked as expected for 1, 2, and 3 (the new event or task dialog opened with
the items included). Number 4 didn't work for me anywhere. I'm not
positive that 4 is even expected to work? When dragging anything to the
calendar and task buttons it did not work consistently for me. It seems
these are not regressions but things that also weren't working before
(right?) so let's work on them in a follow-up bug or bugs.

Now I realized, I am not able to see "calendarTaskButtonDNDObserver" and "calendarCalendarButtonDNDObserver" being used in the task and calendar buttons. They are just used in Today Pane Sidebar.

Comment 36

[Khushil Mistry [:khushil324]]

(In reply to Paul Morris [:pmorris] from comment #33)

No changes here right? Just rebased I assume? I looked but didn't notice
anything that changed from the previous version.

Yes, just the rebasing.

Comment 37

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_use-new-dnd-calendar-3.patch

Comment 38

[Khushil Mistry [:khushil324]]

Attachment: Bug-437711_improve-dnd-today-pane-3.patch

Comment 39

[Khushil Mistry [:khushil324]]

First, apply Bug-437711_improve-dnd-today-pane-3.patch and then Bug-437711_use-new-dnd-calendar-3.patch.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/40262b53174b
Improve dnd operations in Today Pane. r=pmorris
https://hg.mozilla.org/comm-central/rev/9aa142882f18
Use new Drag'n'Drop API in Calendar. r=pmorris