New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Undo event deletion #149

Closed

georgehrke wants to merge 2 commits into master from undo_event_deletion

Member

georgehrke commented Oct 22, 2016

fix #13

please review @nextcloud/calendar

georgehrke added the 3. to review label

mention-bot commented Oct 22, 2016

@georgehrke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tcitworld, @MathieuSchopfer and @raghunayyar to be potential reviewers.

georgehrke mentioned this pull request

add undo for calendar deletion #148

Closed

coveralls commented Oct 22, 2016

Coverage increased (+0.1%) to 43.02% when pulling 66b9143 on undo_event_deletion into 03716b8 on master.

4 similar comments

coveralls commented Oct 22, 2016

Coverage increased (+0.1%) to 43.02% when pulling 66b9143 on undo_event_deletion into 03716b8 on master.

coveralls commented Oct 22, 2016

Coverage increased (+0.1%) to 43.02% when pulling 66b9143 on undo_event_deletion into 03716b8 on master.

coveralls commented Oct 22, 2016

Coverage increased (+0.1%) to 43.02% when pulling 66b9143 on undo_event_deletion into 03716b8 on master.

coveralls commented Oct 22, 2016 •

edited

Loading

Coverage increased (+0.1%) to 43.02% when pulling 66b9143 on undo_event_deletion into 03716b8 on master.

Member

tcitworld commented Oct 23, 2016

Will have a look asap.

tcitworld reviewed

View reviewed changes

js/app/models/fcEventModel.js

+              					}
+              				}, 7500);
+              				const elm = OC.Notification.showTemporary(t('calendar', 'Undo deletion'));

Member

tcitworld Oct 23, 2016

I'll prefer "The event has been deleted. Undo ?"

Member Author

georgehrke Oct 23, 2016

@jancborchardt ^

Member

jancborchardt Oct 25, 2016

Yes, for sure. The pattern is:

eventname has been deleted. Undo?

js/app/models/fcEventModel.js

+              					if (context.isDeleted) {
+              						resolve();
+              					}
+              				}, 7500);

Member

tcitworld Oct 23, 2016

7.5 seconds is too high IMHO. The user has got plenty of time to switch to another app, resulting in the deletion action not executing (correct ?).

I don't know if there's a way to execute queue actions when we're leaving the app. Leaving the browser could be handled though, right ?

Member Author

georgehrke Oct 23, 2016

Good point!

Member Author

georgehrke Oct 23, 2016

(https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload)

Member

tcitworld Oct 23, 2016

Seems good. So undo actions should be in a queue that gets emptied on timeout or on the beforeunload event.

Member

jancborchardt commented Oct 25, 2016

(@georgehrke just btw: is it possible to reduce the Coveralls comments or not have them at all? We have the summary on the bottom with all the other checks, so the comments are just noise.)

Member Author

georgehrke commented Oct 25, 2016

Coverall is only supposed to send one comment per commit pushed. :/
It's going crazy lately.
I'll disable the comments

Member

jancborchardt commented Oct 25, 2016

(Yeah, as said disabling is better than commenting because we have the summary here on the bottom anyway. Thanks!)

wrexroad reviewed

View reviewed changes

js/app/controllers/calcontroller.js

+              							angular.forEach(eventsToRender, function (event) {
+              								fc.elm.fullCalendar('renderEvent', event);
+              							});
+              						});

Member

wrexroad Oct 26, 2016 •

edited

Loading

Rerendering undeleted events like this causes them to double render when another event is created. I know that is a confusing way to word it so I took a short video.

Fortunately, this is simply a rendering artifact and resolves itself on reload.

If you rerender the events like this:
fc.elm.fullCalendar('refetchEventSources', vevent.calendar.fcEventSource);
it does not appear to cause this problem.

Member Author

georgehrke Nov 8, 2016

Good catch!

I tried using refetchEventSources, but I think it's a bit problematic. The event takes at least one second to reappear, because the browser has to send a CalDAV request to the server.
It would be way better, if the event would reappear immediately.


          fix timezone service test

ce14199

georgehrke force-pushed the undo_event_deletion branch from 66b9143 to 24683d1 Compare

November 8, 2016 13:53

georgehrke added 2. developing and removed 3. to review labels


          undo event deletion

10a0eb8

georgehrke force-pushed the undo_event_deletion branch from 24683d1 to 10a0eb8 Compare

November 8, 2016 20:49

Member Author

georgehrke commented Nov 8, 2016

I rebased, changed the string and fixed the issue mentioned by @wrexroad
please check again :)

georgehrke added 3. to review and removed 2. developing labels

Member Author

georgehrke commented Nov 8, 2016

The mysql and postgres builds failed, because it couldn't reach the database.
The sqlite tests passed fine.

Member

wrexroad commented Nov 8, 2016

Two thoughts:

It looks like the double rendering is still an issue, just in a different way. If you undelete an event, then hide the calendar to which it belongs, the event is not hidden. Then, when you unhide the calendar the event gets double rendered.
It looks like the onbeforeunload event listener was never added.

Member Author

georgehrke commented Nov 8, 2016 •

edited

Loading

It looks like the onbeforeunload event listener was never added.

Yes, I want to solve that at another point. We can't send requests in onbefureonload, because the requests are async. The browser won't wait for them to finish but just cancel them.
Making them synchronous is an absolute no-go, because it would block any user interaction with the app and we would also have to patch one library we use.

Member

wrexroad commented Nov 8, 2016 •

edited

Loading

I think ensuring events actually get deleted is really important. Think of the situation where someone knows that the just need to quickly load up the page, delete an event, and close the browser. If they don't know that they have to wait 7 seconds before closing the browser it will create confusion.

I feel like the worst part of it is the event instantly disappears, giving a visual cue that the event is, in fact, gone. How would you feel about changing the notification to be a countdown. Something like "event name will be deleted in 3,2,1s. Click to cancel." Only when the timer runs out will you remove the event from the calendar and delete it from the server. This also solves the double rendering issue.

Another idea is as soon as the delete button is clicked dump the event properties to temporary location then immediately delete the event from the server and clear the event from the calendar. Then if the undo button is clicked you can create a new event using the properties that were saved. If the button is not clicked you dump the temporary storage.

Member Author

georgehrke commented Nov 8, 2016

I think ensuring events actually get deleted is really important. Think of the situation where someone knows that the just need to quickly load up the page, delete an event, and close the browser. If they don't know that they have to wait 7 seconds before closing the browser it will create confusion.

The only thing we can do with onbeforeunload is to ask the user if he really wants to leave the page.
And the problem is that modern browsers don't allow custom messages anymore :/

Another idea is as soon as the delete button is clicked dump the event properties to temporary location then immediately delete the event from the server and clear the event from the calendar. Then if the undo button is clicked you can create a new event using the properties that were saved. If the button is not clicked you dump the temporary storage.

AFAIK all invitations will be send out again in this case.
That's also not really the expected behavior :/

Member

wrexroad commented Nov 8, 2016 •

edited

Loading

AFAIK all invitations will be send out again in this case.

Yikes that's no good at all. I hadn't even considered invitations...

Member Author

georgehrke commented Nov 10, 2016

@jancborchardt What's your opinion about the leaving the page thing?

I checked what files does, but the undo is completely gone even if files_trashbin is disabled.

Member

eppfel commented Nov 13, 2016

It would be awesome if you could cue jobs on the server, like "delete event x in 7 seconds, if you don't here a cancel message" Just a thought...

Member Author

georgehrke commented Nov 13, 2016

It would be awesome if you could cue jobs on the server, like "delete event x in 7 seconds, if you don't here a cancel message" Just a thought...

Hm, what if you are traveling in a train? I wouldn't exactly trust the cancel request to make it's way to the server (in time) ;)

Regarding the server: I'm also working on a retention feature: nextcloud/server#1662

georgehrke added 1. to develop and removed 3. to review labels

georgehrke mentioned this pull request

Add confirm() when deleting a calendar/event #341

Closed

Member Author

georgehrke commented Feb 27, 2018

This approach is at a dead end thats only gonna end up in a huge chain of ugly hacks.
The proper solution is gonna be nextcloud/server#1662

georgehrke closed this

georgehrke deleted the undo_event_deletion branch

February 27, 2018 08:31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels