Editor stays open when trackevent is removed by a duration change

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjschranz, Assigned: mjschranz)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
STR:

1) Add an event.
2) Change the duration so it's less than the END value of that trackevent.

Expected: Editor closes.
Actual: Editor stays open.
A quick drive by in the pull request comments.

Although, don't we close the editor when a track event is removed? Then, doesn't removing the track event because of duration changes just push it into that path?
(Assignee)

Comment 3

5 years ago
The trackevent is handled just fine and won't magically stay in view but it's editor is not closed if it was open at the time. That's what this patch is fixing.

However, it may be more sensible to add a blanked event listener in the editor module to listen for "trackeventremoved" and close the editor from there instead. I redid the patch using this approach.
Comment on attachment 8341210 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/361

Now using this patch, if I delete a track event using the del key, it locks up. Let me know if you see this too.

This is how we currently close the editor: https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/src/ui/ui.js#L518

We should either move the editor close to happen inside the removeTrackEvent, or like you're doing, inside trackeventremoved. Either way, remove the editor close from inside ui.js, two places I see, and inside trackHandles.js, with one place.
Attachment #8341210 - Flags: review?(scott) → review-
I think the lock up was from a breakpoint I had that I forgot about, and didn't have my debugger in focus. So, you can probably ignore that.
(Assignee)

Comment 6

5 years ago
Comment on attachment 8341210 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/361

I can't make it literally apart of removeTrackEvent because it relies on having access to the editor module (in other words, butter).

This seems like the best solution otherwise, a blanket "global" listener for trackeventremoved that closes any open trackevent editors.
Attachment #8341210 - Flags: review- → review?(scott)
Comment on attachment 8341210 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/361

Yeah, I'm fine with listening to removetrackevent.

However, it's closing the editor when I drag the track event it around.

We're close, I think.
Attachment #8341210 - Flags: review?(scott) → review-
(Assignee)

Comment 8

5 years ago
Comment on attachment 8341210 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/361

I *think* this change doesn't cause any cascading problems. Best I could tell the trackevent wasn't duplicated in anyway after drags.
Attachment #8341210 - Flags: review- → review?(scott)
It still manages to close it when dragging from one track to another.

I don't know if trackEventRemoved is what we want here.
Comment on attachment 8341210 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/361

OK, so making one shared path is probably not worth it, unless you can think of another way, but I still think using trackEventRemoved for this is dangerous.

Hvae we tried closing the editor right here: https://github.com/mjschranz/popcorn.webmaker.org/blob/master/public/src/core/media.js#L403

Maybe that's better?
Attachment #8341210 - Flags: review?(scott) → review-
(Assignee)

Comment 11

5 years ago
Comment on attachment 8341210 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/361

New thought process.
Attachment #8341210 - Flags: review- → review?(scott)
Attachment #8341210 - Flags: review?(scott) → review+

Comment 12

5 years ago
Commit pushed to master at https://github.com/mozilla/popcorn.webmaker.org

https://github.com/mozilla/popcorn.webmaker.org/commit/ca5ed30e5d5b2708c1d0d5c250a9d2d05c84f1cb
Fix Bug 944804 - Tie trackevent editor closing to trackevent removal rather than a separate call

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.