Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#5707 closed defect (fixed)

Improved Editor Plugin Architecture

Reported by: ptwobrussell Owned by: bill
Priority: high Milestone: 1.7
Component: Editor Version: 1.0
Keywords: refactor Cc: liucougar, alex, Adam Peller
Blocked By: Blocking:

Description (last modified by bill)

I don't think this is news to anyone, but wanted to start a ticket to discuss the Editor's plugin architecture. Basically, I am hoping that at some point, plugins will be more standalone in that plugin code from _editor/plugins won't have dependencies on Editor.js, which appears to be the case right now.

While trying to write about plugins, I quickly ran out of creative ideas for how to tell someone to write their own custom plugin without suggesting that they hack on Editor.js (which currently has a bunch of case statements that handle some of this stuff.) Am I wrong, and there is there is a fairly simple and elegant way to create custom plugins w/o doing that?

Also, it might be worth talking about how you should and shouldn't pass in values to plugins. For example, on the EnterKeyHandlingPlugin, I don't see a better way of specifying blockNodeForEnter than adding a call to

dojo.extend(dijit._editor.plugins.EnterKeyHandling, { 
                 blockNodeForEnter : "div"

sometime before the editor is parsed. Is there a better way of doing this? Should there be? (All sincere questions.)

Change History (21)

comment:1 Changed 11 years ago by liucougar

for the first issue, you can use dojo.subscribe to listen to dijit.Editor.getPlugin, look at the end of Editor.js for an example

for the second issue, you can use this:

new dijit.Editor({extraPlugins: [{name:'dijit._editor.plugins.EnterKeyHandling',blockNodeForEnter: 'div']}

comment:2 Changed 11 years ago by liucougar

Component: DijitEditor
Owner: set to liucougar

comment:3 Changed 11 years ago by alex

Milestone: 1.1
Owner: changed from liucougar to alex
Status: newassigned

I really don't see that listening to a topic is the right way to handle this. The case statements need to go away in favor of an AdapterRegistry?. This shouldn't be so hard.

comment:4 Changed 11 years ago by Adam Peller

Priority: normalhigh

comment:5 Changed 11 years ago by bill

(In [12588]) Just a testcase for adding custom plugins (against Dojo 1.0 API). Refs #5707.

comment:6 Changed 11 years ago by Adam Peller

(In [12702]) Remove special cases from Editor switch, register topics for plugins instead. Apply styling to font menus. Refs #5707, Fixes #5980

comment:7 Changed 11 years ago by dylan

Milestone: 1.11.2

mass move of editor issues to 1.2.

comment:8 Changed 11 years ago by alex

(In [14733]) first in a set of large-ish merges to address issues related to the iframe refactor. This patch shortens the existing code, adds some documentation, and adds the first handling of selection saving when focus is changed. Refs #6582. Refs #5707 !strict

comment:9 Changed 11 years ago by bill

Description: modified (diff)
Milestone: 1.21.3

comment:10 Changed 11 years ago by bill

Component: EditorDojoX Editor

comment:11 Changed 11 years ago by Adam Peller

Component: DojoX EditorEditor
Keywords: refactor added

comment:12 Changed 10 years ago by bill

Milestone: 1.3future

comment:13 Changed 9 years ago by bill

Description: modified (diff)
Milestone: future2.0
Priority: highnormal

You certainly don't need to modify Editor.js to register a plugin, but I agree that using dojo.subscribe() is strange. I thought Cougar said there was some reason, like that Editor wasn't loaded yet when the plugins were loaded, so they couldn't just register themselves? I don't see why that would happen though.

AdapterRegistry doesn't seem right either since all we really need is a dictionary mapping plugin name (short name like "createLink") to plugin. Actually, I'm not sure why plugins need to be registered at all (except to support that short name --> proper name mapping).

In any case, marking this for 2.0 to decide then.

comment:14 Changed 8 years ago by Chris Mitchell

Owner: changed from alex to Jared Jurkiewicz
Status: assignednew

please review/triage accordingly

comment:15 Changed 8 years ago by Jared Jurkiewicz

Milestone: 2.0future

comment:16 Changed 8 years ago by bill

I'm guessing that by "dependencies on Editor.js" ptwobrussell is referring to the misconception that you need to modify the Editor.js source code, rather than objecting to plugins require()'ing "dijit/Editor" or "dijit/_editor/RichText"? The latter requirement doesn't seem so bad.

Instead of publish/subscribe, I was thinking about a simple:

RichText.registerPluginFactory("fontName", function(args){
   return new dijit._editor.plugins.FontChoice({command:});

(I think using AdapterRegistry is overkill since all we need is a name --> factory hash.)

Cougar, is there some reason that won't work? If each plugin (or _Plugin.js) require()'s RichText, then we know it will be available in time.

Currently we are using this code block to register a [set of] plugins:

dojo.subscribe(dijit._scopeName + ".Editor.getPlugin",null,function(o){
	if(o.plugin){ return; }
	case "fontName": case "fontSize": case "formatBlock":
		o.plugin = new dijit._editor.plugins.FontChoice({command:});

Besides being a bit verbose, for some reason it's accessing dijit._scopeName, which means needing to access the "dijit" namespace, so I don't like that.

comment:17 Changed 8 years ago by liucougar

AdapterRegistry? or a mini-version of that (name->factory hash) sounds fine

I think something like the following is a bit more flexible (allowing a single call to register multiple commands):

Editor.registerPlugins({fontName: function(args){return new ...(args)}})

comment:18 Changed 8 years ago by bill

Milestone: future1.7
Owner: changed from Jared Jurkiewicz to bill
Status: newassigned

OK, I'll add that in, keeping the publish/subscribe mechanism for back-compat until 2.0.

comment:19 Changed 8 years ago by bill

Resolution: fixed
Status: assignedclosed

In [26064]:

Simpler way to register editor plugins. _Plugin holds a hash mapping plugin name --> plugin factory. I didn't add any accessor methods for the hash, although they could be added later.

Old subscribe/publish method of registering plugins will be supported until 2.0.

Fixes #5707 !strict.

comment:20 Changed 8 years ago by bill

In [26069]:

As per Cougar's suggestion, switch code so it isn't creating a separate factory function for each builtin plugin. Shaves a few bytes and theoretically runs faster, although strangely the gzipped version is actually about 50 bytes bigger. Refs #5707 !strict.

comment:21 Changed 8 years ago by bill

In [26090]:

For back-compat with 1.6, accept "viewsource", "fullscreen", and "newpage" as plugin names, in addition to the standard casing of "viewSource", "fullScreen", and "newPage". Refs #5707 !strict.

Note: See TracTickets for help on using tickets.