add inline functionaliy to uploads

changing to patch per request from berkes

This gets a +1 from me in principle, however, the [inline:xx] tag with inline.module gives you greater freedom as to where the inline image must be displayed. If you can add this functionality (I haven't checked the code, I don't know if it is in there) it would be a great addition for Core. This same strategy can be used for inline blocks, I am sure.

Regards,

Kobus

So you will provide an API to do this? For example, with perhaps minor modifications, the inline.module will be able to display these files? In that case I am happy. If not, then I can't give my support to this patch (as if that matters...).

A themer can't do this task as inline images (and blocks for that matter) is too dynamic for theming; it can be placed anywhere in a node where the user pleases. This means for me that there should be a module for this, such as inline module that allows you to define [inline:xx] tags. If your module emits an array of uploaded files (such as upload.module), inline.module can be, with minor changes, adapted to show these files inline.

To show you what I mean with the content is too dynamic for a themer to perform this task, look at this screenshot related to inline blocks (inline images can follow the same pattern): http://drupal.org/files/issues/regions---possibility-3.png

I fully agree that tags are evil.

Kobus: You can always look for tags in $node->body in your theme's node function.

Ber,

the theme-function in your code looks like:

+/**
+ * Theme function for rendering of inline images
+ * @param $file a file object.
+ * @param $image a Boolean, indicating whether an img tag (TRUE) or an anchor tag (FALSE) should be used. 
+ * @ingroup themable
+ */
+function theme_upload_inline($file, $image) {
+  if ($image) {
+    return '<img src="'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'" alt="'.  check_plain($file->filename) .'" class="upload inline">';
+  }
+  else {
+    return '<a href="'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'" class="upload inline">'. check_plain($file->filename) .'</a>';
+  }
+
+}

After looking at your code it's not clear to me when $image is TRUE or FALSE, can you elaborate on me please?

Can't you make use of PHP's

image_type_to_mime_type();

or

image_type_to_extension();

Well, if not in the patch, I need inline functionality one way or another. inline.module works perfectly with the current upload.module to fulfill my needs, and if there is a way where someone can extend your proposed upload.module, that would get my +1, otherwise I will just remain neutral about this issue.

inline.module makes use of $node->files and displays that image. If your patch still uses $node->files, then this is a non-issue. In other words, if I can, with inline.module, or with minor modifications to inline.module still show files inline, I am +1 for this.

As for the themability question... I still disagree. You say "Simply not true. On most sites images are always placed in the same places." That is ONLY true because they have NO alternatives. Means the content is in the same places, because they have no other places to put them. I have made tens of static sites where the content does not resemble a fixed pattern or structure. For me, free-flow layout is my expression of my creativity. This is why I still even BOTHER with static sites; when Drupal don't do what I want it to do. If Drupal could do inline display of content properly, I'd never even build another static site.

It is a great mistake people make (including myself), and that is they design the site before ANY content is available, and this happens a lot in static sites. But in dynamic sites, you have far less control over this, unless you have a VERY clean structure with limited information that can be added, and only a few people posting content. If you can have your content before you start the design, you will have a better fit.

I can't see how you can anticipate every possible posting posted on your site if you have a diversity of users. Especially not if you're using a site such as an designer's showcase site where your creativity is shown by your web sites. Possible, but not easy. If I claim that I have "unique, fresh designs" I can certainly NOT give them a "box-like" site with "left and right side bars only". That's why I need inline content, this includes images AND boxes.

Gerhard: Your concern about tags is answered as well above, if I am not mistaken. The patch need not provide the tags, but should provide for someone to develop a module that provide the filters or tags.

To summarize: I don't need your patch to do the inline images. I just need your patch to be able to allow someone else to develop a module that can display inline files.

Kobus

Then I can't see how this patch could replace the existing upload.module, which allows you to use inline.module to place images inline. I therefore withdraw my +1 and go -1 on this (not that it matters, I believe).

I think you didn't properly read my previous message. I said I don't need your patch to do all this, but I can't live without the functionality currently provided by the upload.module and inline.module combination. I agree that your patch don't need to have that functionality, but if your patch takes away this functionality, in other words, I can't (by hook or by crook) manage what I need to do, my vote is -1.

Regards,

Kobus

Please don't combine unrelated changes together in the same patch. Moving stuff in/out of the _nodeapi hook has absolutely nothing to do with the patch and makes it much harder to read through the patch and see what it's actually doing. I'd appreciate it if you could separate the changes made here which are really relevant and move the other changes into another issue.

Also: You change form_group_collapsible(t('File attachments') to form_group_collapsible(t('File Attachments'). A quick glance at some other core code seems to indicate that we've standardized on capitalizing only the first word in multi-word group titles.

As for the functionality itself, I don't think this belongs in upload.module. It would be useful to have the functionality in core, but as a separate module which can be switched on and off. (upload_inline.module or something like that) If a site were set up using an inlining system from contrib and the functionality you're proposing could not be switched off, it would be extremely confusing to users that there are two different ways of inlining images.

A minor comment: I'm not so sure whether the results of this patch, when inlining non-image attachments, will be the one users expect. A non-web-savvy person may expect his/her Word document (for instance) to appear inline with the node when they click "inline" -- just like their images do. I'd suggest that we limit inlining to images only. I'm not as adamant about this as the other issues, so I could be convinced otherwise if others disagree.

Overall I like the concept, but I think it at least needs some default styling to make the image appear okay, like floating it. Right now it appears inline between whatever was there... very ugly.

Sure you can say "leave it up to the themer" but if the goal is to provide a simple default, we can't expect people to dive into theming if they are going to use this feature instead of img_assist or whatever.

Also, I wonder about the classes "upload" and "image". Is there a reason to use two classes? Isn't "image" a bit too generic? Oh and there is a missing XHTML / for the img tag.

What is proposed here is a good idea.

But I think that this is done in not good way.

I think that upload.module should be only a layer
between file system and modules.
So modify upload module to add a specific feature
like your inline for me is not good.

but I like the idea to add checkboxes for enable
interaction modules between modules and upload.module.

Maybe we could design an of api wich let modules
tell upload.module that they can interact with it.

I make an example:
I'm a developer of video.module.
Now to add a video file to video.module one need
to put the file into drupal folders using ftp and then insert
the path to the file during node creation.

One of the features our users are asking a lot is
the possibility to directly upload a video file to dupal.

So I should write a lot of code, settings, security buggy
code.. that for me isn't useful.

What about if only calling an upload.module api I
should enable a new checkbox on upload form
called "use as video file" which let me use the file
as a video file on my module???

What do you think about it????

After discussing with Ber, I think this patch has some great potential though the simplicity of it..

For the people who don't completely agree what this patch does:
- It adds an extra checkbox for each attachment. If the 'Inline' checkbox is checked, images are displayed inline in your post and other attachments (like .pdf, .ppt, or whatever) were transformed into links.

The potential of this patch is in the fact that this makes it much, much easier to have another contrib module which makes it possible to let you choose at the node submission how you would like your node to be displayed, incl. the selected inline attachments to be displayed (using _nodeapi()).

So, a big +1 from me on this..

> - It adds an extra checkbox for each attachment. If the 'Inline'
> checkbox is checked, images are displayed inline in your post and other
> attachments (like .pdf, .ppt, or whatever) were transformed into links.

Does this mean it is done automatically or do I have control over this? I don't want the images to be inline if I can't control where I put it.

Let me just clarify my position on this short-and-sweet-like:

If it breaks the existing functionality that upload.module + inline.module provides without opening the door for a replacement, then I am -1, otherwise +1. :-)

Kobus

Ber: Great, then I +1 it. From our previous talks about this, it seemed as if this functionality was being taken away.

Agreed, +1. The most on-going and especially most recent discussion has clarified that we can have our simple cake and contrib mod cake as well.

Does this mean that inline module will work as is, or will a post-patch update be required?

--Sohodojo Jim--

I read through the code and it looks good to me, though it is hard to know whats new given all the code thats moved around. this is definately needed functionality.

If you were still up for enhancements, I'd like to see the 'inline' box get checked by default when uploading an image. when the attachment is not an image, the box should be disabled and unchecked.

@Bèr: Why not simplify this patch to only add the hooks to a contrib module would need to add this functionality? You said that your patch would allow inline.module to work much more easily. But then there is a lot of extra stuff there that inline.module won't need. This functionality can't replace inline.module, so anything that it doesn't use just gets in the way.

I'm afraid that users will think this is the best Drupal can provide without looking for a contrib module. I'd prefer we spent our time and effort adding the whole functionality of inline.module or making it easy to add and position inline content in nodes to core rather than adding some functionality to upload.module that doesn't cut it for most users.

So, either strip this patch of everything but the functionality that inline.module (and similar modules) can use or allow me to position images in my text through this patch. Otherwise, -1. If this is the first step in a plan to add the functionality I want to core, I'd like to hear your full plan.

+1 from me for this feature. It's frustrating that core doesn't include some basic image display functionality, and this will cover a large percentage of users needs without introducing complexity. People wanting arbitrary image placement still have inline.module etc., so it's nothing but an improvement on the current situation.

Bèr: one thing is not clear to me... you are right that other modules can access the inline value. But upload.module will always add the image to the body. So, how can other modules use the inline checkbox/option meaningfully?

Ber, check this latest commit: http://drupal.org/node/28483 ... I think your patch no longer applies since this JS based does do inline functionality.

got an extra 'favico' in the patch file there.

I think I'd like to have this feature in core. The patch is quite big, though. ;-/
Also I noticed that the pgsql part is bogus.

Just a quick +1 from a user who is hoping that this gets into core for 4.7. We're using the inline functionality on our Remix Reading test site at the moment, and it's very handy for our theme where the user uploads any number of files and may want one or two images to be previewed in the node.

Patch needs to be rerolled. Latest 2_0 version does indeed fix JS-based uploads commit, however it is missing all of the database changes and updates from the last _1 patch. Also, upload_count_size() is undefined, should be using <a href="http://drupaldocs.org/api/head/function/upload_space_used">upload_space_used()</a> instead. Also, beginning of upload.module patch adds the default maxsize setting which was previously taken out becuase it wasnt working properly. This patch corrects that and is a seperate issue :)

Bèr: I'm afraid you missed my point. It is pretty useless for other modules to be able to read the inline value, when upload.module always adds the image to the node body.

Suppose I want to make a module which adds more advanced inline positioning, perhaps with a marker tag. I cannot avoid _upload_inline($node) being called, so I have to somehow remove that (themed!) image tag again. This makes the "other modules can use it" argument pretty silly IMO.

It would make sense for Ber to provide one or two examples of how this could be used.

Bèr: The idea of clearing $node->inline never occurred to me... but won't that only work some of the time? It depends on the order in which modules' _nodeapi hooks get called. I believe they get called alphebetically (there's an asort in module_list), so a module whose name starts with v, w, x, y, or z will not be able to prevent upload.module from displaying its inline image.

While I do feel this functionality is useful, I think a better way of implementing it would be to allow other modules to add columns to the upload table. Then a module could invoke this upload hook to add a field for "display inline" and invoke _nodeapi to actually place the image. This would provide for future expansion of the upload module in addition to solving the immediate goal.

I think that Ber's suggestion to unset the variable is a great, simple workaround. I don't see any more outstanding objections to the patch. Yes, it can be improved later, but it is useful already.

upload.module is listed twice in the patch ... so patch isn't work, please update!

+1 Ber. Worked well. I agree, the upload preview issue belongs in a diff bug. perhaps i'll create one, i'll try and look into that bug today. but i really like the inline feature, and i like that it IS simple. nice.

Ber,

As stated earlier about the bug, there seems to be a similar issue:

http://drupal.org/node/31102

Just wanted to state, this patch is ready to go. The "bug" is for all intents and purposes unrelated, and shall be resolved when the above mentioned issuse is resolved. I'd like to see this go in.

The problem with your upload file lists dissapearing has been resolved.
http://drupal.org/node/31102

Patch in #60 does work as advertised. The noted bug above is indeed a seperate and indepedent issue and can be recreated without this patch. In otherwords, this patch doesn't introduce any bugs and is ready to go.

I'm going to put this patch on hold. There does not seems to be a much demand for it.

@Dries - I thnk you have underestimated the demand. Of all the image handling solutions, this is the simplest. It feels very much like core functionality to me.

Like I said before, I really like this patch too. Makes things a lot easier, IMO. Has lots of potential! Patch probably no longer applies, huh?

hum does this only inline images? i think if there's going to be inline in core, there has to be configurable inlining for different mime types and pre defined choosable aliases.

I would like to see this in core.. I like the functionality and the way it's implemented, very clean and straightforward..

Ber: Please update you patch so we can test it and set it to 'Ready to be committed'

The feature Bèr attempted to push here gets a big +1 from me. For many users, tags (such as [inline:N]) are too difficult. This feature would be exactly what they need. AFAIK, no better solution has been proposed for those users. The interface Bèr proposed is simple, elegant and consistent with the rest of the attachment interface.

Even where the users are able to use tags, I think this solution would be the most appropriate in many cases.

I naively hope this issue can be revived... :-)

+1 for this patch

In terms of simplicity: Why should there be a workaround for removing automatically inlined images from nodes? Make it simple: Update themes to handle inlined uploads as stated in #51 by Ber! If you already use your theme to decide whether images are displayed in your example, why would you expect others to do that in another way? Instead of tricks, let the theme decide where to put inlined uploads. - Bottom line: This patch

• adds simple inline image support to Drupal core through an additional checkbox, which is the greatest of all demands
• adds the possibility for other modules to handle inline checked uploads in their way

If neither the theme nor a module handles those checked uploads, nothing will be inlined. That's default and simple. I would love to see this patch in core.

Thanks Bèr for your dedication!

Well, I think this issue could be closed, as this functionality has been integrated into the Inline module. Thanks a lot Bèr!

I strongly believe this function belongs in core. It is just the kind of simple and straight forward support for inline images that many users needs.

I use my own hacked version of inline.module on all my own and my customers sites. If this patch gets commit to core it would replace inline.module at least 7 times of 10.

When I first read this post many month ago I was sure this would get committed in no time, I'm really surprised that we are still discussing it.

Pity to have a such an exciting thread just end like this.
:(
Shot for the hard work with this patch Ber!

3 years later and still nothing in core. This was a huge mistake made long ago... Please consider re-opening...

No. A core-worthy way to introduce a general "inline"/embedding functionality that can be leveraged by any contrib module is in development and called Inline API.