Allow disabling comment preview + unify with node preview settings

Not a bad idea, but the patch needs code style work, especially on code comments. I'd also look at some examples of other form field descriptions and follow that style rather than the one currently in the patch, to help maintain consistency.

Also, I would call this a feature request for Drupal 7, especially as Drupal 6 is feature- and string-frozen. I don't have any reason to think it "minor", though. :)

Created a patch to address this issue applies cleanly against D7 unstable-5. Please review.

I found that the preview option became useless once installing a rich text editor (such as TinyMCE) - at least for previewing formatting.

This is a valid concern but can we think of something more elegant than adding a weird third option ("hidden? Why would I want to do that?"). From reading the patch I don't see any explanation on why an admin/user would want to choose the 'hidden' option.

Adding options just means more actual work for the user we should try not to do that here, it's a bit of a cop-out.
Can we think of something that would "automatically hide if RTE is enabled" ?

I actually just find the preview option annoying. I dont use a RTE but i disable the preview button so i can hit return on a form value to submit the form instead of preview it. This third option is getting rolled into a "per" content type setting as well so it can be used on content types where "preview" doesnt make much sense like a "link" for a "link directory", etc.

I generally remove the preview button manually with hook_form_alter, per node-type, if I have a site where the client doesn't want it showing for certain edit forms. I think that this is a better way to do it than adding more configuration clutter, because it's easy for any developer to do (or even for any themer), and because it avoids more confusion for site admins.

@yoroy: I agree with #3, adding more options is a cop-out. For site builders who really want to remove the preview button regardless of RTE use, call unset($form['preview']) in a form_alter implementation.

For the RTE issue, maybe we should consider that the problem is not "preview is broken with RTE, therefore we should remove the preview button from forms with RTE", but rather "preview is broken with RTE, therefore we should fix preview with RTE".

+1 I would support the ability to hide the preview buttons from within the interface. I have a number of clients who use Drupal but can't code and this is often a request I get. As Jaza said I also use hook_form_alter but I'd like to see this available for non-coders.

I dont understand the reclassification to "needs work". A number of people have requested the feature and it doesn't add a new configuration it only adds a new value to an existing configuration. If doesn't even have to change the form element type because we are already using a radio button on a binary option for some reason. I appreciate that you might not agree with the direction but that doesn't mean it needs work. Reverting to needs review.

How does one go about inserting this patch, where? Will it work for Drupal 6?

I think it should be

disabled = 0
optional = 1
required = 2

the patch seems to make disabled = 2. We could also make these constants instead of magic numbers.

I'm split on whether adding the option is a good idea or not, both supporters and detractors make good points.

Wouldn't hurt to have a test as well.

We want to move this configuration option to content-type settings, "Publishing" fieldset.

Disabling previews globally makes no sense. Equally, I can't count the number of times I had to form_alter away the preview button for certain content-types only, while leaving it required or optional for all other contents.

Ok new patch applies cleanly to head and:
* Ditches old comment specific constants for OPTIONAL/REQUIRED replaces them with the new DRUPAL_* variants.
* Adds disbaled option for comment preview. This is per content type and simply adding a radio option to existing comment config under content types.
* Preserves existing comment preview settings by transcoding the old values to the new ones in an update
* Cleans up 1 pre-existing line of bad whitespace

And this one even passes the tests. ;)

First of all, this somehow applies to comments only. I thought the OP was about nodes, but we probably want to improve both.

I didn't even know of those new constants, but they are awesome! D7, I like. :)

-    'comment_preview' => COMMENT_PREVIEW_REQUIRED,
+    'comment_preview' => 1, // WAS: deleted constant COMMENT_PREVIEW_REQUIRED = 1.

Should be DRUPAL_REQUIRED instead of 1, I guess. In any case, please remove that added comment.

 }
+/**

Missing blank line.

+  $type_list = node_type_get_types();
...
+  foreach ($type_list as $type => $object) {

You can shortcut that and simply use foreach (node_type_get_types() as ...).

+  /**
+   * Upgrade variable on all types.
+   * 0 used to equal optional
+   * 1 used to equal required
+   */

That's the wrong syntax for inline comments. Multiline comments are only used for PHPDoc. Comment lines that form a list should use - as prefix. Missing trailing periods. Overall, the update comments don't explain anything that wouldn't be visible by looking at the code. We want to explain the non-trivial instead. Why? How? Where?

+    $original_preview = variable_get('comment_preview_' . $type, DRUPAL_REQUIRED);
+    $original_preview ? $preview = DRUPAL_REQUIRED : $preview = DRUPAL_OPTIONAL;

That's a very strange syntax. Please use a proper condition to assign the target value:

// There were only two comment modes in the past:
// - 1 was 'required' previously, convert into 2 (DRUPAL_REQUIRED).
// - 0 was 'optional' previously, convert into 1 (DRUPAL_OPTIONAL).
$old_mode = variable_get('comment_preview_' . $type, 1);
if ($old_mode) {
  $new_mode = DRUPAL_REQUIRED;
}
else (
  $new_mode = DRUPAL_OPTIONAL;
}
variable_set('comment_preview_' . $type, $new_mode);

.

     $form['comment']['comment_preview'] = array(
       '#type' => 'radios',

I'm not sure whether we shouldn't use a select for this setting now.

+  // Show if not specifically disabled in content type admin.
+  if (variable_get('comment_preview_' . $node->type, DRUPAL_REQUIRED) != DRUPAL_DISABLED) {

Either leave out this comment or explain something non-obvious.

   }
-  
+
 }

That white-space fix is a good catch, but there shouldn't be a blank line here at all.

@sun, thx for the thorough review.

-    'comment_preview' => COMMENT_PREVIEW_REQUIRED,
+    'comment_preview' => 1, // WAS: deleted constant COMMENT_PREVIEW_REQUIRED = 1.

DRUPAL_REQUIRED = 2 and COMMENT_PREVIEW_REQUIRED = 1, I used the INT because we need the update to function like the OLD constant not the new one so that our later update will update them all uniformly. Suggestion? I'll fix the rest up this afternoon. I agree about the Select instead of radios, whitespace (IDE auto trims blank lines with spaces up to me to actually remove them :),

Ok all issues above addressed. I used an INT for the deleted constant as that was its value anyway. Seemed most appropriate. I would also like to convert a couple other 3+ option radios into selects on this page but they shuld probably be in other issues. anyone else agree though (e.g. Default comment setting: Hidden, Open, Closed).

Regarding the "disable *node* preview" issue, i have already addressed this in a previous patch and it has been committed to core (#361277). This one addresses the comment version of the same. I'll be creating a new issue to deal with the usability concerns of this page (e.g. Selects instead of radios for longer lists of simple options, etc.)

Code style looks fine, nice that we have those DRUPAL_ constants now, and changing from radio to select makes sense as well. If we have this for nodes already, makes sense to do it for comments, and while it's an extra admin option, it does let people easily reduce stuff on the comment / node forms.

1. + } else { should be 2 lines

2. It looks like we need to update setCommentPreview() to take an int instead of a boolean -- we can pass in the defines.

3. I'd like to add a new test for this to comment.test. There are already some tests for comment previewing but we haven't updated or extended those.

Dare I ask for a screenshot? From reading the comments this sounds all good, but you know…

That'll teach me to review patches before tea. Looks like yoroy crossposted.

I thought someone else would mention this in a review, but well...

Please also (re-)add this bit:

// There were only two comment modes in the past:
// - 1 was 'required' previously, convert into DRUPAL_REQUIRED (2).
// - 0 was 'optional' previously, convert into DRUPAL_OPTIONAL (1).
$old_mode = variable_get('comment_preview_' . $type, 1);

Without that information, no one will understand that update function easily. The default value for variable_get() should use the old integer value, not the new DRUPAL_OPTIONAL, because the value will be converted to the new constants afterwards. Using the constant for variable_get() would mean that we convert from current constants to current constants.

Ok this patch builds on my last one and modifies the tests as Dries requested. In summary I:
* Updated setCommentPreview to accept the new DRUPAL_* constants instead of BOOL.
* Consolidated the assertions of *subject* and *preview/save* button field presence into the PostComment() helper function.
* Preview modesettings variable is now checked dynamically in the postComment() function and has been removed from its signature and uses throughout the testfile.
* Transposed order of subject and comment arguments in postComment() signature to reflect its optional nature. Even though this is slightly less intuitive it saves alot of passing of empty strings. Updated throughout test to reflect changes. I can pull this out but it seemed to make sense while i was cleaning it up anyway.
* Need to replace the CommentSubject INTs with the new DRUPAL_* constants but ill leave that for another patch becuase it adds another update too. i plan on finding these throughout core and consolidating them all into 1 patch anyway.
* I think the way the postComment() auto drupalGets the node if specified is pretty hacky and should probably be either broadened to support comments or removed from the function but I left it as is to save a few kittens.

Daniel and i crossposted, here is the same patch with his suggested improvement.

I scanned this patch and it looks much better. A more in-depth review is in order though.

While breaking #521468: Redesign content type editing (after CCK UI is in) into smaller chunks, I was about to create an issue "Unify node and comment preview settings" to reduce mental processing for very similar setting in different context and provide more consistency. I even created a mockup for this (attached). Luckily here is already work on this going on, so let's not create dupes.

This patch does not apply any more -- possibly because of comment settings simplification but it is still relevant.

Mixing to proposals:

1. Add "Disabled" option for comment preview settings, provide upgrade path
2. Convert node and comment preview setting form elements into selectboxes as shown in mockup

Sub, was just about to post an issue for this. Looks good.

Wait - this is still not in? Was RTBC a few times.... Tagging. Those DRUPAL_* constants would be really, really useful for contrib.

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Re-rolled against HEAD. Looks RTBC if bot passes.

Alrighty, slight confusion with those form button conditions.

Interestingly, those preview & submit button #access conditions are now *identical* to those in node_form(). I copied those and replaced 'node' with 'comment'.

Which.... is kinda very interesting, because it basically allows to apply an automated form submit button handling, and with that, a consistent pattern.

Don't know if its bad form to RTBC a patch I originally wrote but after the investigation by sun and Dries and the subsequent passing of all of the tests by the bot i think we are in decent form here. -mf

uhm. yeah, let me fix the status for you :)

This isn't even remotely critical, but it is a nice clean-up from a UX and DX perspective.

Screenshots:

Node:

Comment (before):

Comment (after):

I know it wasn't introduced by this patch, but in the interest of cleaning up everything related to this here, DRUPAL_OPTIONAL / REQUIRED / etc. are absolutely *horrible* names for these constants. We're not making Drupal optional, we're making preview optional. Please include PREVIEW in the constant name.

Thanks for your work on this :) So exciting to see this simple usability improvement finally in core. Can't wait to click "Disabled" ;)

Not sure what "needs work" here. I think I already clarified in IRC that those constant names were carefully chosen when they have been introduced. It's true that we don't use them much yet (this is the second implementation next to Node module), but that does not dispute their generic purpose.

Indeed these those constant names were carefully chosen so that they can be used through out core. This was done in this issue #361277: Remove the 'post settings' admin screen and relocate contents. Pushing back to the testbot to make sure it applies before marking RTBC. I'd really like to get this simple usability improvement in.

Needed to increment the update function in comment install. I assume this was the invalid PHP syntax. would be nice to add the error line number/message to the testbot to make this easier in the future. Is there any reason left to hold this little guy up now? Happy to fix it today if so.

Looks good to me. Only thing i might suggest that i had earlier in this patch was implementing the new comment option as a "select" form item instead of "radios" to save space. If others think that it adds usability/discoverability and justifies the extra little clutter I'm all for it. The option is what is important to me.

This looks good now, and should have been committed a while ago. Committed it now! :)