[WIP] Simplify UI.Highlighter API

[tilgovi]

Rather than working with annotation objects, the UI.Highlighter
component receives ranges directly and returns highlights. It is
now the job of the caller to determine what to do with those
highlights and when to undraw/redraw them.

[tilgovi]

Marked as WIP because the highlighter plugin should also be updated.

[tilgovi]

I just added a commit which removes the Highlighter plugin.

The code for this is all within the DefaultUI plugin. We should have a conversation about this.

I'm very tempted by the following after discussing with @azaroth42 today and thinking a bit more about the future:

1. DefaultUI shouldn't be a plugin, it should be a default Annotator
2. .addPlugin is unnecessary API
3. Hooks are not an obviously a good idea

So the proposal is that it's much better for most non-trivial uses of Annotator if we don't have plugins that wire themselves up on .addPlugin. Since most of the tools needed to build an annotation application have been split into separate components, wiring things up becomes pretty straightforward, and is easiest if Annotator stays out of your way.

I would suggest that our DefaultUI instead be a legacy Annotator that defines the addPlugin interface in a way that's backward compatible with Annotator v1.2 but that v2.0 doesn't have any plugin API on the core. It's not at all clear that the Core even needs to be a thing, since it would just do nothing, and that's what I want.

As a user of Annotator, I would much rather just write my application and use the tools Annotator gives me to construct it.

function MyApp (element) {
  var annotation, interactionPoint;

  this.storage = OaHttpStorage('https://annotateit.org/oa');

  this.adder = new UI.Adder({
    onClick: function () {
      storage.post(annotation).then(function () {
        var highlights = this.highlighter.draw(annotation._local.ranges);
        annotation._local.highlights = highlights;
      });
    }
  });

  this.highlighter = new UI.TextHighlighter(element);
  this.selector = new UI.TextSelector(element, {
    onSelection: function (ranges, event) {
      annotation = {
        _local: {ranges: ranges},
        hasTarget: {
          hasSource: document.location.href,
          hasSelector: {
            exact: ranges.map(function (r) { return r.toString() }).join('')
          }
        }
      };
      interactionPoint = Util.mousePosition(event);
      adder.show(interactionPoint);
    });
  };
}

[tilgovi]

So, anyway, toward that direction, this PR simplifies the API of UI.Highlighter in a move toward isolating the knowledge of the "shape" of annotations (the names of poperties like .ranges, etc) to the application, which is responsible for wiring things together and determining format.

I don't really think there's any need at this point to bless a particular internal model. We have a bunch of useful components now. Let's just use them for a bit an see what happens.

And without a plugin API in core we're free to make a legacy Annotator application that implements it in a backward-compatible way, being an event emitter and firing the old events.

[aron]

This block belongs back in a highlighter.drawMany() method. This glue code shouldn't be concerned with the performance of rendering many highlights. By putting it back into the highlighter module anyone wishing to render multiple annotations gets the benefits of this code without having to re-implement it.

[tilgovi]

I'm not so sure. The draw function already takes an Array of Ranges and returns an Array of Highlights. For a drawAll to be useful to the place it's been used, loading many annotations, it would be expecting an Array of Arrays of Ranges and would have to return an Array of Arrays of Elements. That may be getting a bit busy unnecessarily. @nickstenning clearly felt some desire to give the developer more control over how to optimize this, because he had options on the UI.Highlighter to control the chunk size and interval. I might rather let the developer do that if it fits their needs. For instance, other approaches might be to use requestAnimationFrame, some other async flow control library of choice, etc.

Do you think it's too onerous to push this up to the caller?

[aron]

I think it should definitely be using requestAnimationFrame. I'm pretty sure that method wasn't very widely supported (if it even existed) when the code was originally written.

Do you think it's too onerous to push this up to the caller?

I look at this file and what it's doing and this particular block of code looks completely out of place to me, which is a general indication that it should be moved either down into the module or into some kind of helper function.

[tilgovi]

Definitely agree with the last. Let me see about a prettier helper function here.

[tilgovi]

See what you think now.

[aron]

I've left some comments on the implementation here, I think it's a good start but moves too much UI code out of the UI module. I need a little more time to think about the larger direction changes to how plugins work, in general though the plugin API is in flux, and suggestions like this are a good thing.

[tilgovi]

Now with rAF sauce. Love me some browserify.

[nickstenning]

So, I like some of this, but like @aron I think some of the highlighter's concerns have been pulled out too far.

If I can try to summarise the main concern that has led to this rejig: UI.Highlighter has too much knowledge about the structure of annotations. It shouldn't need to know that the thing it's interested in lives on the "ranges" property of an annotation.

I agree completely with this assessment.

However, by moving as much as you have into DefaultUI, you've now made it much harder for someone to reuse the highlighter on its own. Before this PR, you could simply add the highlighter plugin and annotations would be drawn/redrawn/removed without you having to do anything else. Now I have to copy a fair bit of code from the DefaultUI plugin.

Perhaps a better compromise would be for UI.Highlighter to have an API which deals in ranges, but for Plugin.Highlighter to be responsible for translating annotations into ranges. In the trivial case, this means that it pulls the ranges property off the annotation, but it would also be possible to extend the highlighter plugin to translate a TextQuoteSelector into a set of ranges, too, and pass them along to the UI component.

I also think UI.Highlighter should remain responsible for actually drawing highlights in the DOM (although +1 for using requestAnimationFrame here).

[nickstenning]

2. .addPlugin is unnecessary API
3. Hooks are not an obviously a good idea

More generally, I'm not sure about the above. Certainly, we should make it possible for people to wire up their own Annotator applications as they wish, and they can already do this by reusing the components from UI.* just as you've done in your example above. The current approach does not preclude this use case.

But there's also a class of users that falls somewhere in between the extremes of "plug and play" and "hypothesis" -- the kind of users that might want to be able to add specific lifecycle plugins with default behaviours. For example, maybe I have a static set of annotations on a document, but I don't want to allow people to create or edit annotations. I'd like to be able to do something like:

annotator
    .addPlugin(Annotator.Plugin.Highlighter(element))
    .addPlugin(Annotator.Plugin.Viewer(element));
annotator.registry.annotations.load(query);

This is more-or-less possible at the moment, but not if you remove the Highlighter plugin and its default mapping of lifecycle events to the draw/drawAll functions of the UI.Highlighter component.

[aron]

But there's also a class of users that falls somewhere in between the extremes of "plug and play" and "hypothesis" -- the kind of class of users that might want to be able to add specific lifecycle plugins with default behaviours. For example, maybe I have a static set of annotations on a document, but I don't want to allow people to create or edit annotations.

+1 I think we've seen this many times on the mailing list over the years. And I think having to wire up the UI elements to do this creates a high barrier to entry.

[azaroth42]

To put in Stanford's support for the change, we want to reuse the framework with modular components and fine-grained control of where and when annotators can be instantiated. It seems that by decomposing the code it makes this much easier than the current plugin architecture and defaultUI.
Further, it seems that the proposed approach isn't in conflict with what @aron and @nickstenning are saying, those are just higher level applications that could be provided on top of the decomposed code.

.addPlugin() could be a function of LegacyAnnotator rather than the baseline. The functions on LegacyAnnotator would then implement the current model of calling the hooks on each of the plugins in the order added. This gives the opportunity for other extensions of the base Annotator to be smarter about which order the components are used in, getting rid of the potentially infinite beforeBeforeBefore...AnnotationCreated hooks.

[tilgovi]

However, by moving as much as you have into DefaultUI, you've now made it much harder for someone to reuse the highlighter on its own. Before this PR, you could simply add the highlighter plugin and annotations would be drawn/redrawn/removed without you having to do anything else. Now I have to copy a fair bit of code from the DefaultUI plugin.

Before this PR, the Highlighter plugin was not actually used in this repo anywhere. The DefaultUI was using UI.Highlighter directly.

Perhaps a better compromise would be for UI.Highlighter to have an API which deals in ranges, but for Plugin.Highlighter to be responsible for translating annotations into ranges.

That's fine, but such a plugin would be dependent then on finding annotation.ranges. With this PR, only DefaultUI knows that's where ranges go.

What we end up with when plugins try to respond to generic annotation hooks is a situation where sets of plugins are all sharing implicit assumptions about the data they receive.

We should, at the very least, do this:

1. DefaultUI is an Annotator. All the code in DefaultUI should move to annotator.js. It's an implementation of the Annotator and the Core was separated to make space for exactly this.
2. I would argue we should either move plugins out of Core and into this particular implementation, opening up the option to re-instate a backwards compatibility API there, or we should at least stop calling hooks on all plugins.

I hate the hooks as much as I hated the events. Now we call them hooks because they weren't being used like events but they're still a crap way to orchestrate components. A hooks system is fine, but a hooks system to allows multiple subscription and does it well and allows for flexibility without a proliferation of before and after hooks is not trivial. The best hook system, if we want one now, should be a simple one that calls a single callback on the Annotator. We have .extend() for a reason. Use it and compose your hooks yourself.

[tilgovi]

Here's my high level summary of the situation. There are three kinds of things we can build:

1. Libraries
2. Frameworks
3. Applications

I think we used to have only (3). Now we have (1) and (3), but it needs some work and this PR is an example. We probably, eventually want (2), but we don't know the shape of that yet. I don't think we will know until we build several different applications with (1). I know it shouldn't be a generic front-end JS framework. I think it should have some configuration-driven setup, involve things like what @csillag has for an anchoring manager, and reduce boilerplate for applications. A good registry is a key component of this, but the current registry is next to useless (if the plugins receive a registry then the registry is where the plugin API belongs, so plugins can load other plugins and plugins can share settings, at which point it becomes possible to shift the knowledge about shape-of-things and property names, options, et al to one place).

[tilgovi]

function Registry() {
  this.components = {};

  // This is important.
  // It's how plugins share config, so they don't have to make assumptions.
  // It increases their re-usability.
  // It also means they have a place to get their setting so that they don't take another argument.
  // That allows us to call `.include()` and pass the constructor, rather than instantiating an Object
  // that then gets passed to `addPlugin`, important so that `addPlugin` can be idempotent, which
  // allows plugins to include their dependencies (if they depend on other plugins).
  this.settings = {};
}

Registry.prototype.include = function(componentFactory) {
  // Go through this.components and if componentFactory has not been included yet, invoke it, passing the registry, and include it.
};

This is the direction I was going at one point. @nickstenning are you sure you don't want to go this way?

[tilgovi]

The registry is also the place for a hooks API, so plugins can fire hooks and subscribe to hooks.

But we need to make a decision. Hooks should either a) have only one subscriber or b) have no defined return value. Having both is a recipe for awful plugin ordering issues and whatnot.

[tilgovi]

Pyramid and Angular are two of the best examples I've experienced of frameworks that successfully allow for patterns of composable / extensible applications. Admittedly, my direct experience with other frameworks is somewhat limited, though I did extensive research and this belief was the primary decision driver for using both in Hypothes.is, where I expected to need to white label and remix applications for different deployments. Pyramid (Zope Component Registry) and Angular (Dependency Injection) both provide two things I think are crucial for this kind of modularity:

1. A way for components to depend on other components
   1. Angular, through angular.module('myModule', ['dependencies'])
   2. Pyramid, through the idempotent nature of config.include()
2. A place to share settings
   1. Angular, through injecting providers
   2. Pyramid, through registry.settings
3. A separation between configuration and run phases
   1. Angular, through angular.module().config() and angular.module().run()
   2. Pyramid, through config.include() and config.make_wsgi_app() (handed off to WSGI server)

This is why I believe that we need:

1. A Registry that's instantiated before the Annotator (or, without loss of generality, any Application), and which is available during the configuration phase.
2. A settings Object available on said registry rather than passing options to every component by hand.

[tilgovi]

Without these things we have an Annotator application which isn't really very modular nor easily extensible (very limited ability to mix and match plugins). If we want the plugin pattern to be useful, we should abandon the way hooks are done, we should pass settings on the registry, and plugins should be able to require other plugins. Otherwise, we should support it as a LegacyAnnotator implementation for old Plugins in the old style for backward compatibility and say nothing of plugins in the core.

At one point I had been trying to do two things at the same time, and I think we still could:

1. I had a registry.include() function, modeled after Pyramid. This API purposely never spoke the word 'plugin'.
2. The old 'plugin' API was intact, on the Annotator.

I did also have the registry holding properties it then transferred to the Annotator, which I realize now was stupid. However, I think the general direction was right.

[nickstenning]

Going to close this for now.