Drag and Drop (revisited) #4048

brueckner · 2020-11-10T11:56:24Z

Adds drag & drop support for moving emails to folders

First off, I know there has been a previous attempt (#2834) to implement this and it has been closed in favor of #3591. This PR doesn't aim to replace the newly implemented modal window to move mails, but rather to complement it. It in fact uses methods introduced in #3591 to actually move the mails.

This approach also makes use of the multi-selection feature, to enable dragging of single and multiple emails.

I have recorded a quick video to showcase the feature (too bad I cant attach it right here, Nextcloud to the rescue!):
https://cloud.dotplex.com/s/RHNGPL8xCzspCkr

Sneak peek:

Features

dragging and dropping of single or multiple mails
customized draggable-ghost to match the general style
- with count of mails being dragged, their subject and sender
visual feedback for invalid drop-targets (faded out)
collapsed folders of the same account expand automatically when starting to drag
support for subfolders
dragging to trash/junk is possible
source and target folders are synced right away once the mails have been dropped

Code

This is dependency-free. I am not using any external libraries for this. I dug into the HTML5 drag and drop API. I've studied #2834 and the library that was used there and figured out that in order for this to work, event listeners would have to be attached in a more dynamic manner, since mails are loaded asynchronously. I was trying to make it work with vue-draggable, as well as some other libraries, but each of them would spawn a different set of challanges. So in the end I decided to do it "by hand", which has the huge benefit of being able to tailor the feature exactly to our needs.

It's a draft!

In order to get this to work, I had to do some low-level attaching of eventListeners and dom manipulations (to create the draggable-ghost). It's basically exactly what vue-draggable does, just with less overhead. I would love to wrap that up in two custom directives (one for the draggables, one for the drop-targets), but I've never done that before and ran into some issues. So, before pouring more time into this, I wanted to make sure that this feature would be accepted in the first place.

How does this work?

There are dragstart and dragend eventListeners attached to an envelope in Envelope.vue. On the other hand, there are dragenter, dragover, dragleave and drop eventListeners on each folder/mailbox in NavigationMailbox.vue that matches the criteria for a valid drop-target.

On dragstart, each envelope being dragged is added to the global $store and a drag-ghost is created. The HTML5 API to customize that is kinda wierd... you have to create an element to your liking and attach it to the DOM. The browser will then take a "screenshot" of that element, at which point you can remove the element from the DOM again. So in practice: you create the element, attach it to the dom, set it as a DragImage and remove it right away. 🤷‍♂️

Once you drop the mail(s) on a valid drop target (mailbox), the mails to be moved are pulled from the $store and processed. It would be possible to make them disappear right away, but I went with a slight fade-out until they are actually moved. In theory, it would be possible to use the HTML5 DataTransfer object instead of the $store, but that API is pretty wierd as well. Good luck passing objects... :/

Improvements

Code isolation – as stated before. Maybe using custom directives? Glad for any suggestion.
Styles: I blatently stuck the styling for the draggable-ghost into App.vue, as it needs to be global. Of course, that's not the right place. How/where is mail.scss compiled? I'd like to add it there.

Trojan Horse 🐴

This commit has nothing to do with the feature, but it neccessary for it to work. I need to pass an event from an envelope to a mailbox/folder. Apparently there is an event bus, but it is created in MailboxThread.vue and only passed down as a prop to its' subcomponents. Instead, I am proposing this pattern to make it available whereever neccessary. I'd submit this as a separate PR once out of draft-phase.

Conclusion

I strongly believe that this is a must have for an email client. Every user will try to drag an email. We are just used to it from any other client out there. However, I am not married to the specifics of this implementation and am glad for any help with this!

st3iny · 2020-11-10T12:24:25Z

Awesome! In my opinion this is a huge usability improvement. I will look at the code and test as soon as I can :)

ChristophWurst · 2020-11-10T14:11:29Z

🤯

ChristophWurst · 2020-11-10T14:12:53Z

@jancborchardt please have a look at the design :)

brueckner · 2020-11-12T13:28:11Z

So, does anyone happen to have an opinion on this?

It's a draft!

In order to get this to work, I had to do some low-level attaching of eventListeners and dom manipulations (to create the draggable-ghost). It's basically exactly what vue-draggable does, just with less overhead. I would love to wrap that up in two custom directives (one for the draggables, one for the drop-targets), but I've never done that before and ran into some issues. So, before pouring more time into this, I wanted to make sure that this feature would be accepted in the first place.

I'm eager to put time into optimizations, but would like to put that time into the "right" thing.

st3iny

First of all, I really like your changes. Dragging works very well, feels nice and fits the design of nextcloud. The code looks clean overall but I commented on some minor things here and there.

The new global event bus is a great addition but might be out of scope of this PR. If this is blocking the review/merge we could move this to a follow up PR (but keep the uses in the dragging related code here). In my opinion we should definitely keep it (be it here or in another PR) as it increases our code quality. Currently, it is kind of hard to trace all the separate event buses scattered in our code.

When moving an envelope the thread is not updated. E.g. moving a collapsed envelope to another mailbox and then expanding this envelope in the thread will cause it to not load because the attached message/envelope does not exist anymore. I had a similar issue in my move modal code. You could use the new global event bus and update the thread after moving via this.fetchThread() or navigate away when the thread root was moved. Take a look at my solution https://github.com/nextcloud/mail/pull/3915/files#diff-ee27750a392035af626e82a2c822204689adf614396d005d18281faf701da601 (especially at the onMove() method)

The draggable ghost does not have a transparent background but this seems to be an issue with the proprietary nvidia linux drivers and compositing and is not related to your code.

EDIT: spelling

src/components/NavigationMailbox.vue

brueckner · 2020-11-12T19:16:15Z

src/App.vue

+// TODO: move to mail.scss
+// The drag-ghost is appended to the body
+// and cannot be styled from within a
+// component with scoped CSS


🤔 Where/How is mail.scss compiled though? It's not picked up by the default watcher process.

By Nextcloud server in the background. But we're trying to move away from this. Please try to put everything into a component scss so it's bundled with the app ✌️

src/store/actions.js

ChristophWurst · 2020-11-13T12:54:08Z

In theory, it would be possible to use the HTML5 DataTransfer object instead of the $store, but that API is pretty wierd as well. Good luck passing objects... :/

I remember facing the same problem when I once tried to revive drag and drop in an early version of Mail with Vue. Now that we streamlined our data structures a bit we don't have to pass around objects anymore because messages can be identified by unique integer IDs (was a combination of account id, mailbox name and message UID before). So, can we pass the array of IDs? Is that easier than an object? If a string works we can also serialize the ID array to a json string. 🤔

brueckner · 2020-11-16T08:46:55Z

It would be charming indeed to detach this thing from the store. My first approach was to attach a DataTransferItem for each Envelope in a DataTransferItemList. I remember it feeling a little hacky and it felt much cleaner using the store. However, passing a serialized array could simplify this a lot. I'm gonna look into it again and will report back.

brueckner · 2020-11-16T10:44:45Z

Okay, so this API is melting my brain. Apparently, you shouldn't be able to access the data attached to the dataTransfer object on dragover, but only in the drop event handler (for security reasons). Oddly enough, it DOES work in Firefox, but not in Webkit. Here's a StackOverflow reply that describes the issue: https://stackoverflow.com/a/31922258

That's a bummer, because I am trying to prevent the user from dropping the mail into the same folder it came from. People are recommending global variables in that case (so... the store in our case). 🤷‍♂️

FWIW – attaching a stringified JSON and parsing it in onDrop works like a charm!

The other reason the current implementation is relying on the global store is for each Mailbox in the sidebar to detect wether it is a valid drop target or not (and to disable/fade it out in that case). Without using the store, I'd have to emit an event to inform the mailboxes about the origin of envelopes being dragged instead. Which is... I don't know. It's just shifting the load from the store over to events. I prefer the data-driven approach using the store tbh.

What do you think?

ChristophWurst · 2020-11-18T15:52:22Z

Okay, so this API is melting my brain. Apparently, you shouldn't be able to access the data attached to the dataTransfer object on dragover, but only in the drop event handler (for security reasons). Oddly enough, it DOES work in Firefox, but not in Webkit. Here's a StackOverflow reply that describes the issue: https://stackoverflow.com/a/31922258

That's a bummer, because I am trying to prevent the user from dropping the mail into the same folder it came from. People are recommending global variables in that case (so... the store in our case). man_shrugging

Yeah guess we have to use the store then. Or we just don't do the move (and show a hint to the user via a Toast) if source=destination.

The other reason the current implementation is relying on the global store is for each Mailbox in the sidebar to detect wether it is a valid drop target or not (and to disable/fade it out in that case). Without using the store, I'd have to emit an event to inform the mailboxes about the origin of envelopes being dragged instead. Which is... I don't know. It's just shifting the load from the store over to events. I prefer the data-driven approach using the store tbh.

The dynamic drop target is required for the move from the unified/priority inbox to their real mailboxes, right? Because otherwise the current mailbox that is open should never be a valid drop target.

brueckner · 2020-11-19T16:26:28Z

Alright, I refactored it into two directives v-draggable-envelope and v-droppable-mailbox and moved most of the code into these, along with the new DragEventBus. All of these live in this folder structure:

- directives/
- - drag-and-drop/
- - - draggable-envelope/  <--- v-draggable-envelope
- - - droppable-mailbox/   <--- v-droppable-mailbox
- - - util/                <--- DragEventBus

I still need to move the styles, but the most important change is the extraction into directives.

Also: I am not using the store to temporarily persist the envelopes anymore. I am using local state within the directives in combination with the DataTransferObject: event.dataTransfer.setData('text/plain', JSON.stringify(envelopes)).

It works faily well like that!

I also still need to look into @st3iny 's comment:

When moving an envelope the thread is not updated. E.g. moving a collapsed envelope to another mailbox and then expanding this envelope in the thread will cause it to not load because the attached message/envelope does not exist anymore. I had a similar issue in my move modal code. You could use the new global event bus and update the thread after moving via this.fetchThread() or navigate away when the thread root was moved. Take a look at my solution https://github.com/nextcloud/mail/pull/3915/files#diff-ee27750a392035af626e82a2c822204689adf614396d005d18281faf701da601 (especially at the onMove() method)

Oh, there is one addition in terms of usability: when starting the drag, not only is the list of folders expanded, I am now also expanding all folders and their subfolders, which helps alot I think. When dragging is done, the subfolders are collapsed again.

I'm sure I forgot to mention something, but I am stoked about the fact that I managed to separate most of the functionality into these two directives. 🥳 Could you please have a look at that?

brueckner · 2020-11-20T10:49:22Z

Okay, I'm thrilled to announce that I think I'm done. 🔥

The issue @st3iny mentioned (thanks for that by the way, I actually didn't notice that myself!) is resolved here: 173201d. I decided to check if the user is currently viewing a message and if that threadId is in the list of moved messages. If it is, I'm navigating to the mailbox root without selecting a message. I think that's better than opening the next message automatically, because it doesn't trigger a load that would potentially delay the next move-operation.

As for the styles, I moved all related styling to separate scss files within the directive's folder. To avoid potential styling collisions (since they're not scoped anymore), the classes and stylable attributes are now fairly specific.

I also refactored and moved around some code. The v-droppable-mailbox now emits an envelopesMoved event after the messages have been moved and puts the responsibility of what's supposed to happen afterwards back into the "hands" of the NavigationMailbox.

Oh, I also reverted my global EventBus changes, which were still lingering in there... ;-)

It would be great if you took a final look at the whole thing one more time, @st3iny and @ChristophWurst, before I bring it up to speed with master and squash everything.

jancborchardt

Really great design-wise @brueckner, amazing work! :)

ChristophWurst

Rest of the code looks good. Still have to wrap my head around those directives and the other logic but it looks solid :)

src/components/Envelope.vue

src/components/EnvelopeList.vue

src/components/NavigationMailbox.vue

src/directives/drag-and-drop/draggable-envelope/draggable-envelope.js

brueckner · 2020-11-23T08:31:56Z

Thanks for the review @ChristophWurst, I've implemented all of the remarks. I'll hit the "Ready for review" button to move it out of drafting phase. 🚀

ChristophWurst · 2020-11-25T14:55:48Z

Please rebase and squash this awesomeness into a single commit, then we can merge 🚀

brueckner · 2020-11-25T15:16:12Z

Ready to roll! 🚀 🥳

ChristophWurst · 2020-11-26T11:00:55Z

src/components/NavigationMailbox.vue

+			if (this.mailbox.databaseId !== mailboxId) {
+				return
+			}
+			this.$store.dispatch('syncEnvelopes', { mailboxId })


I think we don't need/want that, do we? The destination mailbox is always a different one than the current mailbox. And we don't care about keeping another mailbox updated (on the front-end).

@GretaD found out that this action can run into

mail/lib/Controller/MailboxesController.php

Line 171 in f37124c

return new JSONResponse([], Http::STATUS_PRECONDITION_REQUIRED);

. That is, a sync request is sent for a mailbox that had never been sync'ed before. In this case the js code is actually supposed to do a full initialization sync request. So, that is of course out of scope for the drag and drop feature, but this dispatched action could potentially carry out a very expensive operation because the initial sync isn't happening fast for larger mailboxes.

Hence I'd say we remove the explicit sync of the destination mailbox for now. When the user navigates there, the background sync will update the mailbox anyway. #3951 is a bit of a missing piece here, but again, off topic 😉

@brueckner does that make sense or am I missing something?

Hmmm, I see the point. However, before I implemented triggering the sync it was pretty irritating to me to move a mail to its new destination and not seeing it when going to that mailbox right after the move. Sometimes it would take quite some time until the background sync hit. Until then – from the perspective of an unaware user – the mail is just... gone. I don't know about you, but I would be pretty angry if a mail app (seemingly) made my mails disappear 👀 . Also: wouldn't the background-sync essentially do the same expensive operation as well, just... delayed? ;)

(Disclaimer: I didn't look at the background sync yet) Couldn't we reset the timer of a background sync based on a move-operation's triggered sync or something along those lines?

OR... yes! Hear me out... What if we dropped the sync-trigger and marked every mailbox that was the target of a recent move-operation instead (something like outdated). If a user clicks on a mailbox that is marked outdated, a sync is triggered immediately. That way, the user will be greeted by a loading spinner and glad to find that all their mails have been moved successfully. On the other hand, all outdated mailboxes the user does not navigate to will not trigger a sync (until the regular background sync hits).

That way I can move a thousand mails to 20 different mailboxes while only ever triggering one sync once I navigate to one of the 20 outdated mailboxes. In addition to that, the background sync will remove the outdated flag, thus preventing an unnecessary sync.

👯

Are you describing #3951? 🤔

Ha! Hell yea! :)

So in terms of what's described in #3951 we could just disable the sync on any move-target-mailbox in the store without persisting that setting?

Well, wait. It would just have to be disabled for like 30 seconds, right? If the user has bg sync enabled, moves a mail into the folder and doesn't open the mailbox right away afterwards, the background sync should still to its' thing on regular schedule.

Btw we have the same problems/questions for drafts (they should show up if you switch mailbox) and also sent messages.

I'm all in for improving that. But can we keep those two things separate?

Yea sure – so should I just remove the sync-trigger? We'd just have to make sure to have a solution for it when this gets released, right?

We'd just have to make sure to have a solution for it when this gets released, right?

Absolutely. We can discuss this the next few days (maybe in a call) and then have a solid fix for the next release :)

Sound good to me! I'll remove the trigger :)

Removed, rebased on master and squashed. ✅

GretaD

It works 👍 see comment

Signed-off-by: Johannes Brückner <johannes@dotplex.com>

ChristophWurst

Works really well 🚀 🙏 👍

brueckner requested review from ChristophWurst, StCyr and st3iny November 10, 2020 11:57

st3iny added the enhancement label Nov 10, 2020

ChristophWurst added 3. to review feature parity labels Nov 10, 2020

ChristophWurst added this to the v1.8 milestone Nov 10, 2020

ChristophWurst requested review from jancborchardt and GretaD November 10, 2020 14:12

ChristophWurst added the priority:medium label Nov 10, 2020

st3iny reviewed Nov 12, 2020

View reviewed changes

brueckner force-pushed the enhancement/drag-and-drop-mails branch from 39f8c89 to 1ed3169 Compare November 13, 2020 07:50

brueckner mentioned this pull request Nov 13, 2020

Refactor: Global EventBus #4066

Closed

brueckner force-pushed the enhancement/drag-and-drop-mails branch 2 times, most recently from 1165912 to d7ecfb1 Compare November 19, 2020 16:14

brueckner force-pushed the enhancement/drag-and-drop-mails branch from 15fbb4e to a326bb1 Compare November 20, 2020 09:50

jancborchardt approved these changes Nov 20, 2020

View reviewed changes

ChristophWurst reviewed Nov 20, 2020

View reviewed changes

brueckner force-pushed the enhancement/drag-and-drop-mails branch 2 times, most recently from f2c36a0 to 404b0ad Compare November 23, 2020 08:29

brueckner marked this pull request as ready for review November 23, 2020 08:32

brueckner force-pushed the enhancement/drag-and-drop-mails branch from eeb5adc to b5c1a8f Compare November 25, 2020 15:14

ChristophWurst reviewed Nov 26, 2020

View reviewed changes

GretaD approved these changes Nov 26, 2020

View reviewed changes

Implement dragging and dropping mails into folders

6682c8e

Signed-off-by: Johannes Brückner <johannes@dotplex.com>

brueckner force-pushed the enhancement/drag-and-drop-mails branch from b5c1a8f to 6682c8e Compare November 30, 2020 08:15

ChristophWurst approved these changes Nov 30, 2020

View reviewed changes

ChristophWurst added 4. to release and removed 3. to review labels Nov 30, 2020

ChristophWurst merged commit 2f68cdc into nextcloud:master Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drag and Drop (revisited) #4048

Drag and Drop (revisited) #4048

brueckner commented Nov 10, 2020 •

edited

Loading

st3iny commented Nov 10, 2020

ChristophWurst commented Nov 10, 2020

ChristophWurst commented Nov 10, 2020

brueckner commented Nov 12, 2020

It's a draft!

st3iny left a comment •

edited

Loading

This comment was marked as outdated.

brueckner Nov 12, 2020

ChristophWurst Nov 13, 2020

ChristophWurst commented Nov 13, 2020

brueckner commented Nov 16, 2020

brueckner commented Nov 16, 2020

ChristophWurst commented Nov 18, 2020

brueckner commented Nov 19, 2020

brueckner commented Nov 20, 2020

jancborchardt left a comment

ChristophWurst left a comment

brueckner commented Nov 23, 2020

ChristophWurst commented Nov 25, 2020

brueckner commented Nov 25, 2020

ChristophWurst Nov 26, 2020

brueckner Nov 26, 2020 •

edited

Loading

ChristophWurst Nov 27, 2020

brueckner Nov 27, 2020

brueckner Nov 27, 2020

ChristophWurst Nov 27, 2020

brueckner Nov 30, 2020

ChristophWurst Nov 30, 2020

brueckner Nov 30, 2020

brueckner Nov 30, 2020

GretaD left a comment

ChristophWurst left a comment

Drag and Drop (revisited) #4048

Drag and Drop (revisited) #4048

Conversation

brueckner commented Nov 10, 2020 • edited Loading

Adds drag & drop support for moving emails to folders

Features

Code

It's a draft!

How does this work?

Improvements

Trojan Horse 🐴

Conclusion

st3iny commented Nov 10, 2020

ChristophWurst commented Nov 10, 2020

ChristophWurst commented Nov 10, 2020

brueckner commented Nov 12, 2020

It's a draft!

st3iny left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented Nov 13, 2020

brueckner commented Nov 16, 2020

brueckner commented Nov 16, 2020

ChristophWurst commented Nov 18, 2020

brueckner commented Nov 19, 2020

brueckner commented Nov 20, 2020

jancborchardt left a comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

brueckner commented Nov 23, 2020

ChristophWurst commented Nov 25, 2020

brueckner commented Nov 25, 2020

Choose a reason for hiding this comment

brueckner Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GretaD left a comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

brueckner commented Nov 10, 2020 •

edited

Loading

st3iny left a comment •

edited

Loading

brueckner Nov 26, 2020 •

edited

Loading