Problem/Motivation (as of #137)

Media supports inclusion of files into WYSIWYG areas. WYSIWYG editor renders files using the same view mode as it will be later used in rendered entity display. This is sometimes appropriate, but it can lead to inadequate results if a non-image formatter is used. This commonly surfaces with document and audio files that might have a text link. Once inserted, these are no longer editable from within WYSIWYG.

This issue tries to solve that problem by introducing the ability to configure the default view mode used in WYSIWYG itself. This would still allow the usage of non-image formatters for rendered entities, but could be replaced with some sort of a default image formatter (e.g. maybe a thumbnail of the PDF) in WYSIWYG.

Proposed resolution

  • Merge functionality from view_mode sub-module into media_wysiwyg.
  • Store WYSIWYG View Mode configuration in a db table.
  • Make WYSIWYG View Mode exportable via Features.
  • Add a UI field to entity file display form to disable view modes in WYSIWYG. (admin/structure/file-types/manage/%file_type/%file_display)
    restrict view modes
  • Add a UI field to entity file file type form to set a default view mode in WYSIWYG. (admin/structure/file-types/manage/%file_type)
    default WYSIWYG view mode

Remaining tasks

Reviews and testing of #166.

User interface changes

- adds configuration for wysiwyg view modes

API changes

n/a

Original report by Devin Carlson

An offshoot of #1283844: [meta] Improve WYSIWYG integration, Media cannot currently embed non-images using WYSIWYG. The current WYSIWYG code requires an image to be output or it will not allow you to submit the "add media" form after selecting a view mode for a chosen piece of media.

The reason the embedding of non-image media is broken is because the current WYSIWYG code tries to render the chosen media using the media player that is currently configured for the selected view mode and place the resulting HTML into the editor. In many cases this HTML will not include an image.

Media places a "token" into the WYSIWYG editor to represent a file. Similarly, it should also display an image "token" to represent the file instead of inanely trying to render the actual media. Custom plugins for Media may output iframes, divs or simply hundreds of lines of HTML; all things that don't belong directly in the WYSIWYG.

In order to allow embedding of non-image media, I propose that Media does the following:

  • Assume that a file displayed using the "preview" view mode outputs an image
  • Use whatever image is outputted by a file's preview view mode as a "token" to represent the media when it is in a WYSIWYG editor
  • Display the file appropriately when outside of WYSIWYG
CommentFileSizeAuthor
#208 interdiff_205-208.txt477 bytesjoseph.olstad
#208 media-custom_view_modes_for_wysiwyg-1792738-208.patch34.86 KBjoseph.olstad
#205 media-custom_view_modes_for_wysiwyg-1792738-205.patch34.81 KBjoseph.olstad
#203 media_wysiwyg_view_mode-view_modes_fix-1792738-201.patch394 bytesTom.Camp
#200 media_wysiwyg_view_mode-view_modes_fix-1792738-200.patch406 bytesafsch
#199 media-custom_view_modes_for_wysiwyg-1792738-199.patch35.87 KBcircuscowboy
#185 media-custom_view_modes_for_wysiwyg-1792738-185.patch38.17 KBazinck
#184 media-custom_view_modes_for_wysiwyg-1792738-184.patch38.21 KBazinck
#174 media-custom_view_modes_for_wysiwyg-1792738-174.patch39.82 KBmstrelan
#173 media-custom_view_modes_for_wysiwyg-1792738-173.patch39.82 KBmstrelan
#166 media-custom_view_modes_for_wysiwyg-1792738-166.patch39.75 KBheddn
#164 media-custom_view_modes_for_wysiwyg-1792738-164.patch32.21 KBheddn
#160 media-custom_view_modes_for_wysiwyg-1792738-159.patch39.81 KBmansspams
#154 restrict.jpg2.79 KBheddn
#154 default.jpg6.64 KBheddn
#147 1792738-146-147.txt2.41 KBklokie
#147 media-custom_view_modes_for_wysiwyg-1792738-147.patch42.15 KBklokie
#146 interdiff_145-146.txt1.05 KBheddn
#146 media-custom_view_modes_for_wysiwyg-1792738-146.patch39.81 KBheddn
#145 interdiff_143-145.txt1.78 KBheddn
#145 media-custom_view_modes_for_wysiwyg-1792738-145.patch40.41 KBheddn
#143 media-custom_view_modes_for_wysiwyg-1792738-143.patch40.04 KBheddn
#141 interdiff_139-141.txt1.59 KBheddn
#141 media-custom_view_modes_for_wysiwyg-1792738-141.patch40.11 KBheddn
#139 media-custom_view_modes_for_wysiwyg-1792738-139.patch39.44 KBheddn
#139 interdiff_137-139.txt543 bytesheddn
#137 media-custom_view_modes_for_wysiwyg-1792738-137.patch39.57 KBheddn
#137 interdiff_134-137.txt3.23 KBheddn
#134 media-custom_view_modes_for_wysiwyg-1792738-134.patch24.81 KBheddn
#134 interdiff-132-134.patch2.98 KBheddn
#132 media-custom_view_modes_for_wysiwyg-1792738-132.patch38.28 KBheddn
#132 interdiff_130-132.txt11.08 KBheddn
#130 media-custom_view_modes_for_wysiwyg-1792738-130.patch36.51 KBheddn
#120 1692738-120-no_wysiwyg_view_mode.png82.89 KBfengtan
#120 1692738-120-thumbnail_sticks_to_left.png186.1 KBfengtan
#120 media-custom_view_modes_for_wysiwyg-1792738-120.patch34.32 KBfengtan
#116 interpatch-115-116.diff1.82 KBfengtan
#116 media-custom_view_modes_for_wysiwyg-1792738-116.patch34.23 KBfengtan
#115 media-custom_view_modes_for_wysiwyg-1792738-115.patch33.58 KBMirabuck
#112 media-custom_view_modes_for_wysiwyg-1792738-112.patch33.47 KBiwayMan
#108 media-custom_view_modes_for_wysiwyg-1792738-108.patch26.13 KBiwayMan
#105 media-custom_view_modes_for_wysiwyg-1792738-105.patch13.78 KBaaron
#100 restrict_wysiwyg.png126.77 KBfengtan
#100 wysiwyg_view_mode.png100.96 KBfengtan
#100 media-custom_view_modes_for_wysiwyg-1792738-100.patch22.97 KBfengtan
#94 media-custom_view_modes_for_wysiwyg-1792738-94.patch23.29 KBfengtan
#88 media-custom_view_modes_for_wysiwyg-1792738-88.patch23.37 KBfengtan
#82 custom-view-modes-for-wysiwyg-1792738-82.patch15.79 KBfengtan
#74 custom-view-modes-for-wysiwyg-1792738-74.patch3.72 KBaaron
#68 updated 2013-07-17 16-25-49.jpg64.92 KBgmclelland
#67 popup 2013-07-17 16-14-53.jpg108.72 KBgmclelland
#61 1792738-media_wysiwyg_view_modes-61.patch9.84 KBaaron
#58 1792738-media_wywiswyg-view_modes-58.patch0 bytesaaron
#22 media-n1792738-22.patch1.26 KBmrfelton
#19 media-n1792738-19.patch1.06 KBDamienMcKenna
#15 use-preview-mode-if-not-image-1792738-15.patch1.06 KBRob_Feature
#7 use-preview-view-mode-with-wysiwyg-1792738-7.patch937 bytesDevin Carlson
#1 use-preview-view-mode-with-wysiwyg-1792738-1.patch949 bytesDevin Carlson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Devin Carlson’s picture

Status: Active » Needs review
FileSize
949 bytes

A patch to implement the change.

brunodbo’s picture

Works great. After applying this patch, I was able to embed a pdf file using the media browser in the WYSIWYG editor. In the WYSIWYG editor field, the file appeared as a placeholder media icon; when viewing the node, it displayed as a pdf icon with a link to the file.

One question (which probably should be dealt with in a separate issue): would it be possible to use smaller placeholder icons in the WYSIWYG editor? They look very big right now in the editor, especially when embedded in a paragraph of text.

RobW’s picture

If the final output for a PDF is an image in this case, why not use that same image in the WYSIWYG?

Devin Carlson’s picture

RobW: It does. You can enable a formatter that returns an image for the preview view mode and it will be used inside of the WYSIWYG.

For example, if you enable the "Image" formatter for the "preview" view mode of the "Image" file type, an image preview will be inserted inside of the WYSIWYG instead of a filetype icon.

This also works for other filetypes. If you want to see previews of PDF files inside of the WYSIWYG instead of file type icons, install an appropriate formatter that returns an image, such as PDFPreview (with some slight modifications) and Media will insert a preview image of the PDF file into the WYSIWYG.

brunodbo: if people are relatively satisfied with this method, I would be happy to work on creating preview formatters for different file types or at least providing better (more appropriately sized) default file type images for files embedded with WYSIWYG.

Devin Carlson’s picture

Just took a look through the issue queue, this would fix/address concerns/improve:

ParisLiakos’s picture

Devin Carlson’s picture

mrfelton’s picture

Status: Needs review » Needs work

Can not insert PDF files even with this patch. I can't even select them in the media popup. Note, I have enabled the Document filetype for use with Media WYSIWYG at /admin/config/media/browser. I can see the PDFs in the media WYSIWYG popup. But, I can not select, nor insert them.

Devin Carlson’s picture

Status: Needs work » Needs review

@mrfelton, you'll need to make sure that you've enabled a formatter that returns an image for each file type's "preview" display mode.

To embed a pdf file, try enabling the "large filetype icon" formatter for the Documents file type.

I'm mainly interested in seeing if this idea has any support. I suppose I should look into doing something like enabling the "large filetype icon" formatter for every filetype's preview view mode by default. Or at least changing the fallback/default display from "generic file" to "large filetype icon".

martins.bertins’s picture

Setting the "large filetype icon" formatter for "Document" file type "Preview" display mode really fixes the problem for selecting and embedding pdf and other documents from media library. Just the thing that I needed.
But I ran into another problem. First we had Media and File Entity module versions 7.x-2.0-unstable3, I updated both of them to latest dev and added #7 patch to fix the pdf embed problem. So far so good, but now the problem is that other document files that were added to media library before module upgrade still are not selectable and use the old display style. There is no problem with images added before module upgrade.
Screenshot: https://skitch.com/becizzz/e2t65/luo-sivu-innoomnia.fi

martins.bertins’s picture

Status: Needs review » Needs work

also changing the status

Devin Carlson’s picture

Status: Needs work » Needs review

@martins.bertins your issue is caused by #1496624: Provide an upgrade path for media types to file_entity types. I'd suggest testing the latest patch in that issue and see if it fixes the problem (and please report your feedback, it would be much appreciated).

martins.bertins’s picture

Thanks Devin for pointing to that issue. Used the patch from it and now got everything working.

Rob_Feature’s picture

If I understand this correctly, all of this "just use the preview mode" stuff works on every file type...INCLUDING on images?!?

I'd say this behavior should work on every other type of file, but leave images alone to work how they do now. It's often very important that we see the actual output size of an image when inserting it into content. It allows us to determine how much space it's sucking up and give a reasonable preview of what we're embedding. Always inserting the same size image preview seems like a really bad idea all the way around.

If possible, I'd suggest this patch target all file types except images...but leave images alone.

Rob_Feature’s picture

Here's a patch that basically does this same thing, but only if it's not an image file type (for those we use the existing behavior).

Kazanir’s picture

We really need to find a way to merge this with rootatwc's patch here: http://drupal.org/node/1451316

This patch pre-selects the Preview view_mode for use inside the WYSIWYG editor, while rootatwc's patch above renders the output as it will actually output on the page. Sometimes one or the other is preferable and it would be ideal to be able to pick between them somehow. This would elegantly solve the problem that Rob is hacking on above.

OTHER than that issue this patch works great and is very useful for being able to embed documents and other image-less content in the first place.

Devin Carlson’s picture

For anyone following this issue, I've setup a sandbox project which enables you to choose a select a view mode to use, per file type, when rendering media inside of the WYSIWYG editor. Combined with Entity View Mode, this should be able to accomplish what Rob_Feature described in #14.

Project: Media WYSIWYG View Mode

If you're unfamiliar with applying patches, you can use the sandbox project to test out this approach to WYSIWYG and see what you think.

Rob_Feature’s picture

I've tested Devin's Media WYSIWYG View Mode and I REALLY think this is the future....a couple small tweaks (which he said he's doing today) and it's exactly what's needed.

I propose closing this issue in favor of moving in the direction of that module...

DamienMcKenna’s picture

FileSize
1.06 KB

An updated version of #15 that a) adheres to the coding standards ;) b) has only one call to media_get_file_without_label().

DamienMcKenna’s picture

The separate sandbox brings up the question of: where do you draw the line between functionality to be included in the core module vs a separate module? The functionality is quite small, most of the code could be dropped if the functionality was just rolled in as an option on the file display settings.

ParisLiakos’s picture

I think this is a very good idea..i tried today to insert some non standard filetypes to wysiwyg and if i was selecting displays which are not standard..eg not generic file...there was clearly a problem..this approach fixes this, but it somehow does not feel right to me..
I am gonna think about it a bit more in the next few days.

mrfelton’s picture

FileSize
1.26 KB

Here is the same patch (I think), updated to work with latest media code.

ShaunDychko’s picture

Status: Needs review » Reviewed & tested by the community

Patch works for me, thank you!

Anonymous’s picture

Thanks for the patch - working great for me.

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs review

i think we should discuss this a bit more..this approach feels very aggressive when embedding stuff that would otherwise work when embedding..eg file link, not just images..i would also love to hear other people opinions, especially other maintainers..
instead of whitelisting stuff that can be displayed in a wysiwyg editor in media module, maybe we should introduce a new property for formatters, eg sth like
'wysiwyg_view_mode' => 'preview'
that are known not to work inside wysiwyg..

what do you think?

mrfelton’s picture

I think something like that makes sense @rootatwc. The more control that individual formatters have over the specifics of their wysiwyg integration the better. Media should not really assume anything one way or the other, although sensible defaults are good.

SebCorbin’s picture

#22 Patch solve PDF linking issue here

Rob_Feature’s picture

I'm getting nervous, honestly, that no one is looking at Devin's Media WYSIWYG View Mode module, which really solves all of these problems perfectly....PLEASE try it and feedback to the Media team. I now find that I can't use Media/WYSIWYG without it and I really REALLY think it (or parts of it) should be considered as being added to Media core....

http://drupal.org/sandbox/DevinCarlson/1823634

PS: Note a small bug creeped in recently but it's still an essential for me: #1886234: Restrict View Modes Broken

rooby’s picture

I have installed the sandbox at http://drupal.org/sandbox/DevinCarlson/1823634 and with pretty much no configuration required I could do what pretty much every client of mine wants...

Add a pdf file via WYSIWYG and have it embed inline as a link to the file.

Fantastic, and it works in a logical manner.

I have not reviewed very thoroughly yet though.

ParisLiakos’s picture

cosmicdreams’s picture

What is the way forward? Package up Devin's work as a patch?

Dave Reid’s picture

The sandbox looks good, but I think we can still go with a simplified approach that can be easily extended. Why not make which view mode is forced to be in the WYSIWYG a variable, with preview as the default value, but not exposed in the UI.

Basically implementing https://gist.github.com/3012194 but in the Media module.

gmclelland’s picture

Yesterday I tried out the Media WYSIWYG View Mode module at http://drupal.org/sandbox/DevinCarlson/1823634 and I really liked how it worked.

I just wanted to point out that it also provides another big feature that enables you to see an ajaxified preview of the view mode selector after choosing a media item from the media browser before the media is inserted into the wysiwyg.

@Dave Reid's suggestion sounds good too.

I think it would be beneficial to set a message when editing the "Preview" mode at "Manage File Display" that says something like this view mode is used for previewing non-image media that is inserted into a WYSIWYG editor. This could help newcomers who question what the view mode does.

Rob_Feature’s picture

It's important to be able to select, per file type, whether we use a special wyiswyg view mode or not (in the UI). For example: I always want to use the "WYSIWYG preview view mode" for audio, video, and documents. But I NEVER want to use it for images because I want to truly see the view mode (ie imagestyle) for images...forcing these to use a single view mode would be extremely unfriendly.

RobW’s picture

Choosing in the UI seems a bit overkill. IMO this is a place where decisions trump options; media provider modules deciding in code seems like it would create the best Drupal sitebuilder/user ux.

gmclelland’s picture

I just wanted to point out that the Media WYSIWYG View Mode at http://drupal.org/sandbox/DevinCarlson/1823634 allows the ability to select which view mode to use on the media item that is going to be inserted into the WYSIWYG editor. If the media item is an image, the module will initially display the image above the view mode selector in the media browser modal dialog.

Should that initial preview use the "Preview" view mode as well? Right now it shows the image at full size see #1901114: Default view mode for previews initially until someone chooses another view mode and it updates the preview via ajax.

So my question is where will the "Preview" view mode be used? For clarification, where is it currently used now?

- in the WYSIWYG editor (except the image file type)
- possibly above the view mode selector when using Media WYSIWYG View Mode module?
Should these use the same view mode?

Also note that the Media WYSIWYG View Mode module also adds a "WYSIWYG" view mode.

Rob_Feature’s picture

gmclelland you're correct, and keeping this stuff in the UI is what I advocate for. There's such a mix and match of how this could be used (and it's useful works great) that stripping it out seems strange.

RobW, I'm not trying to be difficult, but have you used Devins module in a real world circumstance? It's SOOO perfect in how it addresses all the issues that I fear if it goes away it makes media/wysiwyg actually LESS flexible than it is now.

I guess all I'd advocate for would be to do this in a way that something LIKE Devin's module could continue to exist as a separate contrib...it's a crucial piece to this puzzle. (basically I have no problem either way, as long as something like Devin's module COULD exist...it doesnt matter to me if it's in media core or not)

rooby’s picture

+1 for #37

RobW’s picture

Re: #37, I have no strong feelings against it, I just prefer to make the choice in code. If others find it useful, go for it.

Kazanir’s picture

Devin's module is the way to go. Simplifying it down to a toggle of a hard-coded preset view mode or making the choice in code is not a good UX for the average Drupal site administrator -- one of the audiences that benefits MOST from a module like Media. This is doubly especially since so many other site-administrator-level modules in D7 (Views, Panels, Display Suite, Media, et al) rely on picking view modes and configuring them to do what you want. If I had the coding chops I would even expand Devin's module to work like this:

[File Bundle WYSIWYG Configuration]

[Checkbox] Enable $view_mode when inserting via WYSIWYG? If yes, enable [Select List of $wysiwyg_modes] Replace $view_mode with $wysiwyg_mode while inside editor?
[...] for each $view_mode

This would allow the site administrator to replace problem-causing view modes (such as video players or colorbox links) with image-only view modes while inside the editor frame, but not mess with "friendly" view modes at the same time.

RobW’s picture

In the example in #40, it seems to me that replacing a malfunctioning wysiwyg insert with another vm is the responsibility of the media provider module maintainer. If an insert doesn't work it's a bug, not config. As a person who both builds modules and builds sites, I don't want to do any more config, especially where choices don't offer any significant improvement for the majority of people.

But there seems to be consensus, so do yo thang.

rooby’s picture

I can definitely see both sides.

I like the configurability of the sandbox, however as an example, in the case of adding a pdf inline, if it defaulted to uploading the pdf and inserting it in the text as a link to the pdf (what I imagine would be the 99% use case for pdfs) I would be even happier and would not need to touch the config 99% of the time.

So I definitely see where RobW is coming from too.

Rob_Feature’s picture

I want to suggest again that, if there's some big proponents of just hard coding this setting in code that's ok....IF (huge 'if' here) it can be overridden easily by an external module such as WYSIWYG View Mode.

The way I see it, this has to be fixed, somehow, in Media core. The module needs to be able to embed all these different types, in some way, by default. This means something has to go into media to make that work better by default.

But, what we're talking about here is exactly how that's added. If there's truly proponents for a hard coded preset, I'd suggest going in that direction but always keeping in mind that another module MUST be able to hook in and do what Devin's mod does in the UI.

Something like that seems the best of both worlds if there are truly a bulk of folks who advocate for no UI. On the other hand, if the mass agrees that a UI component makes the most sense, I certainly throw my weight behind that direction.

gmclelland’s picture

Dave Reid’s picture

Can we add the functionality of the sandbox without exposing the UI of it?

slashrsm’s picture

Title: Assume the preview view mode outputs an image, use the preview view mode with WYSIWYG, enable embedding of any file type » Allow custom file view modes for WYSIWYG display
Category: task » feature
Status: Needs review » Needs work

Issue summary made me think that it is currently only possible to include image files in WYSIWYG. This is however not true. Using latest -dev of Media and File entity I was able to include PDF document that used Embedded Google Docs Viewer for display. Everything worked OK (GDoC viewer on rendered node and iframe placeholder in WYSIWYG).

Since I agree it is not always the best solution to use same formatter for wysiwyg and rendered node, I still believe that this should be the default behaviour (for similar reasons that were mentioned in #42).

Devins sandbox implements very handy feature, which could be included in Media or it could also live in contrib. It is up to us to decide about this. I am convinced that we also need to provide UI for it if we decide to pull it in Media core. As already mentioned; Media is frequently used by site builders who rely on UI for configuration. Offering good experience for them will definitely speed up the adoption of Media IMO.

Anyone interested in IRC/Hangout meeting about this issue where we could hopefully find and agree on a solution for this?

azinck’s picture

slashrsm: count me in for an IRC or hangout get-together on this issue.

slashrsm’s picture

I suggest that we discuss this on next regular meeting: http://groups.drupal.org/node/286568

slashrsm’s picture

There was a decision made about this issue at yesterday's meeting.

We are going to pull Devin's module in Media. We only need to change configuration stuff a bit. We'd like it to be save in a file type (and being exportable) instead of having things is separate variables.

Let's do it! We have just some small bits left to reach alpha1.

slashrsm’s picture

@Devin Carlson: You are still marked as assignee for this issue. Are you still interested in preparing a patch for this?

slashrsm’s picture

Issue summary: View changes

Update issue summary

chrisschaub’s picture

Any movement on this integration of Devin's sandbox module?

chrisschaub’s picture

I could roll his module as a patch to 2.x dev if needed this morning. I think the only discussion was to move the variables to exportables? Otherwise a straight port -> patch?

chrisschaub’s picture

After some more digging, wouldn't it be better for sites to use ...

http://drupal.org/project/entity_view_mode

just make it a dependency of media? That way users have total flexibility?

azinck’s picture

cschaub -- this issue is about setting rules around which view modes are allowed to be used in WYSIWYG as well as what view mode is used to replace the macro Media injects into the text area when using the WYSIWYG. The needs here go beyond what Entity View Mode provides.

chrisschaub’s picture

I don't see much difference at first glance, I tried both approaches. The only differece I saw was the ability to restrict which view modes are shown. In the end, the module is just creating a view mode. I'll dig deeper to see what I'm missing.

rooby’s picture

Entity view mode only allows you to create new view modes.

This issue is about using those view modes when inserting media via the WYSIWYG.

Use both together for happiness.

Dave Reid’s picture

Issue tags: +7.x-2.0 alpha blocker
aaron’s picture

Status: Needs work » Needs review
FileSize
0 bytes

This is a straight add of the sandbox module.

slashrsm’s picture

Status: Needs review » Needs work

Empty patch....

gmclelland’s picture

@aaron - Can you attach the correct patch?

aaron’s picture

Status: Needs work » Needs review
FileSize
9.84 KB

woops, sorry about that!

supradhan’s picture

Is this patch is same as sandbox project: https://drupal.org/sandbox/DevinCarlson/1823634?

gmclelland’s picture

@ClsSharp - I haven't tested the patch in #61 yet, but it looks like the code is the exact same except now it lives under the Media module as a separate Media WYSIWYG View Mode module.

Dave Reid’s picture

Moving to a beta blocker. I don't think feature requests should hold up a long-awaited alpha release.

Dave Reid’s picture

Issue tags: -7.x-2.0 beta blocker
Dave Reid’s picture

Issue tags: +7.x-2.0 beta blocker
gmclelland’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
108.72 KB

Just tested #61 on a fresh install. Everything is working great.

These recommendations could be added in followup issues:
1. Maybe change the word "restrict" to "disable" at admin/config/media/wysiwyg-view-mode?
2. Add a default view mode selection per file type - since the view modes are sorted alphabetically, someone might have a view mode with a large image. You don't want that person to always see a large image as the default selection.

popup 2013-07-17 16-14-53.jpg

I tested with the latest media and file_entity-2.x-devs and with Ckeditor and TinyMCE.

Hope that helps

gmclelland’s picture

Issue tags: -Needs screenshots
FileSize
64.92 KB

Nevermind, all is working well.

On an older install of file and media 2.x-dev, it would automatically display the viewmode.
On the newer versions of file and media-2.x-dev, it shows just a link with the file name by default and doesn't update the preview until you actually make a selection. Very nice

Here is a screenshot.

updated 2013-07-17 16-25-49.jpg

aaron’s picture

Status: Reviewed & tested by the community » Fixed
Dave Reid’s picture

Priority: Major » Critical
Status: Fixed » Needs work

I don't see the changes that were requested in comment #49 were actually made before this was committed.

Dave Reid’s picture

Also I think the intent was to merge this into media, and not actually have it as a sub-module, but that may not have been very clear.

aaron’s picture

Assigned: Devin Carlson » aaron

Sorry I jumped the gun on that. I will roll a new patch soon.

bgilhome’s picture

I added a default view mode per file type to Devin's sandbox at https://drupal.org/sandbox/DevinCarlson/1823634 - see patch and comment at https://drupal.org/node/1901114#comment-7686315. Once the above patch is re-rolled I could upload a new patch from that.

aaron’s picture

I have made some progress in the sense that I can now save the restriction state but I am having troubles figuring out how to load it again. There appear to be no hooks available during the file type load. I would appreciate if someone could help point me in the right direction. I am largely unfamiliar with the internal workings of the cTools module.

drupal_was_my_past’s picture

Commit 7d6a333 reintroduced a bug from #2003810: When editing a file in the WYSIWYG, the "Display as" view mode selection does not inherit the default value where the Display As select form no longer inherits the default value. I've added a patch in comment #3, but I'm not sure if it is needed if @aaron is going to be reworking media_wysiwyg_view_mode module into the media module itself.

drupal a11y’s picture

Hi All,
as Designer & Sitebuilder I see some issues:

  • What happens when I need a lot different image sizes? With "image styles" and an allowed selection I think this could be solved better than with creating much displays. Is there a way to have a dynamic selection of an image style within one display?
  • How to easy apply display-effects like e.g. "colorbox" on some images?
  • How to avoid problems when migrating or export/import nodes with wysiwyg-images

Please keep in mind I still need to figure out the latest inventions and that maybe I currently have a wrong understanding cause I am not deeply involved in the progress of the past month.

Found these modules on that topic:

drupal a11y’s picture

On my research & tests I also found this older issue by David Reid where support for "WYSIWYG Fields" was discussed: https://drupal.org/node/1664418

The ease of the "insert"-module combined with the media-browser by supporting this module would be really great.

If I can help somehow with Design-Drafts, Mockups, HTML/CSS please let me know.

drupal a11y’s picture

Another D6 module which has a some UI/UX goodies on inserting images is https://drupal.org/project/img_assist

aaron’s picture

mori, thank you for your input. However, it would be appreciated to open a new issue rather than hijacking an existing issue. That helps to keep things clear.

gintaras.r’s picture

Sorry if this is a distracting question, but I'd like to know how these changes might integrate with the Spark project. As far as I can tell, Spark uses a CKEditor module as a wrapper instead of the WYSIWYG module. But WYSIWYG is a dependency for this new sub-module. A more general question might be, will site admins miss out on this functionality if they don't go through the wysiwyg module to set up their wysiwyg editor. Is Spark not following convention by doing that?

StevenWill’s picture

Any progress on this issue?

fengtan’s picture

Following #74: Could not find a way to hook into file_type_load() either. We found a workaround by altering the 'subrecords callback' from ctools, though there may be a better way to do this.

Attached is a patch that moves the config stuff from separate variables to file types (see #49).

We added some documentation while we were at it. Also, we removed the 'restrict' checkbox from the default view mode since it is always presented to the user, but we can change it if this was not expected.

Maybe this can be improved before we move it into media module.

aaron’s picture

Assigned: aaron » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Thanks for getting the ball rolling again, @Fengtan. This approach looks fine to me. I am setting the status to "needs review" so that we can get the testbot's thoughts on the matter. However, I think that we still need to move it all into the media module to be able to wrap it up.

aaron’s picture

I'm not certain how the tag was removed. I'm adding it back in.

igorik’s picture

Status: Needs review » Needs work

After I manually applied patch I got this message:
Fatal error: Call to undefined function file_type_load_subrecords() in ...../sites/all/modules/media/modules/media_wysiwyg_view_mode/media_wysiwyg_view_mode.module on line 137

rooby’s picture

@igorik:

What version of the file_entity module are you using?

igorik’s picture

I tried it now with latest media and file entity (7.x-2.0-alpha3) and still the same problem

igorik’s picture

Issue summary: View changes

Update summary.

fengtan’s picture

Following #83: Re-rolled the patch so the 'wysiwyg view mode' code lives in the media module. Hope it helps.
@igorik not sure how you got this message, file_type_load_subrecords() seems to be a valid function.

gmclelland’s picture

gmclelland’s picture

micbar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 88: media-custom_view_modes_for_wysiwyg-1792738-88.patch, failed testing.

alison’s picture

From @gintaras.r's comment (#80):

As far as I can tell, Spark uses a CKEditor module as a wrapper instead of the WYSIWYG module. But WYSIWYG is a dependency for this new sub-module. A more general question might be, will site admins miss out on this functionality if they don't go through the wysiwyg module to set up their wysiwyg editor.

My understanding was that D8 would be using CKEditor without the WYSIWYG "integrator" module, but the proposed fix requires said module. It doesn't seem like the best plan to me -- easy for me to say -- but, if it's the path you need/want to take, (a) I think this issue title/description should be edited to clarify that the thread/proposed fix is for integration with the WYSIWYG module (trying to discern "WYSIWYG module" vs. "WYSIWYG with a theoretical lowercase W" in issues/forums is so confusing); and, (b) I question whether the it should be in Media core, therefore making WYSIWYG a dependency of Media. (Or am I misunderstanding the intent of the proposed fix?)

I realize you've already given most if not all of this a great deal of thought, but I'm throwing in my two cents anyway.

fengtan’s picture

Looks like file_type_load_subrecords() does not exist anymore in file_entity-7.x-2.0-alpha3.
Here is a new patch that might pass tests.
If anyone knows a kind of hook_file_type_load() that would be awesome.

micbar’s picture

Status: Needs work » Needs review
Mirabuck’s picture

Component: WYSIWYG integration » Code
Related issues: +#2142571: Spin-off WYSIWYG support in dedicated module / project
Mirabuck’s picture

Adding #2142571: Spin-off WYSIWYG support in dedicated module / project as a related ticket as I suspect it has significant implications for this one. Given that #94 was based on the pre-2142571 module structure, it no longer works on top of the most recent dev and will need to be reworked.

Mirabuck’s picture

Status: Needs review » Needs work
Mirabuck’s picture

Given what's happened in #2142571: Spin-off WYSIWYG support in dedicated module / project, what are the current plans for media_wysiwyg_view_mode module? Will it remain separate from media_wysiwyg (in which case patch #82 above may be close to what we need) or should it be integrated with media_wysiwyg (in which case we need a significantly refactored patch)?

fengtan’s picture

Re-rolled patch so it applies against the current -dev.
Attached are a few screenshots of how it should look like.
Note that #2075123: Media browser inserting HTML tag rather than media tag makes the wysiwyg view mode ignored when using Firefox -- but there is a patch that seems to fix it.

fengtan’s picture

Status: Needs work » Needs review
camdarley’s picture

#100 works! Thanks.
But there is still 2 things that I don't understand:
- Why the default view mode is set to "full"? As mentioned in #1901114: Default view mode for previews it's not very easy to embed very large images.
- Should there be an option to display the image in the selected view mode, once embedded in the wysiwig, instead of a default view mode? It may be very confusing for users who chose "link" or "preview" and have a large image in their wysiwyg instead.

fengtan’s picture

Thanks for your feedback camdarley.

  • Think a default display different from 'full' would make sense, though we would have to rework media_wysiwyg as well.
  • The embedded image can be displayed in the selected view mode by disabling the submodule. You would lose the 'restrict' checkbox though. Maybe we could bring the 'restrict' checkbox in media_wysiwyg eventually.
  • Also we are wondering whether we should keep the 'WYSIWYG' file display that this submodule adds, which does not seem to be used.
Mirabuck’s picture

I suspect patch 100 has introduced a markup issue of some sort. Without the module enable, media's modal window works fine for me. With it enabled, in chrome, the scrollbar disappears and the styles look a bit off. Turning off the view modes module restores regular the regular look and feel.

Looks like this was an unrelated issue on my end. Works as expected for me currently. At first I didn't understand why we needed an extra WYSIWYG view mode in addition to the toggles and settings this module adds, but now I'm figuring it allows us to have an additional wysiwyg-specific view mode if none of the existing ones are appropriate.

aaron’s picture

Here is a re-roll that integrates your work with the Media WYSIWYG module.

Status: Needs review » Needs work

The last submitted patch, 105: media-custom_view_modes_for_wysiwyg-1792738-105.patch, failed testing.

Mirabuck’s picture

With additional testing it's become clear to me that the new config patch 100 adds is not currently featurizable.

iwayMan’s picture

Status: Needs work » Needs review
FileSize
26.13 KB

Re-rolled #105 by removing media_wysiwyg_view_mode module and references.

aaron’s picture

If the patch works, then we can put it in, and featurize it later.

dddbbb’s picture

I agree with #109 providing it wasn't "featurisable" in the first place (I'm guessing it wasn't as this is new functionality).

Mirabuck’s picture

The original sandbox module used a variable and could therefore be featurized via strongarm. When the data was moved into a custom table we lost this.

We need featurization pretty desperately on our end. iwayMan is most of the way through a reroll that adds it.

iwayMan’s picture

Here's another patch. Contains everything from #108 and adds featurization of media_wysiwyg in new file includes/media_wysiwyg.features.inc.

Mirabuck’s picture

#112 works for me as expected. It looks like it leaves part of the old view modes module in place, however, which we may want to remove.

Someone other than me should probably verify this given that iwayMan and I are working together on the same project.

Mirabuck’s picture

After working with this for a few more weeks a few things stand out to me:

  • we need the ability to choose not to use a WYSIWYG view mode for certain file types, particularily images. I'm putting a patch together for this now.
  • I'm encountering some weird behaviour wherein inserted images are getting height and width attributes applied to them that don't relate to chosen image styles. This behaviour doesn't happen with the unpatched current dev release and does occur once the patch is applied so I'm pretty confident it's coming from this patch.
Mirabuck’s picture

The attached patch takes what we had in 112 and provides an option to not use a WYSIWYG view mode. This seems to work as expected. What I can't figure out though is how to ensure that when WYSIWYG view modes are disabled images get rendered within the WYSIWYG appropriately. Seems like the following snippet returns receives 'default' as a view mode even when a user has chosen another view mode in the WYSIWYG modal--maybe it doesn't get updated in PHP because the modal stuff is happening in JS? I'm not certain what goes on in the background with the modal stuff.

else {
      $element = media_wysiwyg_get_file_without_label($file, $tag_info['view_mode'], $settings);
    }

Removing the snippet altogether seems to have no effect.

fengtan’s picture

We found an issue when displaying images using a view mode with a larger width/height than the wysiwyg mode. The image would always be displayed using the wysiwyg mode.
Rolled a new patch with a possible fix.

igorik’s picture

Hi, is this already implemented in Media module, or not yet?

bneil’s picture

igorik - the patch above is not included in the module yet - please help us test it and provide feedback so we can include it soon.

Mirabuck’s picture

Status: Needs review » Needs work

This patch (116) works, but doesn't fix the issue with images always being rendered with the default view mode in the WYSIWYG.

It also pushes preview images back into the top of the modal (a departure from the current dev behaviour).

fengtan’s picture

  • Added an option not to use view modes in the wysiwyg box
  • Made thumbnails stick to the left

Attaching screenshots. Hope it helps.

The last submitted patch, 115: media-custom_view_modes_for_wysiwyg-1792738-115.patch, failed testing.

The last submitted patch, 116: media-custom_view_modes_for_wysiwyg-1792738-116.patch, failed testing.

The last submitted patch, 116: interpatch-115-116.diff, failed testing.

Mirabuck’s picture

Patch 120 works for me in terms of allowing file types to be properly excluded from view modes.

Nor sure about the UX stuff. Is the desire to have an unchanging preview image in the LHS margin or to have preview images change based on what display mode's been chosen?

If an unchanging image image desired, we probably need to tweak the patch a bit to accomplish this. If a changing image is desired, putting it back on top may make more sense as otherwise the form gets pushed over significantly with larger sized images.

meecect’s picture

Sorry if my question is too unrelated to this issue, But I noticed that even if I choose to show a pdf insert, for example, and choose to display it as 'link', I get a link to tfile with a small filetype icon, but the user is unable to change the link text.

I'm basically trying to recreate the old 'insert' module function here and I have numerous users that really preferred that workflow to the media centric workflow.

There, they would upload their file and attach it to the document, and then insert a link to it in their content wherever they wanted, and then edit the link text to fit their needs.

Of course, there are limitations to that workflow like inevitable broken links, multiple copies of the same attached file, etc, but it was simple and it worked.

Using the media approach and the work done in this thread is almost there, and does allow one to customize how the content will be rendered, but still within the limitations of the view modes.

I guess a solution would be to add a 'title' field to the file entity and create a view mode for it that rendered as the 'title' with an href to the file URL, but even then, the title would stay the same across the site where the entity is referenced.

If this is too off-topic, can anyone suggest a solution, or another item in the issue queue that may fit my use-case? IE, being able to easily insert a link to content you have uploaded and control the text of the link?

Thanks,

azinck’s picture

@meecect: Add the title field to the file entity. When you insert it in the WYSIWYG you get the chance to edit the field and that edit is preserved as a local override of the global value.

jmuzz’s picture

I've been having some success with the latest dev, but after applying #120 here embedding an image no longer includes the fields attached to the image entity, it is just an image tag now.

delmarr’s picture

Maybe someone can verify this.
When adding an image via view-mode into the wysiwyg the image displays with the appropriate size.
There seems to be an issue when changing the existing view-mode. If you highlight that image and change the view mode via the media button the view-mode class gets appended to the img tag, but doesn't remove the initial view mode class. Which on saving displays the image size correctly, but shows up incorrectly in the wysiwyg.

This is currently working in the 7.x-2.x (dev)

bradezone’s picture

Glad to see work is being done on this. Just thought I would chime in and say that I tested the latest --dev versions of media and file_entity. I also had to update media_youtube to the dev version. Once everything was in place, I can now insert PDF's but several other things seem messed up, such as youtube no longer inserting the actual video (just a placeholder image), and images not being tied to specific display formats. (I don't even have the "file types" option anymore in config, where I set the file fields and display fields for the specific views like preview/default/teaser etc.)

So sadly it doesn't seem like a workable solution for me right now, when all I REALLY want is just the ability to insert PDF's inline with WYSIWYG (using CKEditor BTW).

heddn’s picture

Status: Needs work » Needs review
FileSize
36.51 KB

Here's a re-roll of #120. Me thinks some things are missing in it. But I wanted to get the re-roll done before I start changing anything.

  1. No uninstall of media_wysiwyg_view_mode module after deleting it in #108. This should happen since there are many sites that are already using the dev version of media.
  2. +++ b/modules/media_wysiwyg/media_wysiwyg.module
    @@ -300,3 +303,217 @@ function media_wysiwyg_filter_fields_with_text_filtering($entity_type, $entity)
    +function media_wysiwyg_form_media_wysiwyg_format_form_alter(&$form, $form_state, $form_id)  {
    

    Why alter the form? Why not add to the original form in pages.inc?

  3. +++ b/modules/media_wysiwyg/media_wysiwyg.module
    @@ -300,3 +303,217 @@ function media_wysiwyg_filter_fields_with_text_filtering($entity_type, $entity)
    +function media_wysiwyg_media_wysiwyg_token_to_markup_alter(&$element, $tag_info, $settings) {
    

    Why alter media_wysiwyg_token_to_markup? Why not change add directly to the original callback?

  4. +++ b/modules/media_wysiwyg/media_wysiwyg.module
    @@ -300,3 +303,217 @@ function media_wysiwyg_filter_fields_with_text_filtering($entity_type, $entity)
    +function media_wysiwyg_media_wysiwyg_wysiwyg_allowed_view_modes_alter(&$view_modes, &$file) {
    

    Why not change media_wysiwyg_get_wysiwyg_allowed_view_modes directly? Why use an alter?

Status: Needs review » Needs work

The last submitted patch, 130: media-custom_view_modes_for_wysiwyg-1792738-130.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
11.08 KB
38.28 KB

Let's see how this fares.

heddn’s picture

Hmm, the interdiff didn't work. All the alter functions from .module where inserted into the right places.

heddn’s picture

The latest patch picks up two things.

  1. The features export didn't let one export restrictions on the default view mode.
  2. Files that aren't images don't have dimensions. Added a check that dimensions are set.

The last submitted patch, 134: interdiff-132-134.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 134: media-custom_view_modes_for_wysiwyg-1792738-134.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
39.57 KB

The placement of the view_mode logic inside media_wysiwyg_token_to_markup() caused a regression in #2062721: Add a white list of file fields that can be overwritten when the file is added in the wysiwyg. I've moved the logic inline into the function.

Also, the reason that tests failed in #134 were because the view_mode sub-module deletion got dropped from that patch. This patch also fixes that.

Status: Needs review » Needs work

The last submitted patch, 137: media-custom_view_modes_for_wysiwyg-1792738-137.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
543 bytes
39.44 KB
heddn’s picture

Issue summary: View changes
heddn’s picture

The view mode wasn't getting re-built correctly for images and content that doesn't have a default WYSYWG view mode defined. This patch restores the mechanism that was used previously.

Status: Needs review » Needs work

The last submitted patch, 141: media-custom_view_modes_for_wysiwyg-1792738-141.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
FileSize
40.04 KB

re-roll of #141.

blasthaus’s picture

Patch in #143 is working for images, but that's all I'm able to insert into ckeditor. Any other file types, like pdf or a simple txt file will only display the file icon with no surrounding markup. Is this issue a part of a larger effort to make it possible to insert any file type with all the markup from a view mode or just img tags?

I've identified my particular problem and it has to do with the fact that ckeditor's own (now deprecated) "media" plugin using the same namespace is getting invoked instead of that of media module. Perhaps the file media_wysiwyg.ckeditor.inc which declares the hook_ckeditor_plugin() is not being found. I'm going to open a new issue on this specific bug since there is an error on that hook, but as this thread has several mentions of a similar issue, it's worth noting.

heddn’s picture

FileSize
40.41 KB
1.78 KB

re: #144
This is part of a larger effort to allow non image stuffs to be inserted into WYSIWYG.

This latest patch fixes an issue where media_wysiwyg_get_file_without_label() was being needlessly called twice.

heddn’s picture

Some stuff from another patch slipped into that last patch #145. Let's try again.

klokie’s picture

The patch in #146 worked for images but didn't preserve the view mode choice from the overlay. The attached patch should fix that.

heddn’s picture

re: #147
I'm curious, why are you using this for images?

klokie’s picture

re: #148, it's my understanding that the purpose of this issue is to allow an editor to embed a media object in a WYSIWYG field. That's what the patch in #146 does, but it doesn't preserve the view mode (since it was being incorrectly passed from Javascript to media_wysiwyg_format_form().

I fixed that issue in the patch in #147, although in my testing it only works for embedding images - PDFs an other non-image media embedded in "Link" view mode are simply replaced by a thumbnail of that media type (without the link). I wasn't able to find out where that was happening - could be in either JS for this module or for Ckeditor.

blasthaus’s picture

@klokie

although in my testing it only works for embedding images

This was precisely my experience. I followed up with these 2 issues but silence so far.
media_wysiwyg.filter.js is not always loaded when using CKEditor
CKEditor plugins registered by hook does not include "media" module

Danny Englander’s picture

I just tried the patch in #147 and could not apply it using git apply. The error was:

error: patch failed: modules/media_wysiwyg_view_mode/media_wysiwyg_view_mode.info:1
error: modules/media_wysiwyg_view_mode/media_wysiwyg_view_mode.info: patch does not apply

So I then used the old method:

patch -p1 < media-custom_view_modes_for_wysiwyg-1792738-147.patch

... and it seemed to apply on several files except for this message:

File modules/media_wysiwyg_view_mode/media_wysiwyg_view_mode.info is not empty after patch, as expected

After that I saw there was a db update:

Media_wysiwyg  7201  Install {media_restrict_wysiwyg} and {media_view_mode_wysiwyg}.   Remove variables media_wysiwyg_view_mode_%.
Uninstall media_wysiwyg_view_mode module.

... ran that in drush and cleared cache. Now there's no longer a page at:

admin/config/media/wysiwyg-view-mode

Am I missing something?

Danny Englander’s picture

I see, it looks like the setting is per file type now such as admin/structure/file-types/manage/MYTYPE/edit so that's a good thing. That being said, I expected my custom view modes that I created in a module to show up in the select list but they do not.

heddn’s picture

re: #147 & #150

I'm not seeing that the view mode is getting lost. The way this patch works is it adds the option to specify a default view mode for use in WYSIWYG @ admin/structure/file-types/manage/%file_typ/edit. This should be the default view mode used when inserting into WYSIWYG. If you are loosing the selected view mode, that is probably a different issue.

re: #151:
Are you applying the patch against the latest 2.x dev branch?

heddn’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
6.64 KB
2.79 KB

I've updated the issue summary so hopefully the use case for this issue is easier to understand. I don't think that the patch in #147 is necessary. I'd recommend using #146.

heddn’s picture

Issue summary: View changes
Danny Englander’s picture

Ok I will try #146. I think I was expecting to see custom view modes that I've created with my own module (using hook_entity_info_alter) show up here but I don't think that's the case. I am guessing I need to do something extra for that to work so it's probably outside the scope of this issue. Thanks.

mansspams’s picture

heddn’s picture

re: #158: The hook_update will get rewritten if/when one of these gets committed during the next re-roll.

mansspams’s picture

Attaching patch with bump to update_7202.

Status: Needs review » Needs work

The last submitted patch, 160: media-custom_view_modes_for_wysiwyg-1792738-159.patch, failed testing.

heddn’s picture

re #160: this assumes that the other issue will get committed first. Which might not happen. It is best to leave the hook_update_N as-is from #146. And if you use the patch on your own site, to remove the logic from the hook_update and include in a custom module's for your site. Until a hook_update gets committed, there's no saying with will get committed first. Or even another patch entirely, maybe even one that has security fixes included. And if you haven't removed the patches hook_update... then the other committed patches logic won't ever run.

To get this thing committed, we need testers and folks to RTBC #146. So let's do that instead of rolling needless patches. And leave it to the committers to renumber hook_updates.

mansspams’s picture

Testing setup as in #157 and everything seems to work well. Only question is about rendering of image + fields in WYSIWYG, image size is right, but fields are not rendered as defined in view mode. After saving tempate gets used and everything is fine. Now Im not sure anymore which patch is responsible for that.

heddn’s picture

Status: Needs work » Needs review
FileSize
32.21 KB

Re-rolling #146 as it no longer applies against HEAD. @mansspams, can you re-test and mark it RTBC?

Status: Needs review » Needs work

The last submitted patch, 164: media-custom_view_modes_for_wysiwyg-1792738-164.patch, failed testing.

heddn’s picture

heddn’s picture

Issue summary: View changes
Status: Needs work » Needs review
mansspams’s picture

I wonder if .install file needs hook_install to trigger update. Something like...

/**
 * Implements hook_install().
 */
function media_wysiwyg_install() {
  media_wysiwyg_update_7201();
}
heddn’s picture

re #168: All the steps in the hook update get run in an install by default, or don't need to be run. What part of it needs running?

  • hook_schema will create the tables.
  • On fresh install, media_wysiwyg_view_mode will never have been installed so it doesn't need to get uninstalled.

parijke queued 22: media-n1792738-22.patch for re-testing.

The last submitted patch, 22: media-n1792738-22.patch, failed testing.

Rob C’s picture

Status: Needs review » Needs work

patching file modules/media_wysiwyg/includes/media_wysiwyg.filter.inc
Hunk #1 FAILED at 168.
Needs a minor reroll.

mstrelan’s picture

Status: Needs work » Needs review
FileSize
39.82 KB

Reroll of #173.

mstrelan’s picture

#173 accidentally removes a + character.

The last submitted patch, 173: media-custom_view_modes_for_wysiwyg-1792738-173.patch, failed testing.

heddn’s picture

@mrfelton, since you ran a re-roll here, I hope that means you are using the code? Are you able to mark this RTBC?

izmeez’s picture

Wow, this is a really complicated issue thread.
I have tried the patch in #174 and found it broke the theme when going back into editing the node.
Possibly there was something else wrong with my configuration at that time.

However, in the process I see this patch removes the submodule media_wysiwyg_view_mode which otherwise seems to be working quite well.

I'm not understanding what the purpose is here and would appreciate any clarification.

Leeteq’s picture

@izmeez; the main aim (ref. the summary at the top) is to add a way to specifically set the display mode for file attachments that are used within the WYSIWYG area ("body fields"), so that we can have one display mode for the body area, and another/more specific ones (only) for each of the files that are linked to within that same body area. Specifically useful for images and file-type icons within the WYSIWYG area. But yes, this is a complex issue, touching several things.

izmeez’s picture

@Leeteq Thanks for helping me come to grips with this. I have read and tried to understand this thread while exploring configuration of media with wysiwyg embedding. And again reviewed the summary.

So, if I understand where this is at, there is general acceptance of what the submodule media_wysiwyg_view_mode is doing and the proposed next step is an attempt to merge it into the media_wysiwyg submodule.

And also add some further enhancements to it.

I shall try the patch in #174 again in our dev environment and report back.

izmeez’s picture

The patch in #174 applies cleanly to the latest dev release and works with existing content.

On the admin file types configuration, admin/structure/file-types/manage/{type}/edit, the Wysiwyg view mode default is none and needs to be set to "Wysiwyg" then everything works when new content is created or edited.

I do not have sufficient confidence with coding to declare the code as reviewed. But, can confirm it is tested.

heddn’s picture

The reason for the removal of the view modes module. It isn't needed. It was created previous to the creation of the media_wysiwyg module.

izmeez’s picture

Yes, I understand what the patch is doing and like it.

I see that the patch in #2349977: DoS image derivatives in Media WYSIWYG has some lines overlapping with the patch in #174 and depending on which is committed first the other will need a re-roll.

izmeez’s picture

Status: Needs review » Needs work

Now that #2349977: DoS image derivatives in Media WYSIWYG is committed this patch will need a few lines changed.

azinck’s picture

Status: Needs work » Needs review
FileSize
38.21 KB

Here's a shot at a re-roll.

azinck’s picture

Oops, failed to rename our update hook. Let's try again.

The last submitted patch, 184: media-custom_view_modes_for_wysiwyg-1792738-184.patch, failed testing.

izmeez’s picture

The patch in #185 applies against the new media-7.x-2.x-dev from 2015-02-11 and works as expected.
@azinck Can you please provide an interdiff_174-185

For those who have already been using patch(es) in this thread and have existing data the upgrade will require them to manually set the permissions, Use Media Wysiwyg in editor.

Hopefully, this patch is ready for commit.

azinck’s picture

Interdiffs aren't really possible on rerolls, but here's a rough approximation of one that I've cobbled together to highlight the changes I made:

diff -u b/modules/media_wysiwyg/media_wysiwyg.install b/modules/media_wysiwyg/media_wysiwyg.install
--- b/modules/media_wysiwyg/media_wysiwyg.install
+++ b/modules/media_wysiwyg/media_wysiwyg.install
@@ -97,7 +97,7 @@
  *
  * Uninstall media_wysiwyg_view_mode module.
  */
-function media_wysiwyg_update_7201() {
+function media_wysiwyg_update_7202() {
   $schema = media_wysiwyg_schema();
 
   if (!db_table_exists('media_restrict_wysiwyg')) {
diff -u b/modules/media_wysiwyg/media_wysiwyg.test b/modules/media_wysiwyg/media_wysiwyg.test
--- b/modules/media_wysiwyg/media_wysiwyg.test
+++ b/modules/media_wysiwyg/media_wysiwyg.test
@@ -107,7 +107,7 @@
   function setUp() {
     parent::setUp();
 
-    $web_user = $this->drupalCreateUser(array('access administration pages', 'administer file types', 'view files'));
+    $web_user = $this->drupalCreateUser(array('access administration pages', 'administer file types', 'view files', 'use media wysiwyg'));
     $this->drupalLogin($web_user);
   }
 }
izmeez’s picture

Thanks, I was wondering the same thing and just finished doing a diff on the two patches to see for myself what had been done to help confirm the changes myself.

Now, we are waiting for someone with enough coding experience to mark this as both reviewed and tested. I can add to the tested part. Although I haven't yet had time to test the additional features support.

izmeez’s picture

Status: Needs review » Reviewed & tested by the community

I am going to mark this as RTBC.
This patch is solid in our testing. It applies cleanly. It consolidates the view mode into the media_wysiwyg module which came along later but is where it belongs.
Committing this patch will help move things forward, imho.
If there are any arising issues they can be handled in a new issue. This issue is already very long and has a very useful patch as an outcome.

chiebert’s picture

I can confirm that it applies cleanly to 7.x-2.0-alpha4+37-dev (2015-Apr-24). I've only tested with image entities, but restriction of view modes in the modal are working, and the default for viewing an embedded image is also taking effect.

Dave Reid’s picture

I need a bit more time to review and understand the changes going on here (especially the features code), so I'm moving this to a stable release blocker.

partdigital’s picture

Just applied this patch to 7.x-2.0-beta1 and when I ran updb I go this error:

Error: Cannot redeclare media_wysiwyg_update_7202() (previously declared in /sites/all/modules/contrib/media/modules/media_wysiwyg/media_wysiwyg.install:98)

We need to change the update the hook declaration to media_wysiwyg_update_7203

partdigital’s picture

Status: Reviewed & tested by the community » Needs work
kevinquillen’s picture

Maybe I am missing something, but I still can't seem to upload an image, insert it more than once into the WYSIWYG using different view modes. The tokens all get overridden with whatever the last view mode chosen in the modal was.

PI_Ron’s picture

The patch from #185 fails against the latest 7.x-2.x-dev

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 185: media-custom_view_modes_for_wysiwyg-1792738-185.patch, failed testing.

circuscowboy’s picture

Was successfully using this patch version #143

Media is a fragile ecosystem. I updated my modules to the latest dev and also included media_ckeditor. I have manually applied the patch in 185 and there is a couple of issues.

I have checked off to disable the default, teaser and preview view modes.

They still show up on the list but when they are selected and submitted a js error shows up not allowing them to be submitted.

media_wysiwyg.filter.js?nxzcdc:121
Uncaught TypeError: Cannot read property 'length' of undefined

Others are allowed to be submitted as usual so it mostly works just doesn't hide them any more.

I have attached my patch.

afsch’s picture

For the view modes issue I got this simple patch that assign the $options variable value to a missed variable.

$form['#formats'] = $formats;
$form['options']['format']['#options'] = $options;
izmeez’s picture

@alexito it looks like you are suggesting an additional change to one of the patches in the thread. Would you mind adding it to the patch and posting the new patch with an interdiff? Thanks.

Rob C’s picture

@izmeez
An interdiff for a one line patch kinda isn't worth it. Look at the patch. Thanks.

Tom.Camp’s picture

Updating path on patch 201 to be run from within the Media module directory.

firewaller’s picture

+1

Any status on this?

joseph.olstad’s picture

Status: Needs review » Needs work

The last submitted patch, 205: media-custom_view_modes_for_wysiwyg-1792738-205.patch, failed testing.

joseph.olstad’s picture

testAllowedFormatFormViewModes

fail: [Other] Line 158 of sites/all/modules/media/modules/media_wysiwyg/media_wysiwyg.test:
Option teaser for field edit-format does not exist.

joseph.olstad’s picture

  • joseph.olstad committed 31e20ae on 7.x-2.x authored by heddn
    Issue #1792738 by heddn, fengtan, aaron, joseph.olstad, Devin Carlson,...
joseph.olstad’s picture

committed patch #208 to 7.x-2.x dev branch

This will either go into beta8 or perhaps skip beta8 and go straight to 7.x-2.0

Let this bake in 7.x-2.x dev branch for a few days/weeks.

joseph.olstad’s picture

Status: Needs review » Fixed
steinmb’s picture

@joseph You have done a great job, moving a lot is stale issues and patches into dev. Thank you! Tough, I would be surprised to to see this not also break stuff. I suggest we roll another beta. It would also allow sites that do not update modules that often to catch up and report back.

joseph.olstad’s picture

This is in beta8. I also had to include change suggested by @alexito in comment #200 , without that the simpletest would not pass but with it , it does pass.

steinmb’s picture

You move fast :) Looking at the list of blockers https://www.drupal.org/project/issues/search?issue_tags=7.x-2.0%20releas... Is any issues missing from that list or is it still correct?

joseph.olstad’s picture

@steinmb , I had a look at that list of blockers, I'm thinking it should be trimmed severely, those issues that are not getting any traction stalled out with no workable patch should be postponed and taken off the 2.0 target , backlogged to 3.x or something.

we need the key features that are important stable /stabilized. Otherwise we'll maybe never get past beta.

izmeez’s picture

Would it help to create an issue tag, Plan for media-7.x-2.1 release, and use that to transfer and reduce the list tagged as blockers for 2.0 release?

joseph.olstad’s picture

Yes that (change blockers from 2.0 to 2.1) could help but I think 3.x would be more realistic because we might want a 2.x that just focuses on current functionality bug fixes. Only well tested 2.0 release blocker patches should make it now, the rest backlog to 3.x

dddbbb’s picture

This conversation has gone a bit off-topic for all subscribed. New issue perhaps?

izmeez’s picture

Yes, indeed. I have opened a new issue #2829038: [meta] Last steps to 2.0 release for that and in the process discovered that the link in comment number #214 is not filtered for just the media project. There are only 4 left that are active or needs work. Kudos to joseph.olstad and everyone else.

joseph.olstad’s picture

FYI: the changes made above released in beta8 caused regressions which I've attenuated in beta9,

see these issues:
#2828989: Can't edit nodes with images media_view_mode_wysiwyg because missing db tables

#2828949: media_wysiwyg_view_mode ?

This >may< require additional regression fixes. Keeping a heads up on this.

fuzzy76’s picture

joseph.olstad’s picture

Status: Fixed » Needs work

There was a security vulnerability regression due to this patch which will be fixed in a few moments.

Please upgrade to 7.x-2.0-beta12 immediately if you used this patch or if you have 7.x-2.0-beta8 or 7.x-2.0-beta9 or beta10 or beta11.

  • joseph.olstad committed 197a7ec on 7.x-2.x authored by heddn
    Issue #1792738 by heddn, fengtan, aaron, joseph.olstad, Devin Carlson,...
mikeskull’s picture

Would be nice if this was in the release notes, took me a while to find out what the security issue was. :)

joseph.olstad’s picture

Status: Needs work » Fixed

sorry my internet provider decided to change my IP address just as I was writing this.

Status: Fixed » Closed (fixed)

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

rooby’s picture

fox mulder’s picture

Is there a chance to implement something similar in D8? Or is this implemented yet?

PS: the problem is entity_embed related in D8. This is a possible solution: https://www.drupal.org/node/2882866