Allow admin to select Ajax or non-ajax behaviour per Bookmark type

Moving to Flag, where I have additional interest in this feature.

I'm actually thinking 3 display modes:

I wonder if the admin would like to show non-ajax links in one place, but ajax links in another. So perhaps this setting should be overridable in the themeing layer, but I don't yet see how this should be done. (For Views' 'Ops' field this should be easy: there's an options form).

Here's an initial pass at this. This adds display options for the link as described in #2. It saves the display options in the "options" database column. Things that are missing/I'm not sure of:

- We should probably provide "Flag confirmation message" and "Unflag confirmation message" as two additional fields. The current hard-coded message can have awkward phrasing at times.

- Is there a better way to reuse our form between the Flag configuration form and the Views configuration form? I'm fine with the way it is now, but it seems like there should be a cleaner way.

Looks good.

I haven't tested this yet.

(I wonder if we should add the "link_type" as a class on the link, or on the wrapper. Then, for example, some Javascript magic could open the confirmation box in a "thickbox" or whatever. BTW, I know you've been working on a "confirmation box" feature for Drupal's core.)

Do we want to commit this before beta4? I'd prefer not to, but you decide.

$flag->link_type = 'confirm';
return $flag->theme($is_flagged ? 'unflag' : 'flag', $content_id);

Maybe we should make theme() accept an $options array. This is a general problem I was aware of. I was thinking about a user-settable $flag->identifier, a la Panels, to determine further template "suggestions", but having $flag->identifier, or your $flag->link_type, preserved on the object and accidentally passed to the next renderings isn't very good. Or we could have a $flag->clone(), but...

'%flag_title' => $flag->get_title($content_id));

(BTW, that the 'title' can accept tokens is an undocumented feature. It's probably useless, but it's there.)

+ function get_content_title($comment) {
+ return $comment->title;

(I think comments have 'subject', not 'title'.)

But nothing prevents somebody from typing and using the non-confirmation URL, is that right?

So I think flag_page() should refuse to work if the flag is configured to use confirmation.

Similarly, Views' options_form() shouldn't show 'JavaScript toggle' and 'Normal link' if the flag is configured to use confirmation.

I think we should investigate ajax confirmation solutions right now to see what they entail. I'm not familiar with the jQuery pyrotechnics realm so I don't have suggestions at the moment.

This patch rerolls against the current Dev and adds the following changes:

- The default behavior of the Flag Ops field for Views is not to "inherit" from the link settings.
- Adds two fields for "flag confirmation message" and "unflag confirmation message". These fields are required if using the "Confirmation form" link display option.

I only have one remaining concern about this patch generally: should we provide a hook to let modules provide link types?

With this patch as it is now, modules could create their own link types by implementing hook_form_alter() and modifying the flag form. However, they'd also need to remember to form_alter() the views configuration for the Flag Ops. Finally, they'd implement hook_flag_preprocess() and set the $link_href value to whatever is necessary for their module.

My general inclination is that we should provide hook_flag_link_types(), but modules will still need to use the preprocess function to alter the link before it is themed. Thoughts?

some Javascript magic could open the confirmation box in a "thickbox" or whatever

(Panels 2 has a javascipt popup window. We should check it out, maybe we could "steal" its code.)

Here's a revised patch. After fiddling with the implementation, I didn't find providing a "url callback" each link type was that helpful. Instead this version introduces 2 hooks: hook_flag_link_types() and hook_flag_link().

As you can see, the implementation is pretty simple for a 3rd party module. Now I'm feeling really good about this patch. Anyone care to give it a review?

Looks good... almost ;)

Small error where you set '#default_value' => $this->link_type, should be '#default_value' => $flag->link_type,.

Attached revised patch.

I tested by installing a new d6 and enabling flag. I then modified and duplicated the default 'bookmark' flag resulting in the following flags, each one having a different link type:
http://img.skitch.com/20081114-1m33mp2qb26mrg1ssdpbn3rkmh.png

Upon clicking each it it performed with the desired effect.

Here's a minor update to D6, removing a call to flag_get_token() that wasn't used. This D5 port should be fully working too, might need another look though.

- The previous 'flag.tpl.php' was self-documenting with regard to the CSS classes. A designer could look at it and figure out the classes. Now that the classes are generated inside the module, we lose this property. I'm not saying we should change anything here (maybe we should, I don't know), just voicing my thoughts.

Yeah I'm not sure there's much way around this. We need the special classes so we can identify which links are AJAX and which ones are not. Generally just having a full set of classes is helpful. Hopeful having them in a single variable won't put off too many themers.

- Doesn't (!isset($link['html']) || $link['html'] != TRUE) equal empty($link['html'])? I know the former is considered "faster", but it's also less readable. (I'm only mentioning this because I saw a similar pattern in your "default flags" patch, and had to stop to "decipher" it ;-)

Heh, ultimately I ended up with something awfully similar to empty(). I'll switch this out, the code previously had been using opposite logic, and when I switched it I didn't realize it was almost identical to empty().

- $link['title'] should be check_plain()ed, I think.

Yep, I agree.

- The form in 'flag_handler_field_ops.inc' may trigger a PHP E_ALL warning, I think.

I'll turn back on E_ALL and give this a look.

- (Minor "coding style" issues you can disregard.) "They" now tell us to put a space on each side of a string concatenating dot, and I obey. You disobey, so we have two styles in the same function ;) But that's really a minor issue I even feel silly mentioning. Also, for the sake of uniformity, maybe we sould use only 'echo' in the 'tpl' (and it's faster, some say).

The spacing issue is funny. Since the space on either side of the dot is not "standard" until Drupal 7. The Drupal 5/6 standard is a single space on one side. Honestly though, maybe it's time for me to leave the past and switch over my code too. As for print/echo, core uses print so I think sticking with that would be preferable.

Sweet, thanks for this one guys. It'll eliminate the need I had when I created flag_form.module :D YAY!