search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] New, more robust event-dispatching system #8819

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by peterflynn Thursday Nov 13, 2014 at 09:17 GMT Originally opened as https://github.com/adobe/brackets/pull/9918

(from backlog card Event Infrastructure Improvements)

Instead of jQuery, use our own simpler event system for non-DOM objects.

This initial demonstration converts Document & DocumentManager to use the new system, plus unit tests to verify more of its functionality.

CC@dangoor -- this was the experiment I mentioned back at MAX.

peterflynn included the following code: https://github.com/adobe/brackets/pull/9918/commits

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Nov 18, 2014 at 02:04 GMT

@redmunds Changes pushed -- and now it's the full diff.

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Nov 18, 2014 at 02:16 GMT

A few caveats to watch out for:

This also removes a couple old things that I'm pretty sure are safe to get rid of:

core-ai-bot commented 3 years ago

Comment by redmunds Tuesday Nov 18, 2014 at 16:35 GMT

I'm seeing these KeyBindingManager unit tests fail consistently in this branch (but not in master) on Win7:

should update key map with the user specified key bindings

Error: Expected {  } to equal { Ctrl-L : { key : 'Ctrl-L', displayKey : 'Ctrl-L', explicitPlatform : undefined, commandID : 'edit.selectLine' }, Ctrl-Alt-L : { key : 'Ctrl-Alt-L', displayKey : 'Ctrl-Alt-L', explicitPlatform : undefined, commandID : 'edit.splitSelIntoLines' }, Alt-Shift-Down : { key : 'Alt-Shift-Down', displayKey : 'Alt-Shift-↓', explicitPlatform : undefined, commandID : 'edit.addCursorToNextLine' }, Alt-Shift-Up : { key : 'Alt-Shift-Up', displayKey : 'Alt-Shift-↑', explicitPlatform : undefined, commandID : 'edit.addCursorToPrevLine' }, F8 : { key : 'F8', displayKey : 'F8', explicitPlatform : undefined, commandID : 'navigate.gotoFirstProblem' }, Ctrl-Alt-O : { key : 'Ctrl-Alt-O', displayKey : 'Ctrl-Alt-O', explicitPlatform : undefined, commandID : 'file.openFolder' }, Ctrl-Alt-H : { key : 'Ctrl-Alt-H', displayKey : 'Ctrl-Alt-H', explicitPlatform : undefined, commandID : 'view.hideSidebar' }, Ctrl-Shift-O : { key : 'Ctrl-Shift-O', displayKey : 'Ctrl-Shift-O', explicitPlatform : undefined, commandID : 'navigate.quickOpen' }, Ctrl-T : { key : 'Ctrl-T', displayKey : 'Ctrl-T', explicitPlatform : undefined, commandID : 'navigate.gotoDefinition' } }.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:114:32)
    at null.toEqual (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1235:29)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/KeyBindingManager-test.js:673:59)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1064:17)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)
    at jasmine.Queue.start (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2049:8)
    at jasmine.Spec.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2376:14)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2092:18)
    at jasmine.Spec.finish (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2350:5)

should restore original key bindings when the user key map is updated

Error: Expected {  } to equal { Ctrl-L : { key : 'Ctrl-L', displayKey : 'Ctrl-L', explicitPlatform : undefined, commandID : 'edit.selectLine' }, Ctrl-Alt-L : { key : 'Ctrl-Alt-L', displayKey : 'Ctrl-Alt-L', explicitPlatform : undefined, commandID : 'edit.splitSelIntoLines' }, Alt-Shift-Down : { key : 'Alt-Shift-Down', displayKey : 'Alt-Shift-↓', explicitPlatform : undefined, commandID : 'edit.addCursorToNextLine' }, Alt-Shift-Up : { key : 'Alt-Shift-Up', displayKey : 'Alt-Shift-↑', explicitPlatform : undefined, commandID : 'edit.addCursorToPrevLine' }, F8 : { key : 'F8', displayKey : 'F8', explicitPlatform : undefined, commandID : 'navigate.gotoFirstProblem' }, Ctrl-Alt-O : { key : 'Ctrl-Alt-O', displayKey : 'Ctrl-Alt-O', explicitPlatform : undefined, commandID : 'file.openFolder' }, Ctrl-Alt-H : { key : 'Ctrl-Alt-H', displayKey : 'Ctrl-Alt-H', explicitPlatform : undefined, commandID : 'view.hideSidebar' }, Ctrl-Shift-O : { key : 'Ctrl-Shift-O', displayKey : 'Ctrl-Shift-O', explicitPlatform : undefined, commandID : 'navigate.quickOpen' }, Ctrl-T : { key : 'Ctrl-T', displayKey : 'Ctrl-T', explicitPlatform : undefined, commandID : 'navigate.gotoDefinition' } }.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:114:32)
    at null.toEqual (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1235:29)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/KeyBindingManager-test.js:711:59)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1064:17)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)
    at jasmine.Queue.start (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2049:8)
    at jasmine.Spec.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2376:14)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2092:18)
    at jasmine.Spec.finish (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2350:5)
TypeError: Cannot read property 'commandID' of undefined
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/spec/KeyBindingManager-test.js:738:50)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1064:17)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2096:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2092:18)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2537:5
core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Nov 18, 2014 at 18:01 GMT

Ah, I see -- the unit test window never triggers htmlReady(), so KeyBindingManager never adds its CommandManager listener when running in the test window. So it's a unit-test-only problem. The test window does trigger appReady(), though, so it's sort of a bizarre arrangement... Fixing that would fix the test, but I'm a little hesitant about whether that has any fallout for existing tests.

It seems a bit safer & cleaner to just add the "safe during init" version of on() that I mused about above, and use that instead of AppInit to work around the circular depdency issues. I'll file a spinoff cleanup bug about the weird unit test setup, though.

core-ai-bot commented 3 years ago

Comment by dangoor Tuesday Nov 18, 2014 at 20:58 GMT

One general comment: given that we have JavaScript code hints, I actually think that the signals pattern is preferable. Rather than something like FooManager.on("fooChange", ...) you'd do FooManager.fooChange.add(...) which benefits from code hints and also fails early if there's a typo. Of course, extension authors wouldn't get the code hints benefit because of brackets.getModule.

The disadvantage, though, is that makes the circular dependency problem even more difficult because those signal names wouldn't be defined yet at module loading time.

@peterflynn I see what you mean about the PreferencesSystem. The easiest thing to do might just be to have the PreferencesSystem contain an object that is an EventDispatcher that is uses for handling the global change events. Each Preference would be an EventDispatcher. The signature of PreferencesSystem.on wouldn't change. Does that make sense?

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Nov 18, 2014 at 21:32 GMT

@redmunds Pushed changes that fix the unit tests; a fix for #9903; and a check that logs errors (and pauses the debugger) if anything in core is still using $() on EventDispatcher objects.

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Nov 18, 2014 at 21:44 GMT

@dangoor Interesting... I like the idea of not tossing around string literals as much, but it does seem like a significantly more disruptive change.

The circular-dependency issue would make the implementation uglier though (e.g. it'd have to be a wrapper around a more centralized implementation like the current one, or create a global queue for listeners that are waiting for the signal API to get initialized...).

But another issue is that tons of code currently relies on the jQuery ".namespace" mechanism, which doesn't map cleanly into this model -- what's the equivalent of obj.off(".foo") (i.e. across all events)? If that's impossible, complicates migration and is a step backward in maintainability.

The other thing is that, of the two benefits -- code hints and fail-fast -- we can actually get the latter pretty easily with the current API design too. (And we could make it warn-fast instead of fail-fast, to reduce bug risk during the migration -- something not possible with the signals pattern). So I'm unsure whether code hints alone are a strong enough reason to bite off the larger complexity of that pattern...

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Nov 18, 2014 at 21:44 GMT

I like the PreferencesSystem suggestion though -- will implement that now.

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday Nov 18, 2014 at 22:08 GMT

@dangoor Hmm the disadvantage of using a contained dispatcher object for PreferencesSystem is that event.target will be wrong -- although it's very unlikely anyone is using it, it's a weird little caveat. We could add a private API for trigger() that lets you substitute a different target, but that's still a little ugly too.

The other approach I'd thought of would be something like this:

PreferencesSystem.prototype._on_internal = EventDispatcher's impl of on() PreferencesSystem.prototype.on = prefs-specific 3-arg version, calling pref.on() or this._on_internal() depending on that extra 3rd arg

The only messiness there is that makeEventDispatcher() wants to overwrite on() specifically, so we either have to do a 3-step switcharoo (save off EventDispatcher's copy, then re-overwrite on() with the 3-arg impl) or provide another back-door API to pass custom method names to makeEventDispatcher(). (This is a tad messier since PreferencesBase.js assigns prototype members by overwriting the whole prototype object, instead of one-by-one as in most of our codebase).

core-ai-bot commented 3 years ago

Comment by TomMalbran Wednesday Nov 19, 2014 at 02:06 GMT

Just something I was thinking about.

Instead of making the class an event dispatcher, would it make sense to manage the events on the EventDispatcher module. What I mean is, what if we could use the dispatcher like EventDispatcher.get("DocumentManager").on(). We could then easily make EventDispatcher.DocumentManager.on() and DocumentManager.on() as functions that call the first form.

The disadvantage I see in the third form is that you need to require the module to use the events, and many times, you end up adding a new dependency, and even worst, you could create circular dependencies. The second form would just let you require 1 module and use all the events, but the dispatchers needs to be registered. The final form is useful to times where Emitter.DocumentManager is undefined. This makes it possible to register a listener and start listening as soon as the dispatcher is registered.

core-ai-bot commented 3 years ago

Comment by dangoor Wednesday Nov 19, 2014 at 03:23 GMT

@peterflynn Agreed about signal style being more disruptive, and that's a good point about .off(".foo").

Regarding PreferencesSystem, I wonder if we could just get rid of the special form of "on". There are a small number of uses of that in the core code and I see only 3 extensions (brackets-documents-toolbar, brackets-uitheming, Brackets-Themes) that appear to use that style.

We could use a hacky internal API to start with and deprecate the three argument form or if we thought we could get the extension authors to switch their code now, we can just break it.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Nov 20, 2014 at 02:47 GMT

@TomMalbran I think we did talk about a centralized pub-sub sort of model a long time ago. One downside in my mind is that it's a much bigger shift for events on instances (e.g. Document objects). Some code that deals with instances would have to become much more careful about pairing of on()-off() calls, or it would start leaking memory. There are also both pros & cons to a system that lets you register for events without ever explicitly indicating a dependency on the thing that will produce the events.

As with@dangoor's suggestion above, it's not necessarily bad but it is significantly more disruptive of a migration. My main goal here is to provide greater robustness against buggy extensions ASAP, in a way that's close to 100% backwards compatible so we can land it quickly without breaking lots of extensions. Switching to a different model altogether may well have maintainability benefits, etc., but it feels a lot more urgent to get robustness improvements in sooner -- since we're seeing such a rise in problems resulting from extension bugs.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Nov 20, 2014 at 03:01 GMT

@dangoor 9a3ee3d has the PreferencesSystem conversion -- lmk if you have any concerns about that setup.

core-ai-bot commented 3 years ago

Comment by TomMalbran Thursday Nov 20, 2014 at 03:20 GMT

@peterflynn Is good that you talked about it and came with the best solution. I was actually thinking of creating a new instance of an Event class for each registered module, and then just store them internally in an object where the key is the name of the module. But the same could be done with the current model if we save in an object the references to the modules.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Nov 20, 2014 at 04:43 GMT

@redmunds Changes pushed that I think address all your remaining comments. This link shows just the diff since your review last night: https://github.com/adobe/brackets/compare/e1e825270ff5d2f75dfb4c458f890e473d042b4f...110b532

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Nov 20, 2014 at 04:52 GMT

@TomMalbran Ah, so you're suggesting going through EventDispatcher.DocumentManager.on() for module-level events, but using doc.on() directly on instance objects? Or am I misunderstanding?

core-ai-bot commented 3 years ago

Comment by TomMalbran Thursday Nov 20, 2014 at 05:01 GMT

@peterflynn Something like this for modules:

EventDispatcher.DocumentManager.on = function () {
    DocumentManager.on();
};

And similar with all the other functions. You could either use DocumentManager.on() or EventDispatcher.DocumentManager.on().

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Nov 20, 2014 at 05:29 GMT

@TomMalbran But then what would you do for things that can have multiple instances, like Document objects?

core-ai-bot commented 3 years ago

Comment by TomMalbran Thursday Nov 20, 2014 at 05:32 GMT

For those you would have to use doc.on() as you said. You still need to get the document to add listeners so it makes sense to just use it like that.

core-ai-bot commented 3 years ago

Comment by redmunds Friday Nov 21, 2014 at 17:30 GMT

@peterflynn This looks good to me. Are there any more changes regarding conversation with@TomMalbran or should I merge it?

core-ai-bot commented 3 years ago

Comment by peterflynn Sunday Nov 23, 2014 at 02:39 GMT

@TomMalbran Are you ok with merging this as-is? I'd like to get the robustness improvements in for 1.1, and the smaller the API change the easier that will be.

I guess I'm also not sold on the EventDispatcher.DocumentManager.on() pattern. I don't think it would let us remove many require() dependencies, since without a dependency the event-emitter may load later than the listener, leaving the EventDispatcher.* properties null in the meantime. So in practice you'd almost always have to either keep the require() (losing that advantage) or use a different, less clean API to do deferred listener attachment. It also has the downside of making the event APIs less consistent between modules &. objects, which could be confusing...

core-ai-bot commented 3 years ago

Comment by TomMalbran Sunday Nov 23, 2014 at 02:48 GMT

Yes. We can merge it like this. We can always change it later, since I wasn't thinking of removing the DocumentManager.on(), just a way of removing dependencies.

We can use something like using EventDispatcher.on("DocumentManager", callback) to fix the null issue. If the the events are still not defined for DocumentManager, we can just store the callbacks and once it gets registered, we would add the stored listeners. This API is less cleaner but as it not replacing the use of DocumentManager.on(), it can be used only in the core code.

core-ai-bot commented 3 years ago

Comment by redmunds Sunday Nov 23, 2014 at 16:14 GMT

Merging.