Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for adding plugins #98

Open
13 of 34 tasks
ariutta opened this issue Mar 4, 2015 · 15 comments
Open
13 of 34 tasks

Allow for adding plugins #98

ariutta opened this issue Mar 4, 2015 · 15 comments

Comments

@ariutta
Copy link
Collaborator

ariutta commented Mar 4, 2015

The core of the library is now nearly feature complete, so to keep the size reasonable, we'll think twice before adding new features to the core.

But sometimes additional features will be useful to a subset of users. To allow for sharing the code for these kinds of features, we need to refactor to make it easy to add plugins. Then we can freely add unlimited new features as plugins while keeping the core of the library as small as possible.

Work in progress

@bumbu and @dlg-yahoo, are there other things needed to make it easy to add plugins? It might be awhile before I have time to do this myself, so don't let me be the bottleneck if either of you want to tackle this sooner. I'm fine with whatever you prefer, @bumbu.

@bumbu
Copy link
Owner

bumbu commented Mar 7, 2015

It seems that a plugin system will slightly break current API. That way I would go with a new milestone (v4).

If so it would be great to have following features/changes:

  • Plugins should replace customEventsHandler option
  • It should be possible to attach/detach plugins at runtime
  • Move current builtin controls into plugin
  • A plugin for mobile/touch events (wrap and improve current example)
  • A plugin for limit pan (wrap and improve current example)
  • Refactor inner hooked events
  • It should be possible for 2 different plugins to manipulate with the same event (eg. mouseclick)

And there are few questions:

  • How plugin attaching/detaching should work? jQuery model with attaching methods to jQuery prototype (fn) will not work here as library core should know about its plugins (in order to pass events).
  • Should we allow attaching the same plugin 2 times to the same library instance? It may sound ridiculous, but if a each version of plugin has different options it may be a desired use-case.
  • How should we pass events? We can use modified versions of PubSub/Observer and Chain of Responsibility. But here are some vital questions:
    • Is a plugin allowed to cancel an event? For example there are 2 plugins. Both are listening for click. First one is meant to filter all the clicks outside a circle. So second plugin will receive click event for processing only when first plugin will not block it.
    • If a plugin is allowed to cancel an event, how should it do it (what should it return)?
    • Should plugins work with raw browser events, with processed events (eg. clickevent may contain position and target) or with wrapped events (what jQuery)
    • These questions are also related to library inner events.

Other changes to consider for v4 would be:

  • Joining beforePan/beforeZoom and onPan/onZoom into one event beforeChange/onChange that will contain both zoom and pan data
  • Replacing callbacks (beforePan, onPan) with events thus allowing to have multiple subscribers to the same event (necessary for plugins)
  • A demo page with in-browser play with options.

@ariutta
Copy link
Collaborator Author

ariutta commented Mar 7, 2015

Agreed that this would be a new major version and also on the list of features/changes.

Lots of good questions, and I don't have answers to them all at this point.

How plugin attaching/detaching should work? jQuery model with attaching methods to jQuery prototype (fn) will not work here as library core should know about its plugins (in order to pass events).

Not sure about this one, although my comment at the bottom about Highland might be related.

Should we allow attaching the same plugin 2 times to the same library instance? It may sound ridiculous, but if a each version of plugin has different options it may be a desired use-case.

This sounds analogous to window.onload vs. window.addEventListener("load", function load(event){ },false);, and the latter is definitely better than the former. So if we can, let's do it.

How should we pass events? We can use modified versions of PubSub/Observer and Chain of Responsibility. But here are some vital questions:

Thinking this through - so a click could trigger new zoom event, and then we would then only use zoom events in the core to initiate zooming...

Is a plugin allowed to cancel an event? For example there are 2 plugins. Both are listening for click. First one is meant to filter all the clicks outside a circle. So second plugin will receive click event for processing only when first plugin will not block it.

Following on from the above click example, we could cancel the zoom event, but I'd be hesitant to cancel the click event.

If a plugin is allowed to cancel an event, how should it do it (what should it return)?

Not sure. jQuery does something where when event.preventDefault() is called, you can then use event.isDefaultPrevented() to know that it was called.

Should plugins work with raw browser events, with processed events (eg. clickevent may contain position and target) or with wrapped events (what jQuery)

If possible, it would be nice to allow for a combination, such as we allow the plugins to listen to raw browser events (but not cancel them) and also subscribe to processed (or wrapped) events.

The event handling ideas from Highland.js are pretty interesting. For example:

var clicks = _('click', btn).map(1);
var counter = clicks.scan(0, _.add);

counter.each(function (n) {
    $('#count').text(n);
});

Then if you want to have multiple listeners, you can fork that event stream.

@bumbu
Copy link
Owner

bumbu commented Mar 8, 2015

The event handling ideas from Highland.js are pretty interesting.
I don't think that we need streams. A simple PubSub/Observer should be enough. And it's really easy to be implemented. Or we can use many existing solutions like Radio.

Here is a library that has some plugins functionality. Not what we need but can be useful for inspiration.
https://github.com/brianreavis/microplugin.js

How should we pass events? We can use modified versions of PubSub/Observer and Chain of Responsibility. But here are some vital questions:

Thinking this through - so a click could trigger new zoom event, and then we would then only use zoom events in the core to initiate zooming...

After some thought a solution that I see is:

  • Plugins are initialized and declare their event listeners (eg. 2 plugins subscribed to mouseclick event)

  • A browser event is triggered (eg. mouseclick)

  • Library (svg-pan-zoom) catches this event

  • Library processes the event, wraps it in a new object that is passed to plugins. A wrapped object would look like:

    originalEvent: event
    oldPan: {x: 0, y: 0} // read only
    newPan: {x: 1, y: 1}
    oldZoom: 1 // read only
    newZoom: 2
    preventDefault: function(){} // Prevents library inner processing (not browser event)
    isDefaultPrevented: function(){} 
    
  • Wrapped event is passed forward to listening plugins using Chain of Responsibility (object is passed to first plugin, the result is awaited, when the result is received the object is passed to second plugin and so on until there are no more listening plugins). Plugins are allowed to modify newPan and newZoom. Also by calling preventDefault the event will not be passed forward to other plugins and will not be processed.

  • When all plugins finished processing the event, if event is not prevented - it is executed.

We may also have subscription to mouseclick and before:mouseclick events. before:mouseclick is the event that is wrapped by library, but newPan and newZoom are still not computed. Preventing from before:mouseclick would halt further event propagation and its computation.

Also I'm not sure about wrapped events attributes. They may look different. But the idea is that you should have access to original event, and you should be able to alter library default computation results.

@ariutta
Copy link
Collaborator Author

ariutta commented Mar 8, 2015

Right, I mentioned Highland for inspiration but don't want to make it a dependency. That microplugin library has some good ideas.

Let's go with your proposed solution. Nice work!

For the wrapped events attributes, we can mimic jQuery attributes, whenever appropriate, to make it easier for other developers to understand.

@ariutta
Copy link
Collaborator Author

ariutta commented Mar 8, 2015

@dlg-yahoo, if you have any preferences here, feel free to chime in!

@bumbu
Copy link
Owner

bumbu commented Mar 8, 2015

I feel like all these ideas are still raw and need some more thought. Also some implementation prototypes would be helpful. Unfortunately right now I don't have time to work on these things actively as I'll be in India this whole month (it really takes time to travel).
But if anyone else has time and wants to play with it - it would be a great contribution. At least prototyping this plugin system would help us a lot.

@bumbu
Copy link
Owner

bumbu commented Mar 8, 2015

One more thing to think about: How to allow plugins to update SVG manually. For example if you want to animate zoom/pan then you may want inner state of library to have final values, manipulation with SVG done by the plugin. Or maybe such a thing is not necessary and there are other solution that would allow to achieve the same result.

Related to #101

@ariutta
Copy link
Collaborator Author

ariutta commented Mar 10, 2015

Yeah, this is an ambitious refactoring, and I won't be able to get to it for awhile either. Let's keep thinking about it and then try some prototypes as time allows.

I hadn't thought of the use case from #101, but I can see how that might be useful. If we can accommodate something like that, it would be great, as long as we don't start over-engineering this library to point where it's painful to work with.

@ariutta
Copy link
Collaborator Author

ariutta commented Mar 10, 2015

Also, for any changes that we are more sure about, let's not let this issue become a roadblock. We can come back to this at any time.

@bumbu
Copy link
Owner

bumbu commented Mar 10, 2015

I agree on fact that this issue shouldn't be a stopper, we can still add minor versions to v3.

@bumbu
Copy link
Owner

bumbu commented Dec 28, 2015

I did start working on this. There will be few major changes:

  • Events system (on, off, trigger as in jQuery) instead of old callbacks (onPan, onZoom)
  • Middlewares system - something similar to events but with ability to alter and cancel some events
  • Plugin system - plugins will have almost the same capability as plublic API but will have the benefit of being namespaced and automatically started and stopped on instance creation and destruction respectively
  • Some API methods will be deprecated in favor of new events and plugins

bumbu added a commit that referenced this issue Dec 29, 2015
Add plugins system
Add events system
Mock middlewares system
Replace on/before Pan/Zoom callbacks with events
Trigger events for CTM render
Rename plublicInstance to publicApi
Based on #98
bumbu added a commit that referenced this issue Jan 2, 2016
Add plugins system
Add events system
Mock middlewares system
Replace on/before Pan/Zoom callbacks with events
Trigger events for CTM render
Rename plublicInstance to publicApi
Based on #98
bumbu added a commit that referenced this issue Jan 2, 2016
Removed all the secondary functionality such as:
- Browser events
- Enabling/Disabling pan/zoom events
- Control icons
- Double click
- Mousewheel zoom
- Min/max zoom
- fix/contain/center

Removed functionality will be moved into plugins or as easy to implement examples.

Browser events (click, mousemove...) will be moved into a plugin. The same goes for mousewheel.

Everything will be around events. Some of them will have alterable event data.

Related to #98
bumbu added a commit that referenced this issue Jan 3, 2016
Public API and each module API methods are namespaced.
This means if you call a plugin API method that results into a panzoom event
then this event will contain that plugin name as event namespace.

Such a behaviour is useful for plugins that want to cancel certain events and
trigger those events by themselves without getting into a recursion. One such
example is a plugin that will animate pan/zoom.

API creation was moved into a module. Reference to inner methods is intentionally
hidden so that it is not accessible from exterior.

Related to #98
@bumbu
Copy link
Owner

bumbu commented Jan 4, 2016

  • Event system is implemented.
  • Middlewares idea was merged into events. So events now can be prevented and canceled. Changing event's payload data will update the data that is used internally by library
  • Public and Plugin API. These are objects that give you access to some inner functionality (pan, zoom...). Each such API is namespaced so when a plugin API call triggers an event - you'll know who is responsible for that event (except render event)
  • Removed all secondary functionality from core, how have to add it back as plugins. This will allow me to polish and document plugin system. Most probably library will come in 2 flavors: without any plugins and bundled with all basic plugins. For custom variations a build system will be available

All this changes are available in plugins branch.

@ariutta
Copy link
Collaborator Author

ariutta commented Jul 10, 2018

@bumbu, what are your thoughts on SVG animation vs. just pan and zoom? My initial thought for this library was to make it a simple drop-in solution for just pan and zoom. But many users want full animation capabilities.

Just brainstorming for v4 or v5, but could the core just be a very small cross-browser wrapper for smoothly manipulating the CTM? Maybe the plugin system could provide the ability to call specific CTM manipulations/animations like fit, center, panBy, rotate etc. Could event listeners be part of the plugin system?

What do you think about supporting use with the SVG script element vs. use with the HTML script element? Would it make sense for v4 or v5 to handle both?

As you suggest, we could offer multiple builds. Maybe:

  1. turnkey / drop-in solution for just pan and zoom (include plugins to satisfy 80% of users)
    a. for use with the HTML script element
    b. for use with the SVG script element?
  2. a build for users that want to do complex SVG animation
  3. Other?

@bumbu
Copy link
Owner

bumbu commented Jul 11, 2018

Hey @ariutta,

Regarding animation: I agree that ideally the library would provide the bare minimum (aka pan and zoom) and animations would come as an add-on/extension.

As for the future versions ideally we'd want as you mentioned to provide the core/basic manipulations with ability to extend/hook into them with plugins/add-ons. We could bundle them in different versions or provide instructions how to build different bundles but based on current state of JavaScript - most projects have their own bundlers and we'd only need to provide the source.

Not sure thought that we want to support the SVG script tag as it feels like an edge case which can be covered by simply embeding the SVG into an HTML page.

But starting with a plugin system would be a great start and based on the usage of that it should be easy to see what people need the most (plus we have a ton of suggestions in issues :).

As you can see from this thread I started building the plugin system at some point, but never finished it, and I don't have time to do it any time soon. But if anyone wants to work on that then I'd be definitely open to help.

@ariutta
Copy link
Collaborator Author

ariutta commented Jul 11, 2018

I'm hoping to find some time at some point here, but it is a big task for a free side project! If I have the chance to dig into this, I'll let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants