Development
Threads by month
- ----- 2026 -----
- June
- May
- April
- March
- February
- January
- ----- 2025 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2024 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2023 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2022 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2021 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2020 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2019 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2018 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2017 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2016 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2015 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2014 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2013 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2012 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2011 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2010 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2009 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2008 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2007 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2006 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2005 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
September 2005
- 78 participants
- 615 discussions
15 Sep '05
Issue status update for
http://drupal.org/node/28595
Post a follow up:
http://drupal.org/project/comments/add/28595
Project: Drupal
Version: cvs
Component: comment.module
Category: feature requests
Priority: normal
Assigned to: mrowe
Reported by: robertDouglass
Updated by: moshe weitzman
Status: patch (code needs review)
minor nits:
we use an extra line in our else clauses. this should be two lines:
} else {
also, you can almost always avoid using db_escape_string() directly.
you will need to do a str_repeat('%d, ', count($unpublish)) in the
SQL(for unpublish query) and then pass a an array as the last argument
to db_query. This is the seldom used 'pass all values in an array'
feature of db_query: http://drupaldocs.org/api/head/function/db_query
moshe weitzman
Previous comments:
------------------------------------------------------------------------
Wed, 10 Aug 2005 08:29:14 +0000 : robertDouglass
When using the comment approval queue the workflow for approving
comments is awful. You have to click administer->comments->approval
queue to get a list of comments awaiting approval. Then you have to
click edit on one of the comments, click the collabsible field-set for
the publishing controls, click to published and save the comment. Then
repeat the process for the next comment.
I'd say that this workflow is a significant deterrent to anyone using
this functionality.
To make it better, there would be checkboxes next to each comment in
the table on the approval queue page, and a set of operators analogous
to the content overview page. Better yet would be if each comment in
the table was an AJAX link to the comment's body and info which would
expand right there in the table itself, since it is usually impossible
just from the comment title to know whether to publish or not. If one
could click the various comments open, right there in the table, then
mass publish or delete, we'd have a real approval queue. Perhaps the
spam module could tie into the actions that can be taken on comments so
a mass "mark as spam" could be done too.
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:32:47 +0000 : mrowe
Attachment: http://drupal.org/files/issues/comment_bulk_approval.patch (2.24 KB)
Here's a patch (against HEAD) that gives you the cheap-and-dirty
approach (checkboxes that allow bulk approval/unapproval of comments).
I'll leave the gold-plated AJAX solution to someone with more time. :)
------------------------------------------------------------------------
Wed, 14 Sep 2005 21:17:21 +0000 : Dries
- Don't use tabs.
- SQL queries might be vularnable to SQL injection attacks. Use
db_query()'s %s and %d directives.
------------------------------------------------------------------------
Thu, 15 Sep 2005 02:24:17 +0000 : mrowe
Attachment: http://drupal.org/files/issues/comment_bulk_approval_0.patch (2.38 KB)
Ok, I think I got rid of all the tabs this time...
I've had to use db_escape_string, rather than the sprintf params to
db_query, but it should have the same effect.
1
0
Issue status update for
http://drupal.org/node/28595
Post a follow up:
http://drupal.org/project/comments/add/28595
Project: Drupal
Version: cvs
Component: comment.module
Category: feature requests
Priority: normal
Assigned to: mrowe
Reported by: robertDouglass
Updated by: mrowe
-Status: patch (code needs work)
+Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/comment_bulk_approval_0.patch (2.38 KB)
Ok, I think I got rid of all the tabs this time...
I've had to use db_escape_string, rather than the sprintf params to
db_query, but it should have the same effect.
mrowe
Previous comments:
------------------------------------------------------------------------
Wed, 10 Aug 2005 08:29:14 +0000 : robertDouglass
When using the comment approval queue the workflow for approving
comments is awful. You have to click administer->comments->approval
queue to get a list of comments awaiting approval. Then you have to
click edit on one of the comments, click the collabsible field-set for
the publishing controls, click to published and save the comment. Then
repeat the process for the next comment.
I'd say that this workflow is a significant deterrent to anyone using
this functionality.
To make it better, there would be checkboxes next to each comment in
the table on the approval queue page, and a set of operators analogous
to the content overview page. Better yet would be if each comment in
the table was an AJAX link to the comment's body and info which would
expand right there in the table itself, since it is usually impossible
just from the comment title to know whether to publish or not. If one
could click the various comments open, right there in the table, then
mass publish or delete, we'd have a real approval queue. Perhaps the
spam module could tie into the actions that can be taken on comments so
a mass "mark as spam" could be done too.
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:32:47 +0000 : mrowe
Attachment: http://drupal.org/files/issues/comment_bulk_approval.patch (2.24 KB)
Here's a patch (against HEAD) that gives you the cheap-and-dirty
approach (checkboxes that allow bulk approval/unapproval of comments).
I'll leave the gold-plated AJAX solution to someone with more time. :)
------------------------------------------------------------------------
Wed, 14 Sep 2005 21:17:21 +0000 : Dries
- Don't use tabs.
- SQL queries might be vularnable to SQL injection attacks. Use
db_query()'s %s and %d directives.
1
0
Issue status update for
http://drupal.org/node/27364
Post a follow up:
http://drupal.org/project/comments/add/27364
Project: Drupal
Version: cvs
Component: filter.module
Category: tasks
Priority: normal
Assigned to: Bèr Kessels
Reported by: Bèr Kessels
Updated by: Souvent22
-Status: patch (code needs review)
+Status: patch (ready to be committed)
+1. I honestly have not played around with the filter formats ever. I
looked at them, adn they were a mess. Did the patch, and everything was
clear as day. Anything that makes drupal easier to use...AND makes it
easier to understand and make sense should def. be included.
Souvent22
Previous comments:
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels
This is a mockup for the Filter UI. Currently it is soo complex, that I
dare to call it broken.
Please comment on this, for I will not ake any patches, when the
general idea is disliked.
The idea is t split filters and input formats better. Filters are
filters, defined in modules. Input formats are bundles of filters. This
is how we have it ATM, but the interface fails to communicate that. I
tried to develop a consistent (with the rest of Drupal) interfce that
makes it clearer what is what.
The menu is as follows:
* administer
** ....
** settings
*** input formats
*** filters
** ...
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB)
* administer
** ....
** settings
*** input formats >> see attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB)
* administer
** ....
** settings
*** input formats >> see attachement 2, tab 2
*** filters
**
Note: The default checkbox lmay seem a bit odd. But checking it will
remove the "default status" from the current "default" one and make
this one "default". The help text, and a drupal_set_message should
explain this.
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB)
* administer
** ....
** settings
*** input formats >> clicking a 'configure' link in the table of
attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB)
* administer
** ....
** settings
*** input formats
*** filters >> see attachement 4
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB)
* administer
** ....
** settings
*** input formats
*** filters >> clicking a 'configure' link in the table of attachement
4
**
------------------------------------------------------------------------
Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels
IMO this makes the interface easier. But other disagree. And because it
also removes one feature: (being able to use E.g. HTML in different
input formats, with different settings)
I'll simply wont fix this :(
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB)
I decided to go for it anyway. Be it in a slightly different way then my
initial mockups. The difference is that I left the filters where they
are, hidden beneath the formats.
So, here is the patch that makes the filter UI more consistent with
screens like blocs, menus, vocabularies et al
also nice to note is that:
93 lines are added, 104 are removed. So netto we have less code :)
(cvs diff filter.module | grep ^+ | wc -l)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB)
Here is how the listing now looks.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB)
And the "configure" screen.
The 'add' screen looks exactly the same, only with the default values
pre-filled.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB)
Oh, And here is a really nice example of why the current screen is no
good.
And here I "only" have four roles. I run a site with 12 roles, imagine
this screen then!
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB)
And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2
codebase. Sorry)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal
I like this very, very much!
This is - indeed - much better than the current UI. This always fits,
and makes the life of people who use a fixed width theme much nicer..
Ber, FYI (you probably know this too), 'admin/access' does also break
fixed width themes due to the fact that it expands the width of the
table when more roles are being added..
Good catch! I like it much more this way..
+1 from me...
------------------------------------------------------------------------
Fri, 29 Jul 2005 07:27:34 +0000 : Dries
The code style needs work (spaces, tabs, 'as' -> 'AS'), the code
comments need work, and debug statements should be removed.
Screenshots look good to me. db_query('UPDATE {filter_formats} SET
cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d',
(int)$cache, $name, $roles, $format); can be written better/shorter
quote-wise.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB)
Dries, thanks a lot for the review. OI fixed your comments. Or so I
hope. code-style.pl was of no use (can that *please* be removed from
core?) because it chokes on the help texts. It also pointed out a lot
of old formatting isues, whic, when fixed wuold flood this patch with
unrelated fixes. I tried to narow my style-searching to the areas I
touched. And I made the comments more verbous.
The query is fixed. it saves no characters, but looks better now.
Thanks for the tip.
The print_r..... blush. thats what I get for not running my default
grep -r print_r on my code after finishing up...
I could not find a lot of spaces and tabs. So it lmight be that i
missed one. I triple checked it though.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:32:21 +0000 : Steven
Moving the role settings inward does fix the wide-table problem, but it
makes the filter configuration much more opaque. We could perhaps fix
this by showing the roles as a comma-separate list in the input formats
table. Such a list can wrap and will not stretch the table.
Other than that:
* Having the instructions for the Roles and Filters form_group() at the
bottom is a bit weird. The distance is too big.
* The table that was used for toggling filters on/off was much more
compact before, and IMO a lot clearer. Why change to a flat list with
the description staggered below each item?
------------------------------------------------------------------------
Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels
consistancy, consistancy and more consistancy.
There really is nothing worse that having a different 'concept' of UI
for every thing in Drupal. We are going in the right direction, with
the blocks being editable objects, menus acting like that, taxonomy,
users, etc..
I wanted to make this act similar. I am not at all happy with the fact
tah the list is still a form. But we need this, for setting the
default.
A big -1 for a comma-separate list. That is extremely hackish, even
worse useabilitywise and just silly
* we should avoid typing when not needed.
* we should avoid errors on input, when they can be avoided (a typo
is made very easy, I know all about it) Form set errors won't fix that
usability -wise.
I agree that you sort-of loose the overview. But IMO that is a minor
loss comared to what we gain, usability-wise.
The wtoo-wide was onlywhat triggered me. But do not think that I am
noly making this patch to fix that issue. This patch is there to
improve usability, through consistancy. o make drupal behave the same
in most places in admin. If you know one screen, you know most of them.
We really should move away from custom-hacks for every page, because
that is useability rule #1: consistancy. Stadardisation, even if
sometimes a custom hack might make things easier in that single case,
it will reduce your overall usability so much that nearly ever it is
worth that hack.
That select-boxes instead of the table issue: consistancy. As well that
it completely broke fluid CSS layouts, my solution is more consistant,
and uses forms the way they are meant. If we cannot use $descritption
in checkboxes they should be removed. but IMO it feels much more
consistant. Tables are for tabular data, and this is simply a list of
items, with additional data.
And i do not really get what you mean with "too far down". Do you mean
that you would like to see the order of the groups different?
------------------------------------------------------------------------
Fri, 29 Jul 2005 13:31:24 +0000 : Dries
I haven't tested Ber's patch yet but looking at the screenshots, it like
what I see. I remember being confused by the current filter UI, and
often, I still am.
------------------------------------------------------------------------
Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman
I dislike the recent movement away from overview pages. I think we can
still provide and overview while allowing editing to happen in a
dedicated form. An example in the wrong direction is 'content types'
admin. It is impossible to get an overview, and the listing page is
worthless. I agree with Steven on this one.
It is true that consistency is desirable. But consistent crap is not.
------------------------------------------------------------------------
Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org
I agree with Moshe. Getting a quick overview about what is set how is
essential for an admin.
------------------------------------------------------------------------
Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques
Overall I like the screenshots. Did not test the patch.
Bèr, what I think Steven (#15) meant by "showing the roles as a
comma-separate list in the input formats table" is that as an overview
page it would be better if you added a list (not an input box!) to
screenshot "input-formats_list1.png" (comment #1). Add a column that
reads "Roles" (or "Enabled for") and then for each row a comma
seperated list of the roles that can use this input format (or maybe
even better a HTML list). You would still need to click "configure" to
_change_ the roles.
The "Default" text (not a radio button) you already show in the
"Options" column is something similar: it immediately shows that this
input format is the default without you having to go to the "configure"
page. Why not do the same for "roles"?
If this is what Steven meant, then I tend to agree with him.
And to help moshe and killes: maybe you could even add a column that
_lists_ the used input filters. Again, only a list, not a way to change
it. That's what the "operations" column is for.
A table like that will "fluid" better than what we have now, still it
is a _complete_ overview of the settings.
I could have misunderstand someone... I have the tendency to do that...
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB)
by popular demand: a comma separated list.
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB)
and a screenshot
------------------------------------------------------------------------
Mon, 22 Aug 2005 18:39:44 +0000 : Dries
Please check your use of print theme('page', $output). Normally, you
just return $output.
------------------------------------------------------------------------
Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels
All instances of theme('page',...) in filter.module are now changed to
return ... ;
furthermore, the patch is the same.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann
Bèr, I think you forgot to attach your updated patch.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB)
my email client has this nice warning system for attachements. Maybe
drupal should search for the word attachement too, and when no att.
found give an error ;). Or maybe i should just learn to pay attention.
Guess that is easier.
Anyway, here it is.
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:22:54 +0000 : Dries
I wanted to apply this patch but:
1. I can't change the default input format. 'Save'-button doesn't
work.
2. I got confused by the fact I can't change the roles of the default
filter. I think the form group description should explain this.
3. form_group(t(filter)) should be form_group(t('filter'), ...).
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck
Found a few more issues:
If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab
at the top and then click 'list filters' link in the text, I get this
warning with PHP5 warning: Missing argument 1 for filter_admin_format()
in \drupal\modules\filter.module on line 378.
Also, it is sort of confusing as to what the 'List' tab implies. For
example, if I click on 'input formats' in the admin menu, I get a list
of the current filters. When I click 'configure' for one of those, I
see the options for that filter. However, it says 'list' in one of the
tab, which doesn't really mean much. I thought that was the list of
filters, not the options for that filter. That should be cleaned up.
Also, there is no way to get back to the full list of filters unless
you click on 'input formats' in the admin menu. A setting/link to fix
this would be great.
Also, where is the link to add filters???
I think the options/layout/tabs should mimic of that of admin > users.
So on admin > input formats, it has the tabs 'list', 'add format' ...
very clear and consistent.
On a configure page for a filter, it should have 'list', 'view',
'configure', 'rearrange' ... this would make it easier to navigate and
get back to the original list of filters.
Make sense? I think this and Dries' comments put this patch over the
top :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 01:48:55 +0000 : tag
In my initial battle to figure out this module's (quite nice)
functionality, what tripped me up the most was actually the
nomenclature.
The way I keep it straight in my head now is to translate "input
format" to "filter set". If you view those (the current) screens with
this in mind, I think it's a lot easier to understand what's what. It
sort of describes this in the help text, but the terms "filter" and
"input format" don't really convey the relationship between the two --
there's no way to intuit that an "input format" is a collection of
"filters". Not to my mind at least... and well, we know nobody reads
instructions...
Are there any technicalities that make renaming "input format" to
"filter set" not accurate? Or does someone have a better term/terms to
better show the relationship of these elements? (I'm aware of filters
in servlet architectures but I don't think there's much of a collision
with that usage).
Anyhow, I guess this doesn't quite address the UI controls and/or
layout, but would still help a lot.
------------------------------------------------------------------------
Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels
I prefer filter set. In fact: I like it a lot. Anyone else ideas on
this?
------------------------------------------------------------------------
Fri, 09 Sep 2005 10:18:18 +0000 : Dries
I too had problems with the terminology; it took me months to get used
to 'input formats' versus 'filters'. Using 'filter sets' might improve
the situation -- especially from an administrator's point of view when
you have to wrap your head around the UI/difference. I'm not native
English, but 'filter set' sounds more explanatory, yet slightly more
geeky so I'm not sure if it flies for users who don't need to
understand the underlying concepts (eg. under the node and comment
submission forms).
------------------------------------------------------------------------
Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels
a heads up: I ma redoing the patch. But decided no to change the name
yet. That is a too big change. IT should be in a separate issue, IMO.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB)
* Bugs as reported by dries fixed.
* The [add] tab not appearing is due to your menu caching, refresh that
please.
* Fixed an additional bug: Some agents do not send TRUE for disabled
checkboxen. Also in current filter admin.
and people: we have a problem. Deleting a filter trows errors, due to
the revisions changes. that happens in filter module now, but also
after this patch. I have too little knowledge of the filter system to
fix that, and IMO that is a separate issue. It should be fixed right
after this patch is committed.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels
FYI: the delete bug is waiting here: http://drupal.org/node/30781
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck
Get this warning in PHP5: Warning: Missing argument 1 for
filter_admin_format() in \modules\filter.module on line 379
I have traced this error back to line 240. Why are there no callback
arguments like above on line 235? Looks like a source of a problem/bug
here so needs to be addressed, not sure exactly how to fix it myself
(otherwise I would). Should be easy it seems.
Also, for a quick and easy usability improvement, on line 239, change
t('list') to t('view') ... makes the menus less confusing.
After those fixes, looks like this patch is ready to be committed :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck
Also, this dialog is a bit confusing:
"Choose which roles may use this filter format
You are editing the default format. For the default format, all roles
must be enabled. Therefore you cannot change them.
"
Maybe make it so that first line doesn't appear there on that page?
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:35:50 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB)
changed that line of help.
Can this please still be taken in consideration before the freeze?
------------------------------------------------------------------------
Tue, 13 Sep 2005 18:29:28 +0000 : m3avrck
Can't apply patch, getting error:
Patch malformed at line 287
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:10:07 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_2_0.patch (14.19 KB)
strange. the dowloaded one breaks too, yet the one i uploaded stll
works.
anyway, rerolled
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:25:25 +0000 : Dries
IMO, the form function needs more work. You need to group the form
construction and the form saving. Right now, when you don't provide a
title, you are given an error and an emptied form. This may be a
left-over from the original code though but it would be cool if you
could tidy this up.
The form_group() description still need a trailing point.
There is a broken link in the following help text on the 'add input
format'-page: /If you notice some filters are causing conflicts in the
output, you can [rearrange] them./.
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:30:12 +0000 : Bèr Kessels
dont have time anymore. sorry.
------------------------------------------------------------------------
Tue, 13 Sep 2005 20:50:34 +0000 : m3avrck
new patch soon!
------------------------------------------------------------------------
Tue, 13 Sep 2005 22:10:25 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/filter.module_10.patch (20.27 KB)
Ok here's a new patch. Lot's of fixes in this one (all of the ones from
above except the one noted below), including support for the new
revisions patch.
Additionally, 'input formats' has been renamed to 'input filters'.
After debate on IRC, and the above comments in this post, it seems that
'input formats' is somewhat misleading and doesn't make sense. 'input
filters' clears this up and makes sense to both native and non-native
English speakers (as tested in IRC anywho ;)). This also makes more
sense since Drupal refers to everything as 'filters' and not 'formats'.
However, there is one issue still left unresolved: If you create a new
input filter, and don't specify a title, page reloads clearing all
filled in fields and prompts the error message. Additionally, a blank
filter is created.
Wanted to get this patch posted, I'll look at it later today but if
someone can get this fixed easily, please do, thanks!
------------------------------------------------------------------------
Tue, 13 Sep 2005 22:38:05 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_3.patch (21.49 KB)
this patch should fix the issue m3 rasies abuot amepty formats being
created.
Yes, i use a simple drupal_goto, in case of a for error. And no, it is
not very easy to get the old posted data back.
------------------------------------------------------------------------
Tue, 13 Sep 2005 23:44:26 +0000 : moshe weitzman
the most visible place for this 'input format' term is on the node form.
thats where ordinary users see this stuff. in that context, i don't
think 'input filters is an improvement over input formats. the question
we are asking our users is 'what is the format of your post'?, not what
filters do you want applied ... just my .02. sorry i wasn't in IRC when
this came up. this need not hold up the whole patch.
------------------------------------------------------------------------
Wed, 14 Sep 2005 06:48:03 +0000 : Bèr Kessels
Moshe. I agreed with you, and was against the name change. In this patch
at least.
However, it slipped in.
Stetting code to be committed for it has gone trough so many review
cycles that this patch has MUCH better code than the rest of the filter
module, which REALLY needs a big overhaul :)
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:37:27 +0000 : Dries
Are you saving the data before validating it? If we want to convert
this form to the new form API, we have to fix this cleanly. Sorry.
I agree with Moshe that 'input filters' is suboptimal. People want to
select how their text is going to be /processed/. The word/filter/
sounds rather technical to me. Also, the difference between a single
filter and a set of filters becomes rather blurry to the point it could
be very confusing.
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:41:42 +0000 : m3avrck
Well I was the one that changed 'input filters' to 'input formats' as
that seemed to be the best thing we could come up. I guess it got
confusing because people wanted this to change but maybe not in this
patch. So I will revert that change in a forthecoming patch.
Additinally, this patch is *not* ready. I was going through the form
logic last night and it is a bit off, hence why errors are displayed
along side a an empty form.
I'm going to work as hard as I can and as fast to clean this patch up
like I mentioned I would yesterday. I'll have some ready soon enough
that will be commit worthy.
------------------------------------------------------------------------
Wed, 14 Sep 2005 14:22:41 +0000 : Bèr Kessels
according to me its a go or leave. if m3 or anyone else wants to take
over fine. I am out. no time.
------------------------------------------------------------------------
Wed, 14 Sep 2005 21:30:45 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/filter.module_11.patch (28.03 KB)
Ok new patch! Everything is fixed. Code has been reworked and lots of
bugs fixed. Code should also be simpler to read (or at least follow in
terms of logic now). Ready to go!
1
0
Issue status update for
http://drupal.org/node/27364
Post a follow up:
http://drupal.org/project/comments/add/27364
Project: Drupal
Version: cvs
Component: filter.module
Category: tasks
Priority: normal
Assigned to: Bèr Kessels
Reported by: Bèr Kessels
Updated by: m3avrck
-Status: patch (code needs work)
+Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/filter.module_11.patch (28.03 KB)
Ok new patch! Everything is fixed. Code has been reworked and lots of
bugs fixed. Code should also be simpler to read (or at least follow in
terms of logic now). Ready to go!
m3avrck
Previous comments:
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels
This is a mockup for the Filter UI. Currently it is soo complex, that I
dare to call it broken.
Please comment on this, for I will not ake any patches, when the
general idea is disliked.
The idea is t split filters and input formats better. Filters are
filters, defined in modules. Input formats are bundles of filters. This
is how we have it ATM, but the interface fails to communicate that. I
tried to develop a consistent (with the rest of Drupal) interfce that
makes it clearer what is what.
The menu is as follows:
* administer
** ....
** settings
*** input formats
*** filters
** ...
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB)
* administer
** ....
** settings
*** input formats >> see attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB)
* administer
** ....
** settings
*** input formats >> see attachement 2, tab 2
*** filters
**
Note: The default checkbox lmay seem a bit odd. But checking it will
remove the "default status" from the current "default" one and make
this one "default". The help text, and a drupal_set_message should
explain this.
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB)
* administer
** ....
** settings
*** input formats >> clicking a 'configure' link in the table of
attachement 1
*** filters
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB)
* administer
** ....
** settings
*** input formats
*** filters >> see attachement 4
**
------------------------------------------------------------------------
Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB)
* administer
** ....
** settings
*** input formats
*** filters >> clicking a 'configure' link in the table of attachement
4
**
------------------------------------------------------------------------
Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels
IMO this makes the interface easier. But other disagree. And because it
also removes one feature: (being able to use E.g. HTML in different
input formats, with different settings)
I'll simply wont fix this :(
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB)
I decided to go for it anyway. Be it in a slightly different way then my
initial mockups. The difference is that I left the filters where they
are, hidden beneath the formats.
So, here is the patch that makes the filter UI more consistent with
screens like blocs, menus, vocabularies et al
also nice to note is that:
93 lines are added, 104 are removed. So netto we have less code :)
(cvs diff filter.module | grep ^+ | wc -l)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB)
Here is how the listing now looks.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB)
And the "configure" screen.
The 'add' screen looks exactly the same, only with the default values
pre-filled.
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB)
Oh, And here is a really nice example of why the current screen is no
good.
And here I "only" have four roles. I run a site with 12 roles, imagine
this screen then!
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB)
And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2
codebase. Sorry)
------------------------------------------------------------------------
Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal
I like this very, very much!
This is - indeed - much better than the current UI. This always fits,
and makes the life of people who use a fixed width theme much nicer..
Ber, FYI (you probably know this too), 'admin/access' does also break
fixed width themes due to the fact that it expands the width of the
table when more roles are being added..
Good catch! I like it much more this way..
+1 from me...
------------------------------------------------------------------------
Fri, 29 Jul 2005 07:27:34 +0000 : Dries
The code style needs work (spaces, tabs, 'as' -> 'AS'), the code
comments need work, and debug statements should be removed.
Screenshots look good to me. db_query('UPDATE {filter_formats} SET
cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d',
(int)$cache, $name, $roles, $format); can be written better/shorter
quote-wise.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB)
Dries, thanks a lot for the review. OI fixed your comments. Or so I
hope. code-style.pl was of no use (can that *please* be removed from
core?) because it chokes on the help texts. It also pointed out a lot
of old formatting isues, whic, when fixed wuold flood this patch with
unrelated fixes. I tried to narow my style-searching to the areas I
touched. And I made the comments more verbous.
The query is fixed. it saves no characters, but looks better now.
Thanks for the tip.
The print_r..... blush. thats what I get for not running my default
grep -r print_r on my code after finishing up...
I could not find a lot of spaces and tabs. So it lmight be that i
missed one. I triple checked it though.
------------------------------------------------------------------------
Fri, 29 Jul 2005 09:32:21 +0000 : Steven
Moving the role settings inward does fix the wide-table problem, but it
makes the filter configuration much more opaque. We could perhaps fix
this by showing the roles as a comma-separate list in the input formats
table. Such a list can wrap and will not stretch the table.
Other than that:
* Having the instructions for the Roles and Filters form_group() at the
bottom is a bit weird. The distance is too big.
* The table that was used for toggling filters on/off was much more
compact before, and IMO a lot clearer. Why change to a flat list with
the description staggered below each item?
------------------------------------------------------------------------
Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels
consistancy, consistancy and more consistancy.
There really is nothing worse that having a different 'concept' of UI
for every thing in Drupal. We are going in the right direction, with
the blocks being editable objects, menus acting like that, taxonomy,
users, etc..
I wanted to make this act similar. I am not at all happy with the fact
tah the list is still a form. But we need this, for setting the
default.
A big -1 for a comma-separate list. That is extremely hackish, even
worse useabilitywise and just silly
* we should avoid typing when not needed.
* we should avoid errors on input, when they can be avoided (a typo
is made very easy, I know all about it) Form set errors won't fix that
usability -wise.
I agree that you sort-of loose the overview. But IMO that is a minor
loss comared to what we gain, usability-wise.
The wtoo-wide was onlywhat triggered me. But do not think that I am
noly making this patch to fix that issue. This patch is there to
improve usability, through consistancy. o make drupal behave the same
in most places in admin. If you know one screen, you know most of them.
We really should move away from custom-hacks for every page, because
that is useability rule #1: consistancy. Stadardisation, even if
sometimes a custom hack might make things easier in that single case,
it will reduce your overall usability so much that nearly ever it is
worth that hack.
That select-boxes instead of the table issue: consistancy. As well that
it completely broke fluid CSS layouts, my solution is more consistant,
and uses forms the way they are meant. If we cannot use $descritption
in checkboxes they should be removed. but IMO it feels much more
consistant. Tables are for tabular data, and this is simply a list of
items, with additional data.
And i do not really get what you mean with "too far down". Do you mean
that you would like to see the order of the groups different?
------------------------------------------------------------------------
Fri, 29 Jul 2005 13:31:24 +0000 : Dries
I haven't tested Ber's patch yet but looking at the screenshots, it like
what I see. I remember being confused by the current filter UI, and
often, I still am.
------------------------------------------------------------------------
Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman
I dislike the recent movement away from overview pages. I think we can
still provide and overview while allowing editing to happen in a
dedicated form. An example in the wrong direction is 'content types'
admin. It is impossible to get an overview, and the listing page is
worthless. I agree with Steven on this one.
It is true that consistency is desirable. But consistent crap is not.
------------------------------------------------------------------------
Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org
I agree with Moshe. Getting a quick overview about what is set how is
essential for an admin.
------------------------------------------------------------------------
Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques
Overall I like the screenshots. Did not test the patch.
Bèr, what I think Steven (#15) meant by "showing the roles as a
comma-separate list in the input formats table" is that as an overview
page it would be better if you added a list (not an input box!) to
screenshot "input-formats_list1.png" (comment #1). Add a column that
reads "Roles" (or "Enabled for") and then for each row a comma
seperated list of the roles that can use this input format (or maybe
even better a HTML list). You would still need to click "configure" to
_change_ the roles.
The "Default" text (not a radio button) you already show in the
"Options" column is something similar: it immediately shows that this
input format is the default without you having to go to the "configure"
page. Why not do the same for "roles"?
If this is what Steven meant, then I tend to agree with him.
And to help moshe and killes: maybe you could even add a column that
_lists_ the used input filters. Again, only a list, not a way to change
it. That's what the "operations" column is for.
A table like that will "fluid" better than what we have now, still it
is a _complete_ overview of the settings.
I could have misunderstand someone... I have the tendency to do that...
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB)
by popular demand: a comma separated list.
------------------------------------------------------------------------
Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB)
and a screenshot
------------------------------------------------------------------------
Mon, 22 Aug 2005 18:39:44 +0000 : Dries
Please check your use of print theme('page', $output). Normally, you
just return $output.
------------------------------------------------------------------------
Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels
All instances of theme('page',...) in filter.module are now changed to
return ... ;
furthermore, the patch is the same.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann
Bèr, I think you forgot to attach your updated patch.
------------------------------------------------------------------------
Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB)
my email client has this nice warning system for attachements. Maybe
drupal should search for the word attachement too, and when no att.
found give an error ;). Or maybe i should just learn to pay attention.
Guess that is easier.
Anyway, here it is.
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:22:54 +0000 : Dries
I wanted to apply this patch but:
1. I can't change the default input format. 'Save'-button doesn't
work.
2. I got confused by the fact I can't change the roles of the default
filter. I think the form group description should explain this.
3. form_group(t(filter)) should be form_group(t('filter'), ...).
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck
Found a few more issues:
If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab
at the top and then click 'list filters' link in the text, I get this
warning with PHP5 warning: Missing argument 1 for filter_admin_format()
in \drupal\modules\filter.module on line 378.
Also, it is sort of confusing as to what the 'List' tab implies. For
example, if I click on 'input formats' in the admin menu, I get a list
of the current filters. When I click 'configure' for one of those, I
see the options for that filter. However, it says 'list' in one of the
tab, which doesn't really mean much. I thought that was the list of
filters, not the options for that filter. That should be cleaned up.
Also, there is no way to get back to the full list of filters unless
you click on 'input formats' in the admin menu. A setting/link to fix
this would be great.
Also, where is the link to add filters???
I think the options/layout/tabs should mimic of that of admin > users.
So on admin > input formats, it has the tabs 'list', 'add format' ...
very clear and consistent.
On a configure page for a filter, it should have 'list', 'view',
'configure', 'rearrange' ... this would make it easier to navigate and
get back to the original list of filters.
Make sense? I think this and Dries' comments put this patch over the
top :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 01:48:55 +0000 : tag
In my initial battle to figure out this module's (quite nice)
functionality, what tripped me up the most was actually the
nomenclature.
The way I keep it straight in my head now is to translate "input
format" to "filter set". If you view those (the current) screens with
this in mind, I think it's a lot easier to understand what's what. It
sort of describes this in the help text, but the terms "filter" and
"input format" don't really convey the relationship between the two --
there's no way to intuit that an "input format" is a collection of
"filters". Not to my mind at least... and well, we know nobody reads
instructions...
Are there any technicalities that make renaming "input format" to
"filter set" not accurate? Or does someone have a better term/terms to
better show the relationship of these elements? (I'm aware of filters
in servlet architectures but I don't think there's much of a collision
with that usage).
Anyhow, I guess this doesn't quite address the UI controls and/or
layout, but would still help a lot.
------------------------------------------------------------------------
Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels
I prefer filter set. In fact: I like it a lot. Anyone else ideas on
this?
------------------------------------------------------------------------
Fri, 09 Sep 2005 10:18:18 +0000 : Dries
I too had problems with the terminology; it took me months to get used
to 'input formats' versus 'filters'. Using 'filter sets' might improve
the situation -- especially from an administrator's point of view when
you have to wrap your head around the UI/difference. I'm not native
English, but 'filter set' sounds more explanatory, yet slightly more
geeky so I'm not sure if it flies for users who don't need to
understand the underlying concepts (eg. under the node and comment
submission forms).
------------------------------------------------------------------------
Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels
a heads up: I ma redoing the patch. But decided no to change the name
yet. That is a too big change. IT should be in a separate issue, IMO.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB)
* Bugs as reported by dries fixed.
* The [add] tab not appearing is due to your menu caching, refresh that
please.
* Fixed an additional bug: Some agents do not send TRUE for disabled
checkboxen. Also in current filter admin.
and people: we have a problem. Deleting a filter trows errors, due to
the revisions changes. that happens in filter module now, but also
after this patch. I have too little knowledge of the filter system to
fix that, and IMO that is a separate issue. It should be fixed right
after this patch is committed.
------------------------------------------------------------------------
Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels
FYI: the delete bug is waiting here: http://drupal.org/node/30781
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck
Get this warning in PHP5: Warning: Missing argument 1 for
filter_admin_format() in \modules\filter.module on line 379
I have traced this error back to line 240. Why are there no callback
arguments like above on line 235? Looks like a source of a problem/bug
here so needs to be addressed, not sure exactly how to fix it myself
(otherwise I would). Should be easy it seems.
Also, for a quick and easy usability improvement, on line 239, change
t('list') to t('view') ... makes the menus less confusing.
After those fixes, looks like this patch is ready to be committed :)
------------------------------------------------------------------------
Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck
Also, this dialog is a bit confusing:
"Choose which roles may use this filter format
You are editing the default format. For the default format, all roles
must be enabled. Therefore you cannot change them.
"
Maybe make it so that first line doesn't appear there on that page?
------------------------------------------------------------------------
Tue, 13 Sep 2005 13:35:50 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB)
changed that line of help.
Can this please still be taken in consideration before the freeze?
------------------------------------------------------------------------
Tue, 13 Sep 2005 18:29:28 +0000 : m3avrck
Can't apply patch, getting error:
Patch malformed at line 287
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:10:07 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_2_0.patch (14.19 KB)
strange. the dowloaded one breaks too, yet the one i uploaded stll
works.
anyway, rerolled
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:25:25 +0000 : Dries
IMO, the form function needs more work. You need to group the form
construction and the form saving. Right now, when you don't provide a
title, you are given an error and an emptied form. This may be a
left-over from the original code though but it would be cool if you
could tidy this up.
The form_group() description still need a trailing point.
There is a broken link in the following help text on the 'add input
format'-page: /If you notice some filters are causing conflicts in the
output, you can [rearrange] them./.
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:30:12 +0000 : Bèr Kessels
dont have time anymore. sorry.
------------------------------------------------------------------------
Tue, 13 Sep 2005 20:50:34 +0000 : m3avrck
new patch soon!
------------------------------------------------------------------------
Tue, 13 Sep 2005 22:10:25 +0000 : m3avrck
Attachment: http://drupal.org/files/issues/filter.module_10.patch (20.27 KB)
Ok here's a new patch. Lot's of fixes in this one (all of the ones from
above except the one noted below), including support for the new
revisions patch.
Additionally, 'input formats' has been renamed to 'input filters'.
After debate on IRC, and the above comments in this post, it seems that
'input formats' is somewhat misleading and doesn't make sense. 'input
filters' clears this up and makes sense to both native and non-native
English speakers (as tested in IRC anywho ;)). This also makes more
sense since Drupal refers to everything as 'filters' and not 'formats'.
However, there is one issue still left unresolved: If you create a new
input filter, and don't specify a title, page reloads clearing all
filled in fields and prompts the error message. Additionally, a blank
filter is created.
Wanted to get this patch posted, I'll look at it later today but if
someone can get this fixed easily, please do, thanks!
------------------------------------------------------------------------
Tue, 13 Sep 2005 22:38:05 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/filter_interface_improvements_3.patch (21.49 KB)
this patch should fix the issue m3 rasies abuot amepty formats being
created.
Yes, i use a simple drupal_goto, in case of a for error. And no, it is
not very easy to get the old posted data back.
------------------------------------------------------------------------
Tue, 13 Sep 2005 23:44:26 +0000 : moshe weitzman
the most visible place for this 'input format' term is on the node form.
thats where ordinary users see this stuff. in that context, i don't
think 'input filters is an improvement over input formats. the question
we are asking our users is 'what is the format of your post'?, not what
filters do you want applied ... just my .02. sorry i wasn't in IRC when
this came up. this need not hold up the whole patch.
------------------------------------------------------------------------
Wed, 14 Sep 2005 06:48:03 +0000 : Bèr Kessels
Moshe. I agreed with you, and was against the name change. In this patch
at least.
However, it slipped in.
Stetting code to be committed for it has gone trough so many review
cycles that this patch has MUCH better code than the rest of the filter
module, which REALLY needs a big overhaul :)
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:37:27 +0000 : Dries
Are you saving the data before validating it? If we want to convert
this form to the new form API, we have to fix this cleanly. Sorry.
I agree with Moshe that 'input filters' is suboptimal. People want to
select how their text is going to be /processed/. The word/filter/
sounds rather technical to me. Also, the difference between a single
filter and a set of filters becomes rather blurry to the point it could
be very confusing.
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:41:42 +0000 : m3avrck
Well I was the one that changed 'input filters' to 'input formats' as
that seemed to be the best thing we could come up. I guess it got
confusing because people wanted this to change but maybe not in this
patch. So I will revert that change in a forthecoming patch.
Additinally, this patch is *not* ready. I was going through the form
logic last night and it is a bit off, hence why errors are displayed
along side a an empty form.
I'm going to work as hard as I can and as fast to clean this patch up
like I mentioned I would yesterday. I'll have some ready soon enough
that will be commit worthy.
------------------------------------------------------------------------
Wed, 14 Sep 2005 14:22:41 +0000 : Bèr Kessels
according to me its a go or leave. if m3 or anyone else wants to take
over fine. I am out. no time.
1
0
Issue status update for
http://drupal.org/node/30801
Post a follow up:
http://drupal.org/project/comments/add/30801
Project: Drupal
Version: 4.6.0
Component: block.module
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: Allie Micka
Updated by: Allie Micka
Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/block_query.patch (1.25 KB)
The blocks module is running a separate query for each region, e.g.:
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='left' ORDER BY weight, module
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='right' ORDER BY weight, module
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='header' ORDER BY weight, module
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 AND
region='footer' ORDER BY weight, module
Ultimately, we're going to need all of these so let's save a few
queries. I have changed the query to:
SELECT * FROM blocks WHERE theme = 'bluemarine' AND status = 1 ORDER
BY region, weight, module
And set all results to the static $blocks array on the first pass.
Allie Micka
6
23
Issue status update for
http://drupal.org/node/28595
Post a follow up:
http://drupal.org/project/comments/add/28595
Project: Drupal
Version: cvs
Component: comment.module
Category: feature requests
Priority: normal
Assigned to: mrowe
Reported by: robertDouglass
Updated by: Dries
-Status: patch (code needs review)
+Status: patch (code needs work)
- Don't use tabs.
- SQL queries might be vularnable to SQL injection attacks. Use
db_query()'s %s and %d directives.
Dries
Previous comments:
------------------------------------------------------------------------
Wed, 10 Aug 2005 08:29:14 +0000 : robertDouglass
When using the comment approval queue the workflow for approving
comments is awful. You have to click administer->comments->approval
queue to get a list of comments awaiting approval. Then you have to
click edit on one of the comments, click the collabsible field-set for
the publishing controls, click to published and save the comment. Then
repeat the process for the next comment.
I'd say that this workflow is a significant deterrent to anyone using
this functionality.
To make it better, there would be checkboxes next to each comment in
the table on the approval queue page, and a set of operators analogous
to the content overview page. Better yet would be if each comment in
the table was an AJAX link to the comment's body and info which would
expand right there in the table itself, since it is usually impossible
just from the comment title to know whether to publish or not. If one
could click the various comments open, right there in the table, then
mass publish or delete, we'd have a real approval queue. Perhaps the
spam module could tie into the actions that can be taken on comments so
a mass "mark as spam" could be done too.
------------------------------------------------------------------------
Wed, 14 Sep 2005 13:32:47 +0000 : mrowe
Attachment: http://drupal.org/files/issues/comment_bulk_approval.patch (2.24 KB)
Here's a patch (against HEAD) that gives you the cheap-and-dirty
approach (checkboxes that allow bulk approval/unapproval of comments).
I'll leave the gold-plated AJAX solution to someone with more time. :)
1
0
14 Sep '05
Issue status update for
http://drupal.org/node/30993
Post a follow up:
http://drupal.org/project/comments/add/30993
Project: Drupal
Version: cvs
Component: taxonomy.module
Category: feature requests
Priority: normal
Assigned to: robertDouglass
Reported by: robertDouglass
Updated by: robertDouglass
Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/taxonomy.txt (4.32 KB)
The taxonomy select widget becomes unmanagable very quickly. If there
are more than two levels in a hierarchy or more than X number of terms,
it is almost impossible to use. This patch makes it a themable element
so that creative UI designers can start cooking up better solutions.
Here are the changes, in detail:
* There is a new function theme_taxonomy_term_select which is a carbon
copy of the old _taxonomy_term_select, plus XDoclet comments.
* The old _taxonomy_term_select function calls the theme function using
the theme() syntax.
* taxonomy_form has an extra parameter $title = NULL so that the label
on the form can be set by the public API. When NULL it defaults to the
vocabulary name as usual. The reason I need this and I think Drupal
needs this is that taxonomy is really useful for lists of items that
need to be reused. Take the list {good, bad, ugly}. This set of terms
could describe lots of things, defying any attempt to give the
vocabulary a catchall name. Now, with the updated taxonomy_form, I can
create a form element called 'Rate my code' and get the abovementioned
list. In the same form I could have another element called 'George Bush
is' and use the same list over again.
* taxonomy_form now has XDoclet comments.
robertDouglass
9
16
Issue status update for
http://drupal.org/node/27140
Post a follow up:
http://drupal.org/project/comments/add/27140
Project: Drupal
Version: cvs
Component: contact.module
Category: bug reports
Priority: normal
Assigned to: Souvent22
Reported by: m3avrck
Updated by: Dries
-Status: patch (ready to be committed)
+Status: patch (code needs work)
Patch looks good but needs a bit of work:
1. You don't need to urlencode numeric IDs: urlencode($category->cid)).
2. You can write %d rather than '%d' in queries.
Except for that (and without any testing), the code looks good. This
change can go in after the code freeze; it's a bugfix so you might want
to focus on other stuff first.
Dries
Previous comments:
------------------------------------------------------------------------
Wed, 20 Jul 2005 14:57:03 +0000 : m3avrck
Using the site-wide contact forms, any subjects with an '&' cannot be
deleted. Clicking delete for these subjects brings up the expected
action flow, however, confirming the delete does nothing. Subject still
exists.
------------------------------------------------------------------------
Mon, 25 Jul 2005 13:45:38 +0000 : m3avrck
It appears this issue might be linked with this one:
http://drupal.org/node/23685
A problem with mod_rewrite and not the actual contact module.
------------------------------------------------------------------------
Wed, 31 Aug 2005 16:51:56 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact.module_6.patch (4.7 KB)
Redid how contacts work. It now references contacts by ID instead of
their category as the unique identifier. It also allows for contacts to
have weights, so that one may control where the contact will appear in
the listing.
------------------------------------------------------------------------
Wed, 31 Aug 2005 16:53:01 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/updates.inc (27.23 KB)
This is the update.inc for the database cahnges that goes along with
this patch.
------------------------------------------------------------------------
Wed, 31 Aug 2005 18:31:37 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact.module_7.patch (4.5 KB)
Updated patch.
------------------------------------------------------------------------
Wed, 31 Aug 2005 18:31:55 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/updates.inc_4.patch (1.29 KB)
Updated database patch.
------------------------------------------------------------------------
Sat, 03 Sep 2005 16:17:21 +0000 : Cvbge
Hi,
0. You should keep all changes in one file, i.e. pake one patch, not
patch for every file
1. You should include changes for database/database.{mysql,pgsql}
2. IIRC auto_increment was depreciated, you should use db_next_id()
(but I see auto_increment in cvs...)
3. You use "WHERE cid = '%s'", but cid is integer so this should
probably be %d (although this probably would not break anything..)
------------------------------------------------------------------------
Wed, 07 Sep 2005 17:31:21 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact_full.patch (6.38 KB)
Ok, I've taken your suggestions in, and i think I have it this time. Let
me know if this is a correct format for a patch. This only fixes the bug
of not being able to delete special characters.
------------------------------------------------------------------------
Wed, 07 Sep 2005 17:37:30 +0000 : m3avrck
Check your database updates, the UNIQUE fields don't look right
syntactically.
------------------------------------------------------------------------
Wed, 07 Sep 2005 18:23:07 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact_full_0.patch (6.25 KB)
Ok, i apologize for the last upload. Pleasle ignore. I send the wrong
file. :-[. Anyway, here's a tested, tried, and correct patch.
------------------------------------------------------------------------
Wed, 07 Sep 2005 18:26:33 +0000 : m3avrck
+1 patch works great! Latest HEAD version applied with update.php works
great, modifies database correctly for MySQL (and PostgreSQL looks
correct as well) and contacts can now be deleted if they have '&' in
the subject. Also, dumped entire database and created from modified
database.mysql and everything works great. I'd say this patch is ready
to be committed!
------------------------------------------------------------------------
Wed, 07 Sep 2005 21:44:47 +0000 : Cvbge
Hi, patch does not applies to currect cvs anymore.
For the database.pgsql you need to remove (11) from int(11) and also
first "category" from UNIQUE line, so it looks like this:
CREATE TABLE contact (
cid int NOT NULL,
category varchar(255) NOT NULL default '',
recipients text NOT NULL default '',
reply text NOT NULL default '',
UNIQUE (category),
PRIMARY KEY (cid)
);
The updates are also not compatibile with postgres, but postgres
upgrade path is completly broken in cvs and I'll fix it later, so do
not pay attention to it.
------------------------------------------------------------------------
Thu, 08 Sep 2005 13:05:54 +0000 : Souvent22
Hello. Thanks for the feedback. I'm in the middle of fixing it, but i
have a question. You'll have to excuse my ignorance with PostGres, I'm
not as familiar with it as I am MySQL. With that said, I have a
question.....
I am trying to make my update where one does not lose data. However, in
PostGres, I have to make a seperate "temp" table before hand and then
copy the date over the the new table (cause i don't know the exact name
of the constraint to drop e.g. the primary key).
Basically I'm asking if there is a better suggestion in which I doint
have to use a temp table, and i could just manipulate the original
table? Thanks.
------------------------------------------------------------------------
Thu, 08 Sep 2005 13:12:35 +0000 : Souvent22
Also,
I created a temp name for my temp table. However, for prefix purposes,
should I enclose it? Example:
$temp_name = rand();
$res = db_query("SELECT * FROM $temp_name");
or
$temp_name = rand();
$res = db_query("SELECT * FROM {$temp_name}");
Thanks.
------------------------------------------------------------------------
Thu, 08 Sep 2005 13:55:40 +0000 : Souvent22
Clarification:
$temp_name = rand();
db_query("CREATE TABLE $temp_name AS SELECT * FROM {contact}");
$res = db_query("SELECT * FROM $temp_name");
or
$temp_name = rand();
db_query("CREATE TABLE {$temp_name} AS SELECT * FROM {contact}");
$res = db_query("SELECT * FROM {$temp_name}");
------------------------------------------------------------------------
Thu, 08 Sep 2005 17:52:06 +0000 : Cvbge
"cause i don't know the exact name of the constraint to drop e.g. the
primary key
"
Yes, you know it, it's tablename_pkey. In this case it's contact_pkey.
You can drop it by dropping index with it's name (you could drop
constraint with it's name but it was not available in 7.2)
Adding a column and adding unique attribute is also doable without temp
table. You can look at (some) previous updates. But some of them are
incorrect ;)
By saying that I'll fix it later I meant that I want to introduce
function to modify tables and don't want to make temporary fixes now.
------------------------------------------------------------------------
Thu, 08 Sep 2005 19:47:08 +0000 : Souvent22
Thanks. yes, you're right. didn't know if that was universal across the
board though since that'st he default, but someone could give their
primary key constraint a diff. name. But, with your comment, I'll wait
and let you take care of fixing my pgsql portion of my patch. Thanks for
your comments though, really helpful...and not confusing. :).
------------------------------------------------------------------------
Fri, 09 Sep 2005 19:16:19 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact_full_1.patch (6.23 KB)
Got the postgres correct this time. No temp table needed.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:23:09 +0000 : Souvent22
Attachment: http://drupal.org/files/issues/contact_full_2.patch (6.35 KB)
patch re-roll due to head change.
------------------------------------------------------------------------
Tue, 13 Sep 2005 14:36:11 +0000 : chx
Does this merit a DB change? what about passing md5(subject) instead? or
is that too hackish? seems to be lot simpler to me...
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:15:16 +0000 : m3avrck
chx, md5() would work but that does seem a bit hackish ;)
------------------------------------------------------------------------
Tue, 13 Sep 2005 16:24:34 +0000 : Cvbge
Attachment: http://drupal.org/files/issues/drupal-head-contact-27140_0.diff (6.81 KB)
It's case 'pgsql' not case "pg_sql"
Also, I've noticed now that you use db_next_id().
First, the format is db_next_id('{tablename}_fieldname'), so in this
case it should be db_next_id('{contact}_cid')
Second, using db_next_id() with postgres needs a sequence defined.
Also update path needs changes in case there are already some contacs
in the table - the new column cid needs to be updated and sequences
set.
I've fixed this [for postgres] and attached new patch. I'm not sure how
mysql behaves when adding a primary key and there is an existing data,
buy you probably have to take care of it (if mysql allows you to add a
column that is a primary key and allows it to have nulls then... well,
I don't want to swear ;))
I've tested the code and it seems to run ok.
------------------------------------------------------------------------
Tue, 13 Sep 2005 22:21:23 +0000 : Cvbge
About the update path: since this module is in cvs only, not in 4.6,
then we probably should not worry about data being already in the
table.
(Although drupal.org is already running on cvs with contact.module :))
1
0
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: Souvent22
Status: patch (ready to be committed)
The problem with your upload file lists dissapearing has been resolved.
http://drupal.org/node/31102
Souvent22
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.
------------------------------------------------------------------------
Mon, 25 Jul 2005 12:42:26 +0000 : Bèr Kessels
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!
------------------------------------------------------------------------
Tue, 26 Jul 2005 14:18:23 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/upload_inline_0.patch (12.24 KB)
Suggestions by Steven are now included. I very simple float is added to
the inline images in drupal.css. XHTML fixed.
And I cleaned up some old crufty comments, as suggested by Dries.
------------------------------------------------------------------------
Tue, 26 Jul 2005 17:05:45 +0000 : fax8
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????
------------------------------------------------------------------------
Tue, 26 Jul 2005 20:00:03 +0000 : Bèr Kessels
fax: upload /is/ such a layer. Just look at how image module does it.
You need not write any additional 'buggy' code, you can use all teh
upload functionality.
The upload interface (the table and list), however, is tightly
integrated, so that drupal offers an out-of-the-box file system.
Nox, my patch simply adds an "out of the box" inline functionality.
Small and simple.
For any more advanced file features you should use or write a module
that, in turn, uses the uplaod.module.
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:22:40 +0000 : stefan nagtegaal
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..
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:27:52 +0000 : Kobus
> - 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
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:51:50 +0000 : Bèr Kessels
Kobus: Again: it does not break anything!
It opens up enormous potential for modules such as inline/module.
Because inline module now has access to a variable
$node->files[fid]->inline.
in other words: modules such as inline.module can now find out what
images a user wants to see inline!
------------------------------------------------------------------------
Wed, 27 Jul 2005 11:59:53 +0000 : Kobus
Ber: Great, then I +1 it. From our previous talks about this, it seemed
as if this functionality was being taken away.
------------------------------------------------------------------------
Wed, 27 Jul 2005 16:18:21 +0000 : Sohodojo Jim
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--
------------------------------------------------------------------------
Wed, 27 Jul 2005 17:33:47 +0000 : Bèr Kessels
inline module will continue working. but in order for it to use this
inline variable (and become even better) it needs to be patched.
------------------------------------------------------------------------
Thu, 28 Jul 2005 18:12:09 +0000 : moshe weitzman
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.
------------------------------------------------------------------------
Thu, 28 Jul 2005 18:32:40 +0000 : Bèr Kessels
disabling the box for non-images would break the link function (i.e add
a link in the text. But, on second thought: that is fine, since it
defeats the list checkbox.
I will add that. As well as making it checked by default.
------------------------------------------------------------------------
Thu, 28 Jul 2005 19:54:14 +0000 : Junyor
@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.
------------------------------------------------------------------------
Thu, 28 Jul 2005 21:19:37 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/upload_inline_1.patch (19.71 KB)
to all: everyone has Big Plans[tm] and even worse: everyone has Bigger
Plans for my patch, for inline module. Why not code it? Hooks? fine!
module integration, even better. Sorry if I sound a bit grumpy here.
But so far I have spent nearly three times as much hours debbating the
patch, then I spent creating it. Hooks sound great. but are not the
intention of this patch. I want to add a *default* way, that works. But
that does not break other modules, nd in fact tries to help them. But I
am not inventing some advanced image handling system here.
Here is an updated patch with renewed updates.inc (got lost in the
previous patch)
Moshes suggestion about disabling non-images-checkboxes is also
implemented. I like it a lot more now.
I did not, however, implement Moshes idea about defaulting to
"checked". for a few reasons:
* It looks a bit silly if you have more then one image inline, And
defaulting all uploads to inline will inline any uploaded image.
* When uploading large images they will default to inline, thus pushing
your sites layout to oblivian
* None of the other options default to TRUE, so for consistancy I
prefer to default it to not inline.
------------------------------------------------------------------------
Thu, 28 Jul 2005 21:24:26 +0000 : Bèr Kessels
oh, bytheway: you can test adn play with this patch here:
http://fixme.remixreading.org/?q=add_content
note that it is a sandbox, so your account will get lost, and pages
might not work :)
------------------------------------------------------------------------
Fri, 29 Jul 2005 03:47:35 +0000 : ejort
+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.
------------------------------------------------------------------------
Fri, 29 Jul 2005 06:38:46 +0000 : Steven
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?
------------------------------------------------------------------------
Tue, 02 Aug 2005 18:33:30 +0000 : Bèr Kessels
Steven: what I mean, is that they have access to a variable that tells
them a user wants something inline. Nothing more. Its up to these
modules to use that meaningfully.
------------------------------------------------------------------------
Thu, 01 Sep 2005 15:26:36 +0000 : Bèr Kessels
The patch still applies. Dries, please let me know if I must bother
trying to get this in before 4.7?
------------------------------------------------------------------------
Thu, 01 Sep 2005 15:30:09 +0000 : m3avrck
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.
------------------------------------------------------------------------
Thu, 01 Sep 2005 19:46:35 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/upload_inline_2.patch (20.02 KB)
updated patch works with revisions and JS upload.
------------------------------------------------------------------------
Thu, 01 Sep 2005 20:32:11 +0000 : m3avrck
got an extra 'favico' in the patch file there.
------------------------------------------------------------------------
Thu, 01 Sep 2005 22:47:24 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/upload_inline_2_0.patch (19.22 KB)
favicon removed.
------------------------------------------------------------------------
Thu, 01 Sep 2005 23:06:11 +0000 : killes(a)www.drop.org
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.
------------------------------------------------------------------------
Thu, 01 Sep 2005 23:27:42 +0000 : Bèr Kessels
how would the pgsql look then? I am a complete pgSQL nitwit :)
------------------------------------------------------------------------
Fri, 02 Sep 2005 15:18:47 +0000 : telex4
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.
------------------------------------------------------------------------
Fri, 02 Sep 2005 17:26:49 +0000 : m3avrck
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 upload_space_used() [2] instead. Also, beginning of
upload.module patch adds the default maxsize setting which was
previously taken out becuase it wasnt working properly. This patch [3]
corrects that and is a seperate issue :)
[2] http://drupaldocs.org/api/head/function/upload_space_used
[3] http://drupal.org/node/30025
------------------------------------------------------------------------
Fri, 02 Sep 2005 18:31:12 +0000 : Steven
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.
------------------------------------------------------------------------
Fri, 02 Sep 2005 18:48:32 +0000 : Dries
It would make sense for Ber to provide one or two examples of how this
could be used.
------------------------------------------------------------------------
Fri, 02 Sep 2005 21:02:39 +0000 : Bèr Kessels
@Steven: no I did not miss your point.
on nodeapi view this is appended to the $node->body, regardless. You
are correct.
But nothing witholds you from using that variable *before* the view is
invoked. ou can then choose to unset the variable, or leave it.
I first had yet another hook, that would be called on nodeapi view, by
the upload, on a per-file base; but then I found that all this was very
OTT, since it ca all be achieved with the existing nodeapi. I reverted
to the nodeapi.
@Dries; examples, sure.
1) Look for the result at http://fixme.remixreading.org/node/595, the
small image at the right is rendered with this variable. 5and I must
add that users that tested this behaviour were VERY positive!)
In phptemplate, the node, we do:
<?php foreach ($node->files as $file): ?>
<?php if ($file->inline): ?>
<div id="preview">
<img src="files/<?php print $file->filename ?>"
alt="preview" title="preview of <?php print $node->title ?>" />
</div>
<?php endif; ?>
<?php endforeach; ?>
Because we have ourtheme_upload_inline() to return NULL, this looks
like you see online. Together with some CSS you get a very nice result.
2) For most weblogs, or newssites, you need not do anything, for the
$node->body has the image already appended *if* the inline checkbox is
checked. Esp since these types of sites need a unified look, they do
not (or should not) need to alter the place and aligning of each image.
3) For the power users there are node-layout tools available such as
tinyMCE, wikisyntax or even inline.module. Since that last one is the
simplest I use that as example.
<?php
function inline_nodeapi(&$node, $op, $arg) {
if(is_array($node->files) && $op == 'view') { //on view we replace the tokens with the img tags
$node->teaser = _inline_substitute_tags($node, 'teaser');
$node->body = _inline_substitute_tags($node, 'body');
}
elseif(is_array($node->files) && $op == 'validate') {
$node = _inline_remove_tags($node); //remove the tags that are not flagged as inline
}
elseif(is_array($node->files) && $op == 'load') {
_inline_remove_flags($node) //remove all $node->files[XX]->inline variables, so that upload does not render them.
}
elseif(is_array($node->files) && $op == 'from pre') {
$node = _inline_add_tags($node); //add [inline:X] tokens to 'body', so editor can use them.
$node = _inline_remove_tags($node); //remove [inline] tokens that call files that are (no longer) marked for inline use.
}
}
//....
function _inline_add_tags(&$node, $field) {
foreach ($node->files as $file) {
if ($file->inline && !_inline_check_for_tag($file->filename)) { //look if the tag is not already in body, and if the file is marked for inline usage.
$node->body = _inline_add_single_tag($file) . $node->body; //Actually add the token to the beginning of the body
$node->teaser = _inline_add_single_tag($file) . $node->teaser; // same for teaser
}
}
}
?>
//note. preview is broken no Idea how this post looks ;)
------------------------------------------------------------------------
Sat, 03 Sep 2005 04:37:23 +0000 : TDobes
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.
------------------------------------------------------------------------
Sat, 03 Sep 2005 06:37:17 +0000 : Bèr Kessels
in short my battle plan was:
* introduce inline stuff
* introduce hook for altering the file table
Why i did not go for #2 at once? because IMO you can do two things wit
a file: show it, or download it. Anything else should be done in a file
admin UI.
And furthermore: yes modules are ran in alphabetic order, but,
nodeapi(load) is ALWAYS ran before nodeapi(view). So you can always
make sure on nodeapi(view) the variable is unset.
------------------------------------------------------------------------
Sat, 03 Sep 2005 12:22:00 +0000 : moshe weitzman
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.
------------------------------------------------------------------------
Sat, 03 Sep 2005 13:08:05 +0000 : Bèr Kessels
the patch is still in brokenish state. I am trying to free some time
this weekend (sunday evening) to fix it and synch with HEAd again. not
h revisions and JS upload broke this patch. Its a lot of work to
reroll, due to that amount of changes (nearly a complete rewrite, I am
afraid)
------------------------------------------------------------------------
Mon, 12 Sep 2005 17:48:47 +0000 : Bèr Kessels
please review. This a complete rewrite,
* I ignored all the huge fucntions and just left tath for another ime
to clean up (never). It does make the patch easier to read and smaller,
though
* I left the 'upload preview' bug for what it is: it belons in a
differnt issue, and would clutter this inline feature patch a lot. For
the rest see above.
* Themes could be updated to do nice stuff to the inline patch. I did
not do that, for that goes far beyond what this patch tries to do
------------------------------------------------------------------------
Mon, 12 Sep 2005 17:49:57 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/upload_inline_2_4.patch (14.13 KB)
No, please review /this/ patch.
------------------------------------------------------------------------
Tue, 13 Sep 2005 18:35:55 +0000 : m3avrck
upload.module is listed twice in the patch ... so patch isn't work,
please update!
------------------------------------------------------------------------
Tue, 13 Sep 2005 19:17:38 +0000 : Bèr Kessels
Attachment: http://drupal.org/files/issues/upload_inline_2_5.patch (8.66 KB)
last attempt
------------------------------------------------------------------------
Wed, 14 Sep 2005 17:50:24 +0000 : Souvent22
+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.
------------------------------------------------------------------------
Wed, 14 Sep 2005 18:14:22 +0000 : Souvent22
Ber,
As stated earlier about the bug, there seems to be a similar issue:
http://drupal.org/node/31102
------------------------------------------------------------------------
Wed, 14 Sep 2005 18:15:20 +0000 : Souvent22
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.
1
0
Issue status update for
http://drupal.org/node/31102
Post a follow up:
http://drupal.org/project/comments/add/31102
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
-Assigned to: Anonymous
+Assigned to: Souvent22
Reported by: m3avrck
Updated by: Souvent22
-Status: active
+Status: patch (code needs review)
Attachment: http://drupal.org/files/issues/upload.list.patch (1.07 KB)
List problem fixed. Please review.
Souvent22
Previous comments:
------------------------------------------------------------------------
Tue, 13 Sep 2005 15:47:46 +0000 : m3avrck
If I've created a node and attached a few files, when I goto edit the
node and attach another file, the file list disappears. However, after
the file is added, all files are still present. I'm guessing the hidden
iframe is being overwritten or similar? Problem tested and confirmed in
FF.
1
0