Page MenuHomePhabricator

Make event passing between LightboxInterface and MultimediaViewer non-global
Closed, ResolvedPublic

Description

Migrated from: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/277

...because it is nearly impossible to write tests that do not bleed into each other right now.

Event Timeline

MingleTerminator raised the priority of this task from to Needs Triage.
MingleTerminator added a project: Multimedia.
In mingle on 2014-03-04 at 09:24:00, @Gilles wrote:

The point of having events that don't have to be pushed to a specific element or listened to on a specific element is decoupling. Maybe some events we currently have can be removed. Maybe we can use a different event system than one that goes through the DOM. But it sounds like you want to go back to targeting specific DOM elements, which is counter-productive in terms of making these events useful for 3rd-parties. At any rate your proposed change should not make use of a reference to the viewer object from the interface. These back-references are what made this codebase horrible to begin with.

In mingle on 2014-03-04 at 12:42:13, @Gilles wrote:

Since #278 is done now, I'd argue that this change isn't necessary anymore. The problem that prompted it (stray instances catching events in tests) won't happen after the changeset in #278 is merged.

In mingle on 2014-03-04 at 19:28:00, @Tgr wrote:

Thanks for doing #278 ! That was the main motivation about this change, but I think it has some merit even after that. The current event system uses a global channel (the window), with the usual benefits and costs of globals: always conveniently available, but cannot be mocked or isolated.

I agree that using the DOM would be a step back, but jQuery can pass events outside it with almost zero added complexity. I won't continue work on this unless we have consensus that it is useful, but I'll upload the work already done yesterday, that will give a better picture of how I imagined it.

Gilles triaged this task as Low priority.Dec 8 2014, 4:57 PM
Gilles added a subscriber: Gilles.

Change 173916 had a related patch set uploaded (by Gergő Tisza):
Turn mmv-next/mmv-prev into OO events

https://gerrit.wikimedia.org/r/173916

Patch-For-Review

Tgr removed Tgr as the assignee of this task.Dec 23 2014, 11:22 PM
Tgr added a subscriber: Tgr.
Jdforrester-WMF moved this task from Untriaged to Backlog on the Multimedia board.Sep 4 2015, 5:55 PM
Restricted Application added a subscriber: Matanya. · View Herald TranscriptSep 4 2015, 5:55 PM

Mass-removing the Multimedia tag from MediaViewer tasks, as this is now being worked on by the Reading department, not Editing's Multimedia team.

@Gilles can you rebase or abandon the patch?

Change 173916 merged by jenkins-bot:
Turn mmv-next/mmv-prev into OO events

https://gerrit.wikimedia.org/r/173916

Jdlrobson closed this task as Resolved.Sep 29 2015, 7:19 PM
Jdlrobson claimed this task.