September 2005 - Development - lists.drupal.org

[drupal-devel] [bug] Can't delete subjects with '&' in title
by Souvent22 13 Sep '05

13 Sep '05

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: Souvent22 -Status: patch (code needs review) +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/contact_full_2.patch (6.35 KB) patch re-roll due to head change. Souvent22 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.

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by m3avrck 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck Status: patch (code needs review) Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either.

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by Junyor 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: Junyor Status: patch (code needs review) FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. Junyor Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts?

1 0

[drupal-devel] [feature] Enable customizing of "Title" field on node forms
by fago 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/10056 Post a follow up: http://drupal.org/project/comments/add/10056 Project: Drupal Version: cvs Component: node system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Anonymous Updated by: fago Status: patch (ready to be committed) +1 for this functionality i've tested the patch and it looks nice, however node_validate_title() produces an validation error, where the field is called title, which isn't suitable if the modules changes the name, as the forum.module does. fago Previous comments: ------------------------------------------------------------------------ Sat, 14 Aug 2004 18:17:11 +0000 : Anonymous Attachment: http://drupal.org/files/issues/node.set-title-field.patch (2.02 KB) The default "title" field in a node object is useful but not always appropriate. For some node types it would be preferable to rename the field for display to users (e.g., from "title" to "name" for a record of an organization or business), while in others the title might be best drawn from a combination of other fields (e.g., first_name and last_name fields, for a record of an individual). This patch enables two new alternatives for node titles: renamingThe title field can be renamed, e.g., from "title" to "name". substutingOther fields can be substituted for the title field. The functionality is implemented through a new proposed node hook, _title_field($op). There are two possible values for $op: "action" (either "rename" or "substitute") and "names" (a list of field names to be used in the title field). Sample uses: [?PHP function example_title_field($node, $op) { switch ($op) { case 'action': $return = 'rename'; break; case 'names': $return = array('Name'); } return $return; } ?] In this case, since the requested action is "rename", the title field would be labelled "Name" (instead of "Title") in node add and edit forms for nodes of type "example". [?PHP function example_title_field($node, $op) { switch ($op) { case 'action': $return = 'substitute'; break; case 'names': $return = array('first_name', 'last_name'); } return $return; } ?] In this case, no "Title" field would be displayed in node add or node edit forms of node type "example". Instead, the fields "first_name" and "last_name" would be concatenated to form a title. Implementing this patch would significantly increase the flexibility of the node system, making it feasible to store information of many types not currently supported because they don't have a "title" attribute. ------------------------------------------------------------------------ Sun, 15 Aug 2004 16:39:37 +0000 : Dries This is best accomplished through the existing nodeapi hook, using a separate module. ------------------------------------------------------------------------ Sun, 15 Aug 2004 23:02:13 +0000 : nedjo Thanks for the suggestion. I did look for a way to accomplish this through _nodeapi (and other existing hooks), but couldn't see how. The "title" field is hard-coded into node.module. The _nodeapi hook allows various types of manipulations, but not, so far as I could see, changes to elements that have been generated by, e.g., form_textfield() calls. Are there possibilities I'm missing? Other approaches? Any specific suggestions as to means we could use to change the hard-coded "title" field? ------------------------------------------------------------------------ Thu, 19 Aug 2004 18:27:15 +0000 : nedjo Attachment: http://drupal.org/files/issues/node.set-title-field_0.patch (741 bytes) Drawing on Dries's suggestion, I've substantially revised this patch to enable node title field customization with only three additional lines of code in node.module. In place of the existing line in function node_form() <?php $output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE); ?> the patch substitutes a the following: <?php $title = variable_get('node_titlefield_'. $edit->type, 'Title'); if(!($title == 'none')) { $output .= form_textfield(t($title), 'title', $edit->title, 60, 128, NULL, NULL, TRUE); } ?> With this change in place, node modules will be able to override the standard node title label by setting a variable, node_titlefield_nodetype, where nodetype is the type of node. This could be done, e.g., in a _settings hook, so that the first time settings were accessed the module would set the variable: <?php if (variable_get('node_titlefield_example', '') == '') { // since none is already set, set this variable variable_set('node_titlefield_example', 'Name'); drupal_set_message('Example module initialization completed.'); } ?> The special value 'none' would suppress the title field altogether in node forms, used as follows: <?php if (variable_get('node_titlefield_example', '') == '') { // since none is already set, set this variable variable_set('node_titlefield_example', 'none'); drupal_set_message('Example module initialization completed.'); } ?> In this case, a _validate hook would be used to set the title field value. Assuming a module where a first name-last name combination was to be used for a title: <?php function example_validate(&$node) { if(!(isset($node->title))) { $node->title = $node->first_name . ' ' . node->last_name; } } ?> In short, this three-line addition to the node.module would significantly increase the flexibility of the node system by removing the hard-coded 'Title' field in node add and edit forms and instead enabling custom labelling or concatenation of the field from other sources. ------------------------------------------------------------------------ Thu, 19 Aug 2004 18:50:42 +0000 : moshe weitzman I like this functionality. But the implementation still seems unclean. I would prefer to simply remove the 'title' form field from node_form() and require that modules present this field in whatever way they please. in some cases, they will use a hidden form field because no customer interaction is required. This causes a break with backward compatibility, but cleanliness is facored over compatibilityness (when you must choose). I'm moving this back to 'Active', since I don't think this patch is committable as is. ------------------------------------------------------------------------ Fri, 27 Aug 2004 23:46:45 +0000 : nedjo Attachment: http://drupal.org/files/issues/node.set-title-field_1.patch (830 bytes) Thanks for looking at this. The suggestion - dropping the title field altogether and leaving this for module authors - has the advantage of simplicity. However, this change would imply significant new work as it would would require all or nearly all existing modules - core and contributed - be modified. While I think enabling title field customization is important for Drupal as a whole, and while this is a change I need for two modules I'm working on, I'm not immediately convinced that the benefits of this change justify this cost. Also, in most cases, the present title field works fine, so keeping it as a default option would I feel be desirable. Hence the following patch. In place of my (poorly conceived) initial hook proposals, above, this patch introduces a single very straightforward new node hook, _title_field. By setting this hook to return FALSE, node modules authors will suppress the default title field, and be free therefore to substitute what they wish or need. Returning TRUE, or not using the hook, will result in the default title field being used. Sample usage: <?php function example_title_field() { return FALSE; } ?> This is, I feel, a much better solution than the previous two I suggested--proof, if it's true, that the Drupal issue system works! ------------------------------------------------------------------------ Mon, 06 Sep 2004 15:50:38 +0000 : nedjo Are there any objections to or issues raised by this small patch (introducing a new hook to make the title field optional in node forms)? If not, can it be applied? ------------------------------------------------------------------------ Tue, 07 Sep 2004 13:46:29 +0000 : Anonymous I think I agree with Moshe's opinion that this should be accomplished by having node modules display the title field. In any event, this is a new feature and shouldn't go in during the feature freeze for 4.5. ------------------------------------------------------------------------ Tue, 07 Sep 2004 13:58:34 +0000 : ccourtne +1 for just moving the title field to be a responsibility of the implementing module instead of node_form. There is already to some extent "hook overload", in addition there may be several modules which don't want to even display a title module. Why add that only fixes some of the problems just to have to remove it later when you make a fix that address the whole problem. ------------------------------------------------------------------------ Tue, 07 Sep 2004 21:09:53 +0000 : nedjo Attachment: http://drupal.org/files/issues/node.remove-title-field.patch (636 bytes) Thanks for comments. As per feedback, the attached patch removes the title field from node forms, leaving this instead to node modules. Understood that possible approval of this patch this will need to wait until code unfrozen. ------------------------------------------------------------------------ Sun, 24 Oct 2004 12:06:46 +0000 : moshe weitzman please patch all core node modules as well. ------------------------------------------------------------------------ Sun, 28 Nov 2004 01:00:31 +0000 : nedjo Attachment: http://drupal.org/files/issues/node-move-form-title-field.patch (4.29 KB) Good point Moshe. This patch of node.module plus all core node modules simply moves the title field out of node.module and into the node modules, enabling (if desired) customization of the title field by a module. Patches to contributed modules will in most cases be as simple as adding the line <?php $output = form_textfield(t('Subject'), 'title', $node->title, 60, 128, NULL, NULL, TRUE); ?> at the beginning of a typename_form() function (and, if necessary changing an existing $output = to $output .=). In the attached patch, I've customized the title field for only one of the node types, calling the title of a forum topic the "Subject". We could consider calling the title of a blog a "Subject" or "Topic" as well. I feel this is a good solution (thanks for the feedback and discussion) and, for minimal work (minor updates to contributed modules), will have the significant benefit of enabling flexibility in content types beyond those that have "title" attributes. ------------------------------------------------------------------------ Sun, 28 Nov 2004 17:45:35 +0000 : nedjo Ability to customize title as provided in this patch was requested here [1]: "When adding/editing, the 'title' field is manditory, which is fine. Except that its label is hardcoded to 'title'. A custom node modelling, say, a 'Company' would much rather this field be presented to the user as 'Company Name'. " [1] http://drupal.org/node/13596 ------------------------------------------------------------------------ Fri, 04 Mar 2005 03:44:55 +0000 : nedjo Here's another patch that's sat for months without being either applied or turned down, although there was expressed support and, after several iterations, it addressed all issues that had been raised. The point is to remove the hard-coded "Title" field - inapprioriate for many node types, e.g., companies - from node forms, instead allowing node modules to treat this as appropriate (e.g., a "Subject", "Name" or "Topic", or nothing at all). Are there problems with this approach that were not raised? Is there a reason this wasn't accepted? ------------------------------------------------------------------------ Fri, 04 Mar 2005 03:54:02 +0000 : Steven nedjo: if you read the backlog you can see that this feature was delayed for after the 4.6 release. "I've customized the title field for only one of the node types, calling the title of a forum topic the "Subject". " This is really inconsistent, as what you call the title of a blog, story or forum node is purely subjective. "Subject" "Topic" and "Title" are pretty much interchageable and any subtle differences will surely be eroded by translation. The only thing I can agree on so far is naming a polls' title "Question". ------------------------------------------------------------------------ Fri, 04 Mar 2005 05:15:03 +0000 : nedjo Thanks for the note. I'm not sure what you mean by the "backlog". Where would I find this? I agree that a "Question" field would make sense for polls. ------------------------------------------------------------------------ Fri, 04 Mar 2005 14:12:56 +0000 : JonBob Actually UnConeD, it was postponed until after the 4.5 release, not 4.6, and looks like everyone forgot about it when the release happened. And no, Nedjo, I don't take this as proof that Drupal needs more bureaucracy. :-) Marking this active again; I'm sure the patch will have to be updated once the code branches. We had rolled this function into the needs for CCK, but in our discussions split it off again. ------------------------------------------------------------------------ Sun, 03 Apr 2005 05:28:08 +0000 : chx Attachment: http://drupal.org/files/issues/node-move-form-title-field_0.patch (4.59 KB) The patch applies to ucrrent HEAD with some offset. I have rerolled it so it applies cleanly. ------------------------------------------------------------------------ Fri, 20 May 2005 10:09:50 +0000 : chx Patch still applies (yes, with some offset, but applies). ANd yes, we need this very badly. ------------------------------------------------------------------------ Fri, 20 May 2005 10:18:49 +0000 : Thox I'd benefit greatly from having a feature like this. The example already given is one of the cases I need it: I'd like the title field to be a concatenation of two fields: "First name" and "Last name". ------------------------------------------------------------------------ Tue, 24 May 2005 03:39:40 +0000 : gsperk I wonder if someone could help me with the following problem. After applying the patch I made the output for 'title' in my own_node.module this: $output .= form_hidden('title', $node->firstname . ' ' . $node->surname); When the form is first submitted I get the error 'You have to specify a title'. When I submit again the firstname/surname variables are obviously in $node, and the form submits with the desired title. So, I suppose my question is, how can get the values for firstname and surname into $node without submitting twice? (If it's not obvious I can confirm that I dont have much experience with this kind of stuff. Any suggestions would be a big help.) ------------------------------------------------------------------------ Wed, 25 May 2005 04:23:41 +0000 : Steven To make this patch work you also need to move the title validation out of node.module and into the node modules. I don't think this is that much of a problem, it is code that is not going to be touched much. Also, in light of the upcoming CCK it makes sense to do it like this. ------------------------------------------------------------------------ Sat, 02 Jul 2005 17:02:26 +0000 : hanoii Nice thread. I have come up with a slightly different approach I think that would need less patching/updating of other modules. Drupal 4.6.2 node.module:1275 <?php // Get the node-specific bits. // We can't use node_invoke() because $param must be passed by reference. $function = node_get_module_name($edit) .'_form'; $param = array(); if (function_exists($function)) { $form .= $function($edit, $param, $title); } ?> Notice the $title param on the hook_form invoke. This was what I added. As that hook is called like this an not with the node_invoke() I can define on my custom node module, or patch any module I need to modify the title description with the following hook: <?php function budget_form(&$node, &$params, &$title) { ?> and modify the $title argument there, as it is by reference, It will me modified on node.module. Then again on node.module:1319 I added/changed the following: <?php if ( ! $title ) { $title = t('Title') ; } $output .= form_textfield($title, 'title', $edit->title, 60, 128, NULL, NULL, TRUE); ?> So, if the module modify the title var, it uses that, if not, it uses the default one. Worked for me so far, nice to post on drupal. Greetings, a.= ------------------------------------------------------------------------ Sat, 02 Jul 2005 18:10:28 +0000 : hanoii I posted too soon the thing before... :) I did further patches because of the validation keep telling that I have tu put a "title". I was happy with the previous patch, not so much with this one, but I let you know what I did... on the node.module, node_validate() function I move the title validation block just after the <?php // Do node-type-specific validation checks. node_invoke($node, 'validate'); node_invoke_nodeapi($node, 'validate'); ?> and added another parameter to the form_set_error() <?php // Validate the title field. if (isset($node->title)) { if (trim($node->title) == '') { form_set_error('title', t('You have to specify a title.'), TRUE); } } ?> then, on the form_set_error function in common.inc I did the following: <?php /** * File an error against the form element with the specified name. */ function form_set_error($name, $message, $checkexistance = FALSE ) { if ( !$checkexistance || $checkexistance && !isset($GLOBALS['form'][$name]) ) { $GLOBALS['form'][$name] = $message; drupal_set_message($message, 'error'); } } ?> So now, if any previous title error was added, and the $checkexistance flag is TRUE, no error will be added to the message queue. The moving place of the validation block was to let the modules validate first than the node module. Errrr.. I don't like it, but again, it worked :) a.= ------------------------------------------------------------------------ Mon, 01 Aug 2005 00:54:22 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/title_0.patch (4.56 KB) I've updated the patch to apply to current cvs. As progress on the CCK has been rather slow, I'd appreciate if somebody could address Steven's concerns and the patch could be applied. ------------------------------------------------------------------------ Tue, 16 Aug 2005 19:19:23 +0000 : Thox Attached is a patch with an attempt to move the title validation code into the node modules (from node.module). ------------------------------------------------------------------------ Tue, 16 Aug 2005 19:20:21 +0000 : Thox Attachment: http://drupal.org/files/issues/title-validation.patch (6.22 KB) attached again (issue preview seems busted?) ------------------------------------------------------------------------ Wed, 24 Aug 2005 18:57:26 +0000 : nedjo Thanks chx, killes, and Thox for updates and fixes on the patch. I've applied and tested it and Thox's changes seem to address the title validation issue. It's a couple of releases later--it would be great to see this fix applied! ------------------------------------------------------------------------ Wed, 24 Aug 2005 23:17:25 +0000 : mathias Attachment: http://drupal.org/files/issues/title-validation_0.patch (6.04 KB) +1 I've needed to change the name of the title field or even altogether hide it for custom modules. Issue preview was working for me. We should document for module developers the implications title-less nodes (e.g., less informative watchdog entries, harder to edit nodes, node title lists become buggy, etc). I couldn't get the patch to apply cleanly to HEAD so here's a new one. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:42:30 +0000 : Robrecht Jacques Attachment: http://drupal.org/files/issues/title-validation_1.patch (6.23 KB) +1 I need this too: I want to be able to set the title of a node automatically. Patch still applies with offset (rerolled patch attached).

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by m3avrck 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck Status: patch (code needs review) Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete.

1 0

[drupal-devel] [task] Improvments to Filter UI
by Bèr Kessels 13 Sep '05

13 Sep '05

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: Bèr Kessels -Status: patch (code needs work) +Status: patch (ready to be committed) 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? Bèr Kessels 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?

1 0

[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by Bèr Kessels 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/24804 Post a follow up: http://drupal.org/project/comments/add/24804 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: chx Reported by: moshe weitzman Updated by: Bèr Kessels Status: patch (code needs review) Discussion about a patch should go further then just comments on the code. NSK (and I) just did what the patch review guidelines say under "Challenge the proposed changes.". Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman Attached please find a demonstration patch which moves comment display to its own tab on the node view page. Admin may choose this new view or old view. The patch needs work still: - comment control form redirects to the node page after submit. Should be comment page - same for comment posting form - probably more I hope someone gets inspired to finish this patch. I won't revisit this for a while. ------------------------------------------------------------------------ Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB) with attachment this time ------------------------------------------------------------------------ Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman oh yeah - also on the TODO list is to comment URLs to always point to comment.module and never to node/x#comment-cid or node/x#comment-new and so on. then comment.module can work out whether to show you the discuss tab or the traditional view. ------------------------------------------------------------------------ Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann I like the concept. Would want this to be part of workflow settings, so that comment-tab could be enabled/disabled per content type. ------------------------------------------------------------------------ Sun, 12 Jun 2005 02:20:42 +0000 : Jaza I like the idea - big +1. This is a must for sites that have long articles and/or many comments - viewing the node is enough bandwidth as it is, without all the comments getting displayed as well. Also improves usability, IMO. Would also like to see pagination of the comments tab (I'm sure you've thought of this already, Moshe). Something similar to A List Apart [1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments). Or, if the existing pager system demands it, node/x/discuss?page=2. [1] http://www.alistapart.com/ ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:41:42 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB) Let's discuss! Like do we want to have a reply form on the node display even if the comments are on the discuss page? ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:50:13 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB) I have uploaded moshe's patch. Let's try again. ------------------------------------------------------------------------ Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels I like this approach a lot. However, as i use drupal also for blogging, would dislike my bog to behave the same. Thus there must be some -easy- way to get around this. µ The problem lies mainly in the fact that tabs are not administratable in Drupals menu admin. And I think it is a very bad idea to add a config option where to place the comments. We should go for one way, teh default way, and offer a menu-admin of some sort to change this, for bloggers and slashdot alikes. How to achieve this? I see two options, but I really hope people see more then one.: - make local-tasks appear in the menu admin (gets a huge +1 from me) It's a separate issue, though. - add a config option to the comment admin to allow comments either as tab or as thread under the post. IMO that config option should not set a variable, that gets checked every hook_menu call, but it should alter the menu-entry in the database for the tabs Its possible, I did that before. But, as said, I hope others have better ideas. for I really would love this feature to get in. But I am afraid a huge lot of people will move away from drupal if we allow tabs only. ------------------------------------------------------------------------ Tue, 30 Aug 2005 14:05:59 +0000 : chx No, not tabs only. I know the drupal_gotos are giving you this impression, but will fix in the evening. Also, there is only a variable_get for node/nid pages and then again, variable_get is double cached, it's very fast. ------------------------------------------------------------------------ Tue, 30 Aug 2005 16:19:22 +0000 : Bèr Kessels I know variable is fast, but I was referring to: && variable_get('comment_task', FALSE) that is evaluated every node view. As well as the setting in the comment options. I am no fan of such options; However, it seems in this case that setting is sortof required, unfortunately. So unless others come up with better ideas, just ignore my previous comment/post. Another idea: put this in hook_nodeapi() $op == "settings". that way this setting can be used per type. I can imagine that this makes no sense for forums and blog entries, yet is very usefull for the books on that same site. ------------------------------------------------------------------------ Tue, 30 Aug 2005 16:20:36 +0000 : Bèr Kessels sorry, with "Another idea: put this in..." I meant to say "Another idea: put the settings for tabs vs threads ..." ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:24:22 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_2.patch (6.71 KB) Comment URL is introduced. Instead of node/$nid#comment-cid we have comment/$nid#comment-cid Other minor fixes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 00:31:31 +0000 : Boris Mann Comment URLs are great, except I'd love to have a *full* comment URL -- e.g. comment/$nid/$cid. The probably with anchor tags is that they are not visible at the server level -- i.e. not tracked in referrers, etc. etc. Don't want to hijack this issue, so feel free to ignore for now, but wanted to bring up the issue from a search/web traffic/etc. perspective. ------------------------------------------------------------------------ Tue, 13 Sep 2005 05:51:09 +0000 : nedjo +1 in principle. (I've looked over but not applied the code.) (Optional) tabbed display is a useful enhancement and justifies the extra configuration option. I also like the new comment URL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 05:59:36 +0000 : nsk As a user I have no problem with this as long as I can disable it and use the old view. Please note that there is nothing special or innovative in Wikipedia's use of the comment page (talk page), it's just a feature of MediaWiki, the software they use. Many other sites use the same software. IMO it's bad but others may need, so it's probably good to add something similar in Drupal as long as it's not the default. ------------------------------------------------------------------------ Tue, 13 Sep 2005 06:06:20 +0000 : moshe weitzman @nsk (and others) - please do not post simple "like/dislike" comments when an issue is in 'rpatch: code needs review' status. the review guidelines are at http://drupal.org/patch/review. thank you.

1 0

[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by moshe weitzman 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/24804 Post a follow up: http://drupal.org/project/comments/add/24804 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: chx Reported by: moshe weitzman Updated by: moshe weitzman Status: patch (code needs review) @nsk (and others) - please do not post simple "like/dislike" comments when an issue is in 'rpatch: code needs review' status. the review guidelines are at http://drupal.org/patch/review. thank you. moshe weitzman Previous comments: ------------------------------------------------------------------------ Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman Attached please find a demonstration patch which moves comment display to its own tab on the node view page. Admin may choose this new view or old view. The patch needs work still: - comment control form redirects to the node page after submit. Should be comment page - same for comment posting form - probably more I hope someone gets inspired to finish this patch. I won't revisit this for a while. ------------------------------------------------------------------------ Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB) with attachment this time ------------------------------------------------------------------------ Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman oh yeah - also on the TODO list is to comment URLs to always point to comment.module and never to node/x#comment-cid or node/x#comment-new and so on. then comment.module can work out whether to show you the discuss tab or the traditional view. ------------------------------------------------------------------------ Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann I like the concept. Would want this to be part of workflow settings, so that comment-tab could be enabled/disabled per content type. ------------------------------------------------------------------------ Sun, 12 Jun 2005 02:20:42 +0000 : Jaza I like the idea - big +1. This is a must for sites that have long articles and/or many comments - viewing the node is enough bandwidth as it is, without all the comments getting displayed as well. Also improves usability, IMO. Would also like to see pagination of the comments tab (I'm sure you've thought of this already, Moshe). Something similar to A List Apart [1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments). Or, if the existing pager system demands it, node/x/discuss?page=2. [1] http://www.alistapart.com/ ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:41:42 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB) Let's discuss! Like do we want to have a reply form on the node display even if the comments are on the discuss page? ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:50:13 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB) I have uploaded moshe's patch. Let's try again. ------------------------------------------------------------------------ Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels I like this approach a lot. However, as i use drupal also for blogging, would dislike my bog to behave the same. Thus there must be some -easy- way to get around this. µ The problem lies mainly in the fact that tabs are not administratable in Drupals menu admin. And I think it is a very bad idea to add a config option where to place the comments. We should go for one way, teh default way, and offer a menu-admin of some sort to change this, for bloggers and slashdot alikes. How to achieve this? I see two options, but I really hope people see more then one.: - make local-tasks appear in the menu admin (gets a huge +1 from me) It's a separate issue, though. - add a config option to the comment admin to allow comments either as tab or as thread under the post. IMO that config option should not set a variable, that gets checked every hook_menu call, but it should alter the menu-entry in the database for the tabs Its possible, I did that before. But, as said, I hope others have better ideas. for I really would love this feature to get in. But I am afraid a huge lot of people will move away from drupal if we allow tabs only. ------------------------------------------------------------------------ Tue, 30 Aug 2005 14:05:59 +0000 : chx No, not tabs only. I know the drupal_gotos are giving you this impression, but will fix in the evening. Also, there is only a variable_get for node/nid pages and then again, variable_get is double cached, it's very fast. ------------------------------------------------------------------------ Tue, 30 Aug 2005 16:19:22 +0000 : Bèr Kessels I know variable is fast, but I was referring to: && variable_get('comment_task', FALSE) that is evaluated every node view. As well as the setting in the comment options. I am no fan of such options; However, it seems in this case that setting is sortof required, unfortunately. So unless others come up with better ideas, just ignore my previous comment/post. Another idea: put this in hook_nodeapi() $op == "settings". that way this setting can be used per type. I can imagine that this makes no sense for forums and blog entries, yet is very usefull for the books on that same site. ------------------------------------------------------------------------ Tue, 30 Aug 2005 16:20:36 +0000 : Bèr Kessels sorry, with "Another idea: put this in..." I meant to say "Another idea: put the settings for tabs vs threads ..." ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:24:22 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_2.patch (6.71 KB) Comment URL is introduced. Instead of node/$nid#comment-cid we have comment/$nid#comment-cid Other minor fixes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 00:31:31 +0000 : Boris Mann Comment URLs are great, except I'd love to have a *full* comment URL -- e.g. comment/$nid/$cid. The probably with anchor tags is that they are not visible at the server level -- i.e. not tracked in referrers, etc. etc. Don't want to hijack this issue, so feel free to ignore for now, but wanted to bring up the issue from a search/web traffic/etc. perspective. ------------------------------------------------------------------------ Tue, 13 Sep 2005 05:51:09 +0000 : nedjo +1 in principle. (I've looked over but not applied the code.) (Optional) tabbed display is a useful enhancement and justifies the extra configuration option. I also like the new comment URL. ------------------------------------------------------------------------ Tue, 13 Sep 2005 05:59:36 +0000 : nsk As a user I have no problem with this as long as I can disable it and use the old view. Please note that there is nothing special or innovative in Wikipedia's use of the comment page (talk page), it's just a feature of MediaWiki, the software they use. Many other sites use the same software. IMO it's bad but others may need, so it's probably good to add something similar in Drupal as long as it's not the default.

1 0

[drupal-devel] DB layer skips
by Karoly Negyesi 13 Sep '05

13 Sep '05

Hi! auction module uses mysql_fetch_array download module uses mysql_escape_string media module uses mysql_escape_string recipe module uses mysql_insert_id Given the age and useability of auction and download I suggest their removal. Regards NK

1 0

[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by nsk 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/24804 Post a follow up: http://drupal.org/project/comments/add/24804 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: chx Reported by: moshe weitzman Updated by: nsk Status: patch (code needs review) As a user I have no problem with this as long as I can disable it and use the old view. Please note that there is nothing special or innovative in Wikipedia's use of the comment page (talk page), it's just a feature of MediaWiki, the software they use. Many other sites use the same software. IMO it's bad but others may need, so it's probably good to add something similar in Drupal as long as it's not the default. nsk Previous comments: ------------------------------------------------------------------------ Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman Attached please find a demonstration patch which moves comment display to its own tab on the node view page. Admin may choose this new view or old view. The patch needs work still: - comment control form redirects to the node page after submit. Should be comment page - same for comment posting form - probably more I hope someone gets inspired to finish this patch. I won't revisit this for a while. ------------------------------------------------------------------------ Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB) with attachment this time ------------------------------------------------------------------------ Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman oh yeah - also on the TODO list is to comment URLs to always point to comment.module and never to node/x#comment-cid or node/x#comment-new and so on. then comment.module can work out whether to show you the discuss tab or the traditional view. ------------------------------------------------------------------------ Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann I like the concept. Would want this to be part of workflow settings, so that comment-tab could be enabled/disabled per content type. ------------------------------------------------------------------------ Sun, 12 Jun 2005 02:20:42 +0000 : Jaza I like the idea - big +1. This is a must for sites that have long articles and/or many comments - viewing the node is enough bandwidth as it is, without all the comments getting displayed as well. Also improves usability, IMO. Would also like to see pagination of the comments tab (I'm sure you've thought of this already, Moshe). Something similar to A List Apart [1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments). Or, if the existing pager system demands it, node/x/discuss?page=2. [1] http://www.alistapart.com/ ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:41:42 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB) Let's discuss! Like do we want to have a reply form on the node display even if the comments are on the discuss page? ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:50:13 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB) I have uploaded moshe's patch. Let's try again. ------------------------------------------------------------------------ Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels I like this approach a lot. However, as i use drupal also for blogging, would dislike my bog to behave the same. Thus there must be some -easy- way to get around this. µ The problem lies mainly in the fact that tabs are not administratable in Drupals menu admin. And I think it is a very bad idea to add a config option where to place the comments. We should go for one way, teh default way, and offer a menu-admin of some sort to change this, for bloggers and slashdot alikes. How to achieve this? I see two options, but I really hope people see more then one.: - make local-tasks appear in the menu admin (gets a huge +1 from me) It's a separate issue, though. - add a config option to the comment admin to allow comments either as tab or as thread under the post. IMO that config option should not set a variable, that gets checked every hook_menu call, but it should alter the menu-entry in the database for the tabs Its possible, I did that before. But, as said, I hope others have better ideas. for I really would love this feature to get in. But I am afraid a huge lot of people will move away from drupal if we allow tabs only. ------------------------------------------------------------------------ Tue, 30 Aug 2005 14:05:59 +0000 : chx No, not tabs only. I know the drupal_gotos are giving you this impression, but will fix in the evening. Also, there is only a variable_get for node/nid pages and then again, variable_get is double cached, it's very fast. ------------------------------------------------------------------------ Tue, 30 Aug 2005 16:19:22 +0000 : Bèr Kessels I know variable is fast, but I was referring to: && variable_get('comment_task', FALSE) that is evaluated every node view. As well as the setting in the comment options. I am no fan of such options; However, it seems in this case that setting is sortof required, unfortunately. So unless others come up with better ideas, just ignore my previous comment/post. Another idea: put this in hook_nodeapi() $op == "settings". that way this setting can be used per type. I can imagine that this makes no sense for forums and blog entries, yet is very usefull for the books on that same site. ------------------------------------------------------------------------ Tue, 30 Aug 2005 16:20:36 +0000 : Bèr Kessels sorry, with "Another idea: put this in..." I meant to say "Another idea: put the settings for tabs vs threads ..." ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:24:22 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_2.patch (6.71 KB) Comment URL is introduced. Instead of node/$nid#comment-cid we have comment/$nid#comment-cid Other minor fixes. ------------------------------------------------------------------------ Tue, 13 Sep 2005 00:31:31 +0000 : Boris Mann Comment URLs are great, except I'd love to have a *full* comment URL -- e.g. comment/$nid/$cid. The probably with anchor tags is that they are not visible at the server level -- i.e. not tracked in referrers, etc. etc. Don't want to hijack this issue, so feel free to ignore for now, but wanted to bring up the issue from a search/web traffic/etc. perspective. ------------------------------------------------------------------------ Tue, 13 Sep 2005 05:51:09 +0000 : nedjo +1 in principle. (I've looked over but not applied the code.) (Optional) tabbed display is a useful enhancement and justifies the extra configuration option. I also like the new comment URL.

1 0