944804 - Editor stays open when trackevent is removed by a duration change

Status: RESOLVED FIXED

(Webmaker Graveyard :: Popcorn Maker, defect)

Description

[Matthew Schranz [:mjschranz]]

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.

Comment 1

[Matthew Schranz [:mjschranz]]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/361

Comment 2

[Scott [:thecount] Downe]

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?

Comment 3

[Matthew Schranz [:mjschranz]]

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 4

[Scott [:thecount] Downe]

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.

Comment 5

[Scott [:thecount] Downe]

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.

Comment 6

[Matthew Schranz [:mjschranz]]

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.

Comment 7

[Scott [:thecount] Downe]

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.

Comment 8

[Matthew Schranz [:mjschranz]]

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.

Comment 9

[Scott [:thecount] Downe]

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 10

[Scott [:thecount] Downe]

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?

Comment 11

[Matthew Schranz [:mjschranz]]

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

New thought process.

Comment 12

[[github robot]]

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