Companion task to http://drupal.org/node/152046

CommentFileSizeAuthor
#7 img_assist6.wysiwyg.patch7.78 KBsun
#5 img_assist-179712-D5-080630.patch6.66 KBzoo33
#5 img_assist-179712-D6-080630.patch6.59 KBzoo33
#4 img_assist-DRUPAL-5.wysiwyg-plugin.patch48.87 KBsun
#4 img_assist-DRUPAL-6--1.wysiwyg-plugin.patch48.87 KBsun
#3 img_assist-DRUPAL-5.wysiwyg-plugin.patch902 bytessun
img_assist-DRUPAL-5_9.patch910 bytessun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Postponed
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Postponed » Needs review

I'm working on #152046: hook_wysiwyg_plugin() now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new img_assist-DRUPAL-5.wysiwyg-plugin.patch902 bytes

Applied with some fuzz - re-rolled.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new img_assist-DRUPAL-6--1.wysiwyg-plugin.patch48.87 KB
new img_assist-DRUPAL-5.wysiwyg-plugin.patch48.87 KB

Here are two new patches for D5 and D6.
Be sure to test them with latest Wysiwyg Editor code from CVS.

zoo33’s picture

zoo33 commented
Version: 5.x-1.x-dev » 6.x-1.x-dev
StatusFileSize
new img_assist-179712-D6-080630.patch6.59 KB
new img_assist-179712-D5-080630.patch6.66 KB

Alright, I got it working now. However, the patches contain a lot of whitespace changes and cleanup to the editor_plugin*.js files. While I think this cleanup is a good thing, I have isolated the one change that needs to be made in order for the Wysiwyg connection to work so that it's easier to review. I also removed a simple coding-style change to the module code for now.

I made a few additional changes to the README file to reflect the move to Wysiwyg Editor.

A couple of questions:

- The patch introduces FAPI #wysiwyg support which I suppose is used for checking whether other modules are adding a wysiwyg editor to the textarea. Is it always true that you don't want to add the IA link below textareas that use a wysiwyg editor? Or is #wysiwyg specific to the Wysiwyg Editor module?

- Just to clarify: with this patch you won't be able to use the TinyMCE module anymore. I guess your intention is that Wysiwyg Editor will replace that module. Are people in general moving away from TinyMCE in favor of Wysiwyg Editor? Could this be a problem for existing TinyMCE users?

Tested successfully with D6, not tested with D5.

zoo33’s picture

zoo33 commented

By the way, the hook will be a huge gain as it should eliminate support questions about how to install drupalimage. Great work!

This would go into a DRUPAL-5--2 branch, right? We would probably get some testers if there was a 5.x-2.x-dev or beta release.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new img_assist6.wysiwyg.patch7.78 KB

Awesome!

Just fixed two more PHP notices. I think this is ready to go... will create 2.x branches later on.

zoo33’s picture

zoo33 commented

Yeah, I've got nothing to add. Just my question above about the FAPI #wysiwyg property.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The patch introduces FAPI #wysiwyg support which I suppose is used for checking whether other modules are adding a wysiwyg editor to the textarea. Is it always true that you don't want to add the IA link below textareas that use a wysiwyg editor? Or is #wysiwyg specific to the Wysiwyg Editor module?

Well, Wysiwyg Editor introduces the FAPI #wysiwyg property for textareas. However, in it's current state, it's not that different from before - Wysiwyg Editor "enables" a textarea for wysiwyg editing (if a textarea matches the configured visibility settings), and IA checks whether it's enabled (that integration code was hard-coded in TinyMCE module before). The only real difference is that modules like Journal, which injects a textarea into all Drupal forms to allow an administrative log of changes, are finally able explicitly declare that a textarea should *not* be enabled for wysiwyg editing.

Now, IA will still add its button/link text below the textarea in this case then. There's still plenty of work required to prevent this. I've outlined my overall battle plan over at http://groups.drupal.org/node/8707, which (hopefully) gives an idea about this Inline API/Wysiwyg API stuff... In fact, IA is my favorite developer toy for developing those APIs, because it acts somewhere in between and requires both APIs to work.

Just to clarify: with this patch you won't be able to use the TinyMCE module anymore. I guess your intention is that Wysiwyg Editor will replace that module. Are people in general moving away from TinyMCE in favor of Wysiwyg Editor? Could this be a problem for existing TinyMCE users?

Wysiwyg Editor provides an upgrade path (in wysiwyg_editor.install) for users that used TinyMCE module previously/currently. However, I didn't really test it...

zoo33’s picture

zoo33 commented

Wysiwyg Editor "enables" a textarea for wysiwyg editing (if a textarea matches the configured visibility settings), and IA checks whether it's enabled

What happens if a different wysiwyg editor that doesn't have an IA button sets the #wysiwyg property? Wouldn't that make it impossible to access IA? That's my only concern.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Fixed

Well, the middle/long-term goal is that

a) either all contrib modules implementing support for Wysiwyg editors implement Wysiwyg API or
b) Wysiwyg Editor replaces all contrib modules implementing support for Wysiwyg Editors by supporting all editors at once.

I'd prefer and working towards b), but can't really tell when this will be accomplished.

Committed!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

scottrigby’s picture

scottrigby
he/him
Primary language English
Location Brooklyn, NY
commented

Hi Sun, just so I'm clear on this –
For the 5.x version of WYSIWYG module, for img-assist to work we need to apply the img_assist-179712-D5-080630.patch in #5 above to the img_assist module?
Thanks for clarification –
:) Scott

Edit: It seems to work for me -- I just want to make sure this is the right way to go -- sounds like there may be another direction you're discussing and I don't want to head down a path that won't be updated. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Currently, this is only available in the development snapshot 5.x-2.x. You should use that one (from the project page) instead of applying this patch. A beta or release candidate will follow soon.

scottrigby’s picture

scottrigby
he/him
Primary language English
Location Brooklyn, NY
commented

hi sun,
Ah – well, I had already applied the patch above to the 5.x-1.x version...
Just checking once again, since I don't know how massive an update it is... Am I able to just remove the 1x version and add the 2x, then update.php? or is it more involved than that?
Asking because – while the patch with 1x works ok – I assume based on your response that 2x is a better way (will be better supported moving forward, etc)?
Thanks :)
Scott

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I think you can safely use the development snapshot of 2.x. However, be sure to periodically update to later versions then.

toniw’s picture

toniw commented

#14 mentions 5.x-2.x but that version does not seem to exist on the project page. What version should I use to get img_assist to work (for D5)?

zoo33’s picture

zoo33 commented

The dev snapshot of 5.x-2.x is listed on the project page as "5.x-2.x-dev". A stable 5.x-2.x release is not yet available.

toniw’s picture

toniw commented

Ah, I now see my problem. I landed on this issue clicking through from an issue from the wysiwyg project, not realizing that I had arrived in img_assist. I was therefore under the impression that I needed 5.x-2.x of wysiwyg to fix my problem. Sorry for the confusion. Will try 5.x-2.x of img_assist now...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

FYI: #179712 by sun: Partially reverted changes for #wysiwyg FAPI property.

I.e., special checking for #wysiwyg has been removed. Wysiwyg API no longer uses this property.