[WIP] Plugin refactoring #444
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -321,33 +86,26 @@ if not g.Node? | |||
| DOCUMENT_FRAGMENT_NODE: 11 | |||
| NOTATION_NODE: 12 | |||
|
|
|||
| # Create namespace object for core-provided plugins | |||
There was a problem hiding this comment.
I don't think so. This is the browser entry point file. Even if you only want AnnotatorCore you're going to refer to it as Annotator.Core.AnnotatorCore so I'd rather keep the Core module side-effect free.
|
It's hard to find anything not to like about this, @nickstenning. Great work! The default UI plugin turned out very much like what I was thinking about back in my gist but I like it even more with the
|
nickstenning
force-pushed
the
plugin-refactoring
branch
from
c51d219
to
9775ce8
Compare
Here's another attempt (or start of one) to improve on the plugin
architecture of Annotator. I've stripped away a lot of complexity,
including:
- Factory (gone completely)
- Annotator.Plugin.register (Annotator.Plugin is just a plain old object
again)
But the most important change is the addition of src/core.coffee, which
contains AnnotatorCore. AnnotatorCore is:
- where plugins and storage implementations are registered
- responsible for propagating hook actions ('onAnnotationCreated') to
plugin instances
- able to tear down an Annotator instance
That's all at the moment, but that leaves the Annotator class itself to
be responsible *only* for setting up a "default Annotator application"
-- the thing that most users have been using throughout the 1.2.x
series.
I've commented out a bunch of plugin tests -- the ones that still rely
on nasty direct inter-plugin dependencies.
This is not a registry of any description. Rather, it is an interface adapter which wraps a concrete storage plugin implementation. This commit also bulks up the tests for this component quite substantially.
Storage plugins can now return values from their methods, so NullStore doesn't need to mess with Deferreds.
Moved TextSelector and Adder into a UI module, as they are not "plugins" in any meaningful sense (they do not respond to Annotator lifecycle events).
This commit shows the way for a couple of new patterns. First, the way that the DefaultUI plugin is responsible for composing the new UI components should be beginning to be clear. Second, the division of the Highlighter into a domain-specific Highlighter component in the UI module, as well as a thin wrapper plugin that can, if needed, be used on its own.
Just as with the Highlighter in a13e72d, this commit moves the bulk of the Editor functionality into the UI module. There remains a skeletal Editor plugin, which admittedly isn't terribly useful on its own, but it is included for completeness. As part of this work, the #load() method of the Editor was refactored to return a promise (resolving if the editor is submitted, rejecting if cancelled) rather than having a "private" event handler on the Editor itself.
Moves the bulk of the Viewer functionality into a decoupled object that lives in the UI module. The Viewer plugin becomes a thin wrapper that wires up aspects of the viewer's behaviour to the relevant registry functions.
This commit further excises unnecessary couplings from the Adder: - an Adder no longer needs to know about the internal structure of an annotation (such as how many ranges are in it), thus allowing it to be used in more contexts (image, video annotation, etc.) - an Adder no longer knows the internals of the registry: instead an explicit onCreate handler can be passed to the constructor to indicate what should happen when the adder is clicked while an annotation is loaded
This makes it possible to adjust the callback function after construction.
For consistency with TextSelector#onSelection, we pass the DOM event to the Adder#onCreate handler.
This cleans up the DefaultUI plugin to take advantage of the preceding commits further decoupling the UI components from one another and from Annotator.
This plugin has been replaced with 2 or 3 lines in the DefaultUI plugin.
Move the nasty z-index futzing into the DefaultUI plugin.
Convert the Document plugin to a new-style plugin. This makes the plugin a lot simpler, and while here I've taken the liberty of "purifying" the various metadata-extraction functions so they don't all mutate one enormous object. In addition, it appears that previously all annotations (before a cycle through the store plugin) shared a reference to one global metadata object. This is now fixed -- the metadata object is copied before being added to an annotation.
Move all storage-related components into src/storage.coffee
The functionality provided by the events library (namely the triggerThen function) has been superceded by the rather simpler AnnotatorCore#runHook().
This is not a great idea. It needs to be replaced with a polyfilling build of Annotator that can be loaded by the bookmarklet if it detects missing functionality.
This makes the notifier a new kind of component like storage, that can
be set at startup. The interface of a notification component is very
similar to that of HTML5 Notification.
The default Annotator is now instantiated with the old ("banner")
notification component enabled.
And, for simplicity, use runHook to run the onDestroy handlers.
Moves the Filter component into its own object in the UI module. It's still implicitly coupled to the Highlighter, but doesn't access "this.annotator" any more.
Delegator is really not doing an awful lot here, and if anything hinders
readability. I've replaced the use of Delegator with direct calls to
jQuery's event delegation method (".on()"), using an event namespace to
simplify event unbinding.
While it looks like I'm cheating by removing the tests for this event
binding, these kind of tests are ridiculous anyway -- they're testing
the implementation of the Filter plugin, and not its unit behaviour.
They won't be missed.
Just as with the previous commit, there's very little gained by using Delegator here any more.
Oh Delegator, you have served us well for many years, but now it is time for you to die. *2 minutes silence*
There's no need for one-line event handlers.
These handlers (well, really only the mouseup handler) were returning false, thus stopping propagation of the events and breaking other handlers, as discussed in #431. Also incorporating a change in the button check, from e.which != 1 to e.which > 1 which seems to be necessary for IE8 support (see #436). Fixes #431.
nickstenning
force-pushed
the
plugin-refactoring
branch
from
9775ce8
to
f0383c8
Compare
|
Merged to master and closed. This was getting unwieldy. |
|
\o/
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is another big upset to the organisation of Annotator's internals (sorry!), but one with significant benefits. The important things that change in this PR are:
Annotatorobject constructs the same old Annotator it has always done, complete with default UI -- but instead of being a mess, it is now a simple wrapper over:AnnotatorCore-- the class responsible for plugin and storage registration, which has arunHookmethod to run hooks on all plugins (if the associated callbacks exist)DefaultUIplugin -- where all of the default UI components are instantiated and wired togetherannotationsproperty, that provides the ability to perform annotation CRUD operations to pluginsonAnnotationCreated.Annotator.UInamespace (src/ui/).Annotator.Storagenamespace. These are not plugins, they are "storages"If you're looking for a place to start when reviewing this code, I suggest a quick glance at
src/annotator.coffee, followed by a look at how the default UI is wired together insrc/plugins/defaultui.coffee.This is WIP until I've finished messing with the other plugins (Auth, Markdown, Tags, Filter, etc. etc.)