Closed Bug 437711 Opened 12 years ago Closed 5 months ago

Use new Drag'n'Drop API in Calendar

Categories

(Calendar :: Internal Components, task, P1)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 76.0

People

(Reporter: sipaq, Assigned: khushil324)

References

Details

Attachments

(2 files, 6 obsolete files)

As nsISupportsArray is obsolete and isn't supported by the frozen API very
well, we should drop it on trunk.
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.
Daniel, Philipp,

what is our status here? Is this FIXED on the trunk?
(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'.
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.
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.
Assignee: nobody → mschroeder
Status: NEW → ASSIGNED
Summary: Drop nsISupportsArray usage from Calendar where possible → Drop nsISupportsArray usage from Calendar where possible (use new Drag'n'Drop API)
Martin, are you still working on this?
(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.
Hey Martin, I'd also take up un-cleaned up code per email if you'd like to share :-)
Martin, any updates? Do you still have any bits of code?
(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.
Hey Martin, are those bits still useful?
(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.
Assignee: mschroeder → philipp
Blocks: 394167
I'm pretty sure bug 1317871 will cover the part of this that blocks bug 394167. :aceman, does that sound right?
Flags: needinfo?(acelists)
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.
Flags: needinfo?(acelists) → needinfo?(philipp)
(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.
No longer blocks: 394167, nuke-nsSupportsArray
Summary: Drop nsISupportsArray usage from Calendar where possible (use new Drag'n'Drop API) → Use new Drag'n'Drop API in Calendar
Severity: normal → enhancement
Flags: needinfo?(philipp)
Priority: -- → P5
Assignee: philipp → nobody
Status: ASSIGNED → NEW
See Also: → 1226362

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?

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.)

Attachment #9131494 - Flags: review?(paul)
Attachment #9131494 - Flags: feedback?(mkmelin+mozilla)

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.

Attachment #9131495 - Flags: review?(paul)
Attachment #9131495 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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
Attachment #9131494 - Flags: feedback?(mkmelin+mozilla)
Attachment #9131494 - Attachment is obsolete: true
Attachment #9131494 - Flags: review?(paul)
Attachment #9131802 - Flags: review?(mkmelin+mozilla)
Attachment #9131495 - Attachment is obsolete: true
Attachment #9131495 - Flags: review?(paul)
Attachment #9131495 - Flags: feedback?(mkmelin+mozilla)
Attachment #9131803 - Flags: review?(paul)
Attachment #9131802 - Flags: review?(mkmelin+mozilla) → review?(paul)

(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.

Type: enhancement → task
Priority: P5 → P1
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.
Attachment #9131494 - Attachment is obsolete: false
Attachment #9131494 - Attachment is obsolete: true
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!
Attachment #9131803 - Flags: review?(paul) → feedback+
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.
Attachment #9131802 - Flags: review?(paul) → feedback+
Attachment #9131803 - Attachment is obsolete: true
Attachment #9132498 - Flags: review?(paul)
Attachment #9131802 - Attachment is obsolete: true
Attachment #9132499 - Flags: review?(paul)

(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 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.
Attachment #9132499 - Flags: review?(paul) → review+
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.
Attachment #9132498 - Flags: review?(paul) → review+

(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.

(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.

(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.

Attachment #9132498 - Attachment is obsolete: true
Attachment #9132784 - Flags: review+
Attachment #9132499 - Attachment is obsolete: true
Attachment #9132785 - Flags: review+

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

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 76
You need to log in before you can comment on or make changes to this bug.