Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#1534 closed defect (no action needed)

"fullscreen" toolbar button does not load properly

Reported by: guest Owned by: gogo
Priority: normal Milestone:
Component: Xinha Core Version: trunk
Severity: normal Keywords: toolbar
Cc:

Description

Using Xinha trunk@HEAD, the "fullscreen" toolbar button does not load properly when I add it to xinha_config.toolbar.

If I add the plugin "FullScreen?" to my xinha_plugins list that is passed to Xinha.loadPlugins, I get a Javascript error:

plugin is not a constructor

So I removed "FullScreen?" from the xinha_plugins list and tried to load its toolbar button without explicitly loading the plugin. This time the editor loads without the fullscreen button and I get an error message:

ERROR [createSelect]:
Can't find the requested dropdown definition

I traced through the source code and it seems that the FullScreen module's constructor is never called, even though FullScreen is a required core module.

The following diff fixes the problem, by automatically registering FullScreen if it is present in the toolbar:

Index: XinhaCore.js
===================================================================
--- XinhaCore.js	(revision 1270)
+++ XinhaCore.js	(working copy)
@@ -2389,7 +2389,7 @@
     {
       switch (toolbar[i][j])
       {
-        case "popupeditor":
+        case "popupeditor": case "fullscreen":
           if (!this.plugins.FullScreen) 
           {
             editor.registerPlugin('FullScreen');

Change History (4)

comment:1 Changed 6 years ago by ejucovy

  • Resolution set to no action needed
  • Status changed from new to closed

Actually, "popupeditor" is the way to spell it in the toolbar. This is probably some ancient backwards-compatibility thing. But there's no reason to add "fullscreen" -- better to just have the user's config say "popupeditor".

This should really be better documented though.

comment:2 follow-up: Changed 6 years ago by gogo

Yes popupeditor is ancient and sucked.

FullScreen? was developed as a plugin sometime later, when it was loaded it would replace popupeditor's button.

At some point popupeditor was removed and FullScreen? was adopted as a core module.

changeset:1288 adds the case statement so that people can use "fullscreen" in the toolbar instead of "popupeditor" and it should still auto-load the FullScreen? module.

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

Replying to gogo:

changeset:1288 adds the case statement so that people can use "fullscreen" in the toolbar instead of "popupeditor" and it should still auto-load the FullScreen? module.

I tried testing this change locally, and noticed things got weird and broke with strange errors if my configuration's toolbar included BOTH "popupeditor" and "fullscreen". That's why I closed the ticket without committing the change.

On the other hand, it's probably better with the extra case -- I doubt anyone will ever do this, we can address it with documentation, and it's more convenient than having to know that FullScreen? == "popupeditor" :)

comment:4 Changed 6 years ago by ejucovy

  • Keywords toolbar added
Note: See TracTickets for help on using tickets.