Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#1555 closed defect (fixed)

DoubleClick plugin vs Xinha.Config.dblclickList

Reported by: ejucovy Owned by: ejucovy
Priority: normal Milestone: 0.97
Component: Xinha Core Version: trunk
Severity: normal Keywords:
Cc:

Description

In r1256 a Xinha.Config.dblclickList option was created:

  this.dblclickList = 
  {
    "a": [function(e, target) {e._createLink(target);}],
    "img": [function(e, target) {e._insertImage(target);}]
  };

This functionality already existed, as the DoubleClick plugin:

http://trac.xinha.org/browser/trunk/plugins/DoubleClick/DoubleClick.js#L38

The differences are:

  • The DoubleClick plugin attaches its configuration to the Xinha editor object itself, instead of config. This now goes against Xinha's best practices.
  • The DoubleClick plugin requires a plugin to be installed (obviously)
  • The DoubleClick plugin uses the name dblClickList. The new core config uses the name dblclickList (note the case difference).

I think the DoubleClick plugin should be deprecated and moved to the unsupported_plugins directory, and it should issue a warning if used. The warning should direct users to the new Xinha.Config.dblclickList option.

(See #1485)

Attachments (1)

dblclickList.diff (1.5 KB) - added by ejucovy 7 years ago.
plugin sets config.dblclickList and lets core handle the rest

Download all attachments as: .zip

Change History (10)

comment:1 follow-ups: Changed 7 years ago by gogo

Agreed.

Looks like the plugin also handles td (but perhaps that should be table), built in should do that to.

Probably best if the deprecated plugin is cut down to simply copying it's config into the now built in config and let the built in do everything from then.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 7 years ago by ejucovy

Replying to gogo:

Probably best if the deprecated plugin is cut down to simply copying it's config into the now built in config and let the built in do everything from then.

Oh, interesting -- you mean, the plugin should copy its dblClickList to editor.config.dblclickList on-load, and do nothing else?

That makes sense -- +1

Changed 7 years ago by ejucovy

plugin sets config.dblclickList and lets core handle the rest

comment:3 in reply to: ↑ 2 Changed 7 years ago by ejucovy

Replying to ejucovy:

Replying to gogo:

Probably best if the deprecated plugin is cut down to simply copying it's config into the now built in config and let the built in do everything from then.

Oh, interesting -- you mean, the plugin should copy its dblClickList to editor.config.dblclickList on-load, and do nothing else?

I attached a patch that does this -- let me know if this wasn't what you had in mind.

(The patch isn't a complete fix for this issue -- I'll put together a complete patch later. First I just want to confirm the approach.)

By the way, is there a better way to iterate over the items in dblClickList? I'm doing for( var i in x ) with explicit typeof checks for both i and x[i] .. is there a better way to do this in javascript?

comment:4 Changed 7 years ago by gogo

Yes that looks fine to me.

I think the typeof checks are the safe way of doing it, yes.

comment:5 Changed 7 years ago by ejucovy

  • Owner changed from gogo to ejucovy
  • Status changed from new to assigned

I applied the above patch, r1276.

Now just need to move it to unsupported_plugins.

I think the DoubleClick plugin should be deprecated and moved to the unsupported_plugins directory, and it should issue a warning if used. The warning should direct users to the new Xinha.Config.dblclickList option.

Actually, I won't specifically issue a warning directing users to the new option. Instead, when the plugin is loaded from unsupported_plugins, core emits a debug message which points to a documentation page about all the unsupported plugins (see #1558). That page explains what to use instead of the DoubleClick plugin. I think that's good enough.

comment:6 Changed 7 years ago by ejucovy

  • Resolution set to fixed
  • Status changed from assigned to closed

I moved the plugin to unsupported_plugins. r1277.

comment:7 in reply to: ↑ 1 Changed 7 years ago by ejucovy

Replying to gogo:

Looks like the plugin also handles td (but perhaps that should be table), built in should do that to.

Actually I'm not sure about this. I tested it, and the behavior is kind of bizarre. It inserts a sub-table within the current td, instead of calling up some way to edit the current table's options. This is because of the behavior of InsertTable?. So I think it's best to leave it as is, at least until/unless InsertTable? is changed to let you edit the current table.

(If TableOperations is used, we might want to call up the "edit table properties" dialog. I'm not sure. But, if TableOperations isn't installed, there's no good UI to use for this.)

comment:8 follow-up: Changed 7 years ago by gogo

Yes sounds like the old "td" double click isn't very useful.

The place to add a better handler would be in TableOperations, it can feed handlers into the core's double click list as it desires.

comment:9 in reply to: ↑ 8 Changed 7 years ago by ejucovy

Replying to gogo:

Yes sounds like the old "td" double click isn't very useful.

The place to add a better handler would be in TableOperations, it can feed handlers into the core's double click list as it desires.

Good idea, that makes sense. I've filed this enhancement at #1560.

Note: See TracTickets for help on using tickets.