Allow to select editor theme/skin

Basically a duplicate of #313497: Allow configuration of advanced editor settings, but I would be ok with slipping this in while the major editor settings revamp is not done.

As identified in #374391: Allow to configure editor library variant to load, I am pretty sure this needs to be a global setting that applies to all wysiwyg profiles for the same editor. If not, each editor needs to be tested whether it actually supports to load multiple different themes on the same page.

Which brings me to the actual point: We do not want to hard-code the available theme #options. Instead, we have to test whether the editor implements a "themes callback". If it does not, the select is not displayed at all and we are storing "default" or nothing by default (I'm not sure yet). If it does, then we provide the actual themes returned by the callback.

On that note, the last hunk for wysiwyg_get_editor_themes() seems wrong and should be reverted. This function basically "registers" the available themes only. Validation and loading happens elsewhere.

For 2.x, I would be ok with a per-profile setting to start with. If we find out that editors do not support this, we will have to turn it into a global setting (somehow).

Umm... I don't think this is RTBC as it would probably break at least TinyMCE because the hardcoded theme names in the patch has no meaning to it. Like sun suggested, the code must use the theme callback (if there is one) to provide the options for the selectbox.

In considering this patch, I see that it has been "partially" committed and the changes appear in the 9-18-2009 dev version. To clarify, the change to the wysiwyg.module file is there, but the change to the wysiwyg.admin.inc file had been modified to be a hidden form item. I am guessing that this is because of TwoD's comment in #7 regarding theme callback as options.
I was doing some work on CKEditor integration and wanted to be able to select the theme in the gui so, here is a patch to the aforementioned dev version that uses the theme callback to provide the options to the select list and makes the form option visible.
I moved the location in the form from basic to appearance as it was in the #3 patch since I think that is the more logical location for the option. I also changed the title and description fields.

And since that last patch was against the 6.x-2.x-dev, here's the same thing for 7.x-3.x-dev.

+++ wysiwyg.admin.inc	2009-10-08 22:24:58.000000000 -0700
@@ -150,6 +145,14 @@ function wysiwyg_profile_form($form, &$f
+    '#default_value' => $profile->settings['theme'],
+    '#options' => $editor['themes callback']($editor, $profile),

We could probably use wysiwyg_get_editor_themes() for both these lines. Otherwise I think it's good work!
I'm on crack. Are you, too?

Uhhh... yes. The #options line is doing just that, but in a somewhat indirect way. Calling wysiwyg_get_editor_themes() directly would be clearer. The #default_value, however... I think that should be called as the variable inside the function like I have it so that it refers to the current editor. Unless I'm missing something.

It would not only be clearer, but it would also make sure there is a themes callback, as it's optional. (Falls back to 'default').
For #default_options, if we call it like this wysiwyg_get_editor_themes($profile, $profile->settings['theme]') it will check that the current theme exists, and if it doesn't the first available theme will be returned instead.

Do you think we should change themes to be key/value pairs, so we can have a "machine name" and a "human readable name", or is that just extra fluff which nobody cares about? Theme names are usually short and simple anyway and would not be seen that often anyway.

Ah, yes, I see.

As for key/value pairs, I guess it doesn't matter as long as the callback function in the editor include file returns a static array. However, if it is set to scan the editor's library files and build the array based on the found directory names (ie - as I did here in ckeditor.inc and the commented out section of the function in tinymce.inc), then the numerical key could change if the editor is updated and happens to included new themes/skins. So, with that in mind, using defined keys would make sure that the chosen theme key reference didn't change when the editor is updated.

Whats the state of this issue now?

Yes, I have the 6.x-2.x-dev version but this had no admin interface and such.
Is this still the patches phase?

Yes, the patch in #10 needs a bit more work (see comments below) and also more testing with the editors. We probably also need some more support in the editor implementations to expose the available themes to make sure this implementation does not make theme selection impossible or very hard to do for some editors. Changing the actual theme might also be affect which settings are available etc and parts of Wysiwyg might depend on a specific "base theme" being used, which is why we sometimes only allow to switch the skin even though it says theme in the GUI.

Any help you can provide in improving this is of course welcome. I'm occupied with other issues and projects at the moment but I'll try providing input when I can.

Btw, in the -dev versions it's now possible to override the settings sent to each editor via hook_wysiwyg_editor_settings_alter, including the theme/skin settings.

Marked #951768: Request support for o2k7 skin in TinyMCE a duplicate of this issue.

The above issue also mentions another "dimension" to the editor's appearance: skin-variants - an alternate stylesheet to load instead of [or in addition to?] the skin's regular stylesheet.

@morningtime: If you set the theme/skin via hook_wysiwyg_editor_settings_alter() it'll overwrite what's hardcoded in the editor implementation. Useful workaround until this issue is fixed.

I only going to use kama. Is it safe to delete the other 2 skins from: sites/all/libraries/ckeditor/skins

Thanks,

@CFW, Yes, it's safe, the editor doesn't use or even look for unused skins. Just remember that they've been deleted and you'll be fine.

Thanks, for your reply. I just wanted to make sure.

I had the same problem morningtime did in #21. Rather than hack it I started digging. I found that the problem was that the value of the skins is saved by key, yet when we're making sure that the skin exists before displaying it, we're checking by value. I've tested the following patch with TinyMCE and CKEditor.

EDIT: This patch is against HEAD.

I'm testing this and it's working well.

patch #26 works for me

This patch throws errors for editors which do not have a theme callback ($editor['themes callback']).
I've made some modifications that I'll upload as a patch when I get back home.

And here's the patch. It uses Wysiwyg's wysiwyg_get_editor_themes() function to deal with falling back to a default theme in case the selected one is missing (could happen after a library upgrade).

For those editors which support more than just themes, such as skins and even skin variants, I've made those output a list with entries like theme/skin/skin_variant which is later split into the corresponding settings by the editor implementation itself. I've not yet tested what happens if the "theme" value stored in the database doesn't match one of the values returned from the theme callback, and the editor is used before re-saving its profile.

I strongly recommend not testing this on a production site and to re-save the editor profile to make sure a valid "theme" value is stored in the database.

NOTE: This patch does not yet deal with the appearance of the skins. For example, TinyMCE's office skin looks really bad because we've previously had to add script/style hacks to make the original skin's toolbar wrap better. When removing those hacks, it does look alright, unless a large number of buttons are activated. Once #277954: Allow to sort editor buttons gets in that'll be less of a problem, but we should probably do something about it before that.
Which theme/skin should be the "fallback one" in case the selected one isn't found anymore is more or less a coincidence at this point, I simply used natural sorting for testing.
Each theme function has a small comment detailing what the expected output should be. This was not something I intended to include in a final version but it was good for comparing the actual result with.

I expect this patch to need more work but I'll bump it to "needs review" to get some more eyes on it. Please don't frown upon things like naming conventions this time around. I'm simply too tired right now to come up with good variable names and keep them synced everywhere.

More options related to themes will have to wait until #313497: Allow configuration of advanced editor settings. The reason I merged theme/skin/skin_variant settings to just this selectbox was both to not have to add more widgets before the aforementioned issue is fixed, and to make sure only valid combinations can be selected.

+++ wysiwyg.admin.inc
@@ -150,6 +145,17 @@ function wysiwyg_profile_form($form, &$form_state, $profile) {
+  if (function_exists($editor['themes callback'])) {
+    $available_themes = drupal_map_assoc(wysiwyg_get_editor_themes($profile));
+    $form['appearance']['theme'] = array(

1) Even if no 'themes callback' exists, we still need to output the form element. Moving it into appearance seems to be a good idea.

2) I wonder in which other locations we're using wysiwyg_get_editor_themes(), and depending on that, whether we can change its return value to have the internal theme name/pointer as keys, and human-readable name labels as values. -- Not 100% sure about that, perhaps a separate callback would be better.

Powered by Dreditor.

1) Ok.
2) We only call wysiwyg_get_editor_themes() once in wysiwyg.module, and I doubt anyone else is calling it since it doesn't produce much of interest now.
I'm not sure we can find the human readable names so easily, most store them inside the theme scripts themselves, if storing them at all. Checking if files are there and grab their names is one thing, trying to parse contributed themes/skins to find a string of unknown contents (and perhaps context) is another. Nothing guarantees there's not suddenly a variable instead of a string where we're looking for the name...

If you mean a separate callback for skins/skin variants, I tried that but it just gave me a lot of duplicate code only used by a few editors.

subscribe

Marking #949040: Toolbar separators in TinyMCE's O2k7 skin are not floated left postponed for review when this issue is fixed. It deals with some of the theme specific problems mentioned in #30.

subscribe