[drupal-devel] [feature] add inline functionaliy to uploads
Bèr Kessels
drupal-devel at drupal.org
Mon Jul 25 12:42:31 UTC 2005
Issue status update for
http://drupal.org/node/26288
Post a follow up:
http://drupal.org/project/comments/add/26288
Project: Drupal
Version: cvs
Component: upload.module
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: Bèr Kessels
Updated by: Bèr Kessels
Status: patch
I will fix the broken Xhttml.
I chose "image" because it is exactly the same as the .image class used
by image.module. But I will see to it, to add more functionality.
I agree that it is not up to the themer for some good defaults, thus I
will add a line of CSS to drupal.css (*shiver* ;) )
upload, IMO does not communicate well enough what the Image is doing.
Not sure yet how I ill fix this, but I will see to it to come up with a
better CSS/theme solution. Thanks!
Bèr Kessels
Previous comments:
------------------------------------------------------------------------
Sun, 03 Jul 2005 18:03:35 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/upload_inline.patch (19.05 KB)
One of the most often asked features is proper inlnie handling of files.
Look at the amount of solutions, the popularity of image_assist, and the
amount of peolpe dowloading image.module! That alone should be enough
proof that Drupal lacks proper inline image support.
This patch adds that to core. In fact, it does little more then
appending a link of img tag to the body or the teaser. Off course that
is configurable per file. Next to the [] list checkbox, this patch adds
an [] inline checkbox.
Simplicity is the foundation of this patch. I want no stles for inline
editing, no fancy html wrappers, no tokens, just $node->body or teaser
appended with a small html string.
Another small themable funtion is introduced, (hey, you cannot expect
me to develop something without adding more power for themers, now, can
you? ;) ), that allows people to theme the string that is appended to
the body or the teaser.
Oh, and also note hat the biggest part of this patch is some cleaning I
had to do in order to be able to develop properly. I dont like Ifs
inside cases in foreaches inside swiches. in other words: nodeapi now
calls functions instead of executing code directly.
------------------------------------------------------------------------
Sun, 03 Jul 2005 18:19:25 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/inline.patch.screenshot.png (26.68 KB)
here is how the form now looks
------------------------------------------------------------------------
Sun, 03 Jul 2005 18:19:58 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/inline.patch.screenshot3.png (30.53 KB)
and this is an example of inlined images and a .doc file.
------------------------------------------------------------------------
Sun, 03 Jul 2005 20:44:00 +0000 : sepeck
changing to patch per request from berkes
------------------------------------------------------------------------
Tue, 05 Jul 2005 07:46:13 +0000 : Kobus
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
------------------------------------------------------------------------
Tue, 05 Jul 2005 08:57:50 +0000 : Bèr Kessels
there are a couple of reasons why i did not include the [inline] tags.
* I aimed for extreme simplicity: a checkbox shows an image inline: its
up to the theme where it appears (if one does not like it before/above
the body and teaser.). Simplicity was the main goal.
* We don't have any tokens in core. And we should not have them.
* Tokens are a very bad substitute for a good interface. They give less
power then plain HTLM. Are much worst documented then HTML, but in the
mean time, they are still as hard to learn as HTML. (Yes, I know people
_think_ they are easier, but there _is_ not difference between [ ] and ,
only that its a different ascii char.
So, no. I don't allow any placement of the image. I leave that that for
dedicated modules, or the themer to decide.
------------------------------------------------------------------------
Tue, 05 Jul 2005 09:34:06 +0000 : 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
------------------------------------------------------------------------
Tue, 05 Jul 2005 11:26:15 +0000 : Bèr Kessels
Kobus,
We are dealing with different issues here: You want a method to place
images, files or so anywhere in the post. That is fine, but certainly
NOT addressed in the patch!
I made patch that *only* adds marked files to the body. its really
nothing more then
<?php
$node->body = $filestring . $node->body
?>
No APIS, no, dynamic tokens, no filters, nothing.
However, what I meant with themers, is that there now is a
theme_upload_inline available, so you can theme the abovementioned
$filestring. On top of that $node->files[FILEID]->inline is TRUE if a
file is flagged for inline.
So in node.tpl.php, or wherever you want to theme a node, you can print
nice images inline, when that flag is set.
And about the comment that a themer cant do this:
Simply not true. On most sites images are always placed in the same
places. REally, even the sites see which use img_assist or inline, use
them to place the images on the exact same places in every node. People
/think/ they want the power to place images anywhere, but they hardly
ever use that power. Just look at all the big news/publishing sites out
there (BBC, CNN, BoigBoing, r even freshmeat) images are all placed acc.
to the theme. They are not placed in random places by authors. So if you
are one of the few that still want that power, there aer mots of power
tools like inline module or img_assist. We should offer a good default,
one that is simple.
------------------------------------------------------------------------
Tue, 05 Jul 2005 11:41:34 +0000 : Gerhard Killesreiter
I fully agree that tags are evil.
Kobus: You can always look for tags in $node->body in your theme's node
function.
------------------------------------------------------------------------
Tue, 05 Jul 2005 11:50:52 +0000 : stefan nagtegaal
Ber,
the theme-function in your code looks like:
<?php
+/**
+ * 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 ''. check_plain($file->filename) .' [1]';
+ }
+
+}
?>
After looking at your code it's not clear to me when $image is TRUE or
FALSE, can you elaborate on me please?
[1] http://drupal.org/'. check_url(($file->fid ?
file_create_url($file->filepath) :
url(file_create_filename($file->filename, file_create_path())))) .'\"
class=\"upload inline
------------------------------------------------------------------------
Tue, 05 Jul 2005 12:06:39 +0000 : Bèr Kessels
The function calling the theme function should decide whether its an
image. IMO that is far too hardcore code for a themer ;)
<?php
$image = ereg('^(image/)', $file->filemime);
?>
inside _upload_inline() does the trick.
I did find one issue, though, with svgs, which are image/svg so maybe
we should limit this to really only inline jpg, png, and gif, by
extension? But I am no fan of determining files by extension, and IMO
getimgsize is too heavy;
------------------------------------------------------------------------
Tue, 05 Jul 2005 12:13:52 +0000 : stefan nagtegaal
Can't you make use of PHP's
<?php
image_type_to_mime_type();
?>
or
<?php
image_type_to_extension();
?>
------------------------------------------------------------------------
Tue, 05 Jul 2005 12:45:43 +0000 : Kobus
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
------------------------------------------------------------------------
Mon, 18 Jul 2005 09:56:38 +0000 : Bèr Kessels
okay, so for all clarity, let me sumarise:
* this patch introduces a chackbox "inline". If checked, the file will
show up inline. IT will be appended to the teaser and the body.
* this patch does *not* allow one to place images of files in a
userdefined place in the body.
* this patch does *not* do any resizing nor any thumbnailing.
* This patch is meant to be simple, clear and transparant. No tokens,
no javascript, no nothing.
------------------------------------------------------------------------
Mon, 18 Jul 2005 11:21:54 +0000 : 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
------------------------------------------------------------------------
Mon, 18 Jul 2005 19:58:34 +0000 : Bèr Kessels
Kobus,
And all the others looking fofr advanced inline systems:
please do not -1 this. It will hep inline module a lot too. allthough
not yet in this patch, but can you imagine a perission system for
inline module, that comes for free? Or a core system, that makes inline
module ten times smaller? it is this path!
so, please, if it is not /exactly/ what you are looking for, at least
look at the advantages for Joe Average, and even for the possibilties
for your favorite inline-module. even imag_assist will gain an anomous
usability impcact when usiong this module, for it can then finally use
the upload module in full scale.
Ber
------------------------------------------------------------------------
Sat, 23 Jul 2005 18:59:10 +0000 : Bèr Kessels
Sohodjo jim:
" I hope it matters. A big -1 on enforcing over-simplification at the
expense of important and existing functionality. If automatic,
uncontrollable placement of images becomes the standard, it will mean
fewer not more image-rich Drupal sites as site owners/developers get
complaints about "Why can't I control my images!"
I _strongly_ encourage Ber to provide a "have it both ways"
solution. Why not have an admin configuration setting for "allow inline
tags" with default off. Change it to on and you get an additional
upload
checkbox for 'inline at tag' to accommodate the current functionality
of
the inline module while simultaneously allowing for default placement.
"
NOOO. people; please; look at the PATCH.
Again: and hopefully last time: Simplicity.
This patch does NOT replace ANYTHING inline module or img_assist wants
to do.
This patch hands better data to such modules. It hands a variable over
to these modules, $node->file[FID]->inline, which tells these sorts of
modules if people wnat that file to appear inline.
AND it CAN (by default will) render a file in the most simple way
inline.
So really, if you want to -1 this patch, fine. But do not -1 it,
because it does too little IYO. It still allows MUCH more than what you
can do no, without that patch! And any solution, for core, that allows
advanced handling of inline images will either require an enourmous
amount of work, or it will simply no get in.
So, unless you come up with a good core-worthy patch, this is still a
big leap forward from what we have now.
------------------------------------------------------------------------
Sat, 23 Jul 2005 19:01:51 +0000 : Bèr Kessels
:$ this is an attempt to fix the borken HML in the previous follow up.
------------------------------------------------------------------------
Sun, 24 Jul 2005 04:07:34 +0000 : TDobes
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.
------------------------------------------------------------------------
Sun, 24 Jul 2005 07:58:19 +0000 : Bèr Kessels
Thank you for your thoughts Tdobbes.
As for the "moving things in and out of nodeapi" This was necessary,
because otherwise the patch would have made the code even more
unreadable. I agree with you on this point, but I simply had to clean
it up a little, in order to be able to work in it.
As it stands now, it is simply not possible to make this a separate
module. We cannot hook into the files table. In a next stage, I might
work out some hook for that table. But first things first.
Can Dries or Steven please comment on his. I want to know If i should
bother spending this enormous amount of time on commenting and debating
this issue. If the chances are small this is accepted, Ill just drop it
This ridiculous long thread reminds me again why I try to avoid making
patches for core.
------------------------------------------------------------------------
Mon, 25 Jul 2005 05:49:37 +0000 : Steven
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.
More information about the drupal-devel
mailing list