Feature Proposal: Add +values to radio and checkbox formfields

Motivation

While formfields of type select have the +values feature to map the stored data to a different display name, radio and checkbox formfields should have that feature as well.

Description and Documentation

While at it I'd like to have the Tooltip description in the title html attribute of the input element so that they are displayed when hovering over them.

Somewhat related is to make all three

  • select+values
  • radio+values
  • checkbox+values
behave the same when displaying a mapped formfield value in a normal view.

Examples

  • DataForm is defineing a formfield of type select+values with allowed values as One=1, Two=2, Three=3
  • Rendering this formfield for editing displays a select box with options like
    <option value=1>One</option>
    <option value=2>Two</option>
    <option value=3>Three</option>
  • Selecting Two will store 2
  • Rendering this formfied for a normal view will display Two ... not 2 ...as reported as a related bug in Item10071.
Similarly for radios and checkboxs.

Impact

Affected core files:

  • Checkbox.pm
  • ListFieldDefinition.pm
  • Radio.pm
  • Select.pm
  • RenderFormTests.pm

Affected user experience:

There's a change the way a select+values formfield is displayed during view, as rendered by the form.*.tmpl templates. This has been like this forever, but changing it to be consistent will causes less confusion from then on.

It will also enhance $formfield() token on formatted searches - PH

Affected plugins:

When FlexFormPlugin renders a formfield value, it won't return the stored value but the mapped one as a matter of it being a pure frontend to the internal renderForDisplay() API of DataForms. That's intentional most of the time. For those wiki applications relying on the actual store data being returned, a new variable $origvalue in addition to the default $value. It might be a good idea to have this in the core instead of just inside FlexFormPlugin.

Implementation

Index: lib/Foswiki/Form/Checkbox.pm
===================================================================
--- lib/Foswiki/Form/Checkbox.pm   (revision 11867)
+++ lib/Foswiki/Form/Checkbox.pm   (working copy)
@@ -3,6 +3,7 @@
 
 use strict;
 use warnings;
+use Assert;
 
 use Foswiki::Form::ListFieldDefinition ();
 our @ISA = ('Foswiki::Form::ListFieldDefinition');
@@ -18,12 +19,68 @@
     return $this;
 }
 
+sub finish {
+    my $this = shift;
+    $this->SUPER::finish();
+    undef $this->{valueMap};
+}
+
+sub getOptions {
+    my $this = shift;
+
+    return $this->{_options} if $this->{_options};
+
+    my $vals = $this->SUPER::getOptions(@_);
+    if ( $this->{type} =~ /\+values/ ) {
+
+        # create a values map
+
+        $this->{valueMap} = ();
+        $this->{_options} = ();
+        my $str;
+        foreach my $val (@$vals) {
+            if ( $val =~ /^(.*?[^\\])=(.*)$/ ) {
+                $str = TAINT($1);
+      my $descr = $this->{_descriptions}{$val};
+                $val = $2;
+      $this->{_descriptions}{$val} = $descr;
+                $str =~ s/\\=/=/g;
+            }
+            else {
+                $str = $val;
+            }
+            $this->{valueMap}{$val} = Foswiki::urlDecode($str);
+            push @{ $this->{_options} }, $val;
+        }
+    }
+
+    return $vals;
+}
+
 # Checkboxes can't provide a default from the form spec
 sub getDefaultValue { return; }
 
 # Checkbox store multiple values
 sub isMultiValued { return 1; }
 
+sub renderForDisplay {
+    my ( $this, $format, $value, $attrs ) = @_;
+
+    $this->getOptions();
+
+    my @vals = ();
+    foreach my $val (split(/\s*,\s*/, $value)) {
+      if ( defined( $this->{valueMap}{$val} ) ) {
+          push @vals, $this->{valueMap}{$val};
+      } else {
+          push @vals, $val;
+      }
+    }
+    $value = join(", ", @vals);
+
+    return $this->SUPER::renderForDisplay( $format, $value, $attrs );
+}
+
 sub renderForEdit {
     my ( $this, $topicObject, $value ) = @_;
 
@@ -47,24 +104,35 @@
     $value = '' unless defined($value) && length($value);
     my %isSelected = map { $_ => 1 } split( /\s*,\s*/, $value );
     my %attrs;
+    my @defaults;
     foreach my $item ( @{ $this->getOptions() } ) {
 
+        my $title = $item;
+        $title = $this->{_descriptions}{$item}
+          if $this->{_descriptions}{$item};
+
         # NOTE: Does not expand $item in title
         $attrs{$item} = {
             class => $this->cssClasses('foswikiCheckbox'),
-            title => $topicObject->expandMacros($item),
+            title => $topicObject->expandMacros($title),
         };
 
         if ( $isSelected{$item} ) {
             $attrs{$item}{checked} = 'checked';
+            push( @defaults, $item );
         }
     }
-    $value = CGI::checkbox_group(
+    my %params = (
         -name       => $this->{name},
         -values     => $this->getOptions(),
+        -defaults   => \@defaults,
         -columns    => $this->{size},
         -attributes => \%attrs
     );
+    if (defined $this->{valueMap}) {
+      $params{-labels} = $this->{valueMap};
+    }
+    $value = CGI::checkbox_group(%params);
 
     # Item2410: We need a dummy control to detect the case where
     #           all checkboxes have been deliberately unchecked
Index: lib/Foswiki/Form/ListFieldDefinition.pm
===================================================================
--- lib/Foswiki/Form/ListFieldDefinition.pm   (revision 11867)
+++ lib/Foswiki/Form/ListFieldDefinition.pm   (working copy)
@@ -32,6 +32,7 @@
     my $this = shift;
     $this->SUPER::finish();
     undef $this->{_options};
+    undef $this->{_descriptions};
 }
 
 # PROTECTED - parse the {value} and extract a list of options.
@@ -44,6 +45,7 @@
     return $this->{_options} if $this->{_options};
 
     my @vals = ();
+    my %descr = ();
 
     @vals = split( /,/, $this->{value} );
     if ( !scalar(@vals) ) {
@@ -72,8 +74,11 @@
                 if (/^\s*\|\s*\*Name\*\s*\|/) {
                     $inBlock = 1;
                 }
-                elsif (/^\s*\|\s*([^|]*?)\s*\|/) {
-                    push( @vals, TAINT($1) ) if ($inBlock);
+                elsif (/^\s*\|\s*([^|]*?)\s*\|(?:\s*([^|]*?)\s*\|)?/) {
+          if ($inBlock) {
+            push( @vals, TAINT($1) );
+                      $descr{$1} = $2 if defined $2;
+          }
                 }
                 else {
                     $inBlock = 0;
@@ -84,6 +89,7 @@
     @vals = map { $_ =~ s/^\s*(.*)\s*$/$1/; $_; } @vals;
 
     $this->{_options} = \@vals;
+    $this->{_descriptions} = \%descr;
 
     return $this->{_options};
 }
Index: lib/Foswiki/Form/Radio.pm
===================================================================
--- lib/Foswiki/Form/Radio.pm   (revision 11867)
+++ lib/Foswiki/Form/Radio.pm   (working copy)
@@ -3,6 +3,7 @@
 
 use strict;
 use warnings;
+use Assert;
 
 use Foswiki::Form::ListFieldDefinition ();
 our @ISA = ('Foswiki::Form::ListFieldDefinition');
@@ -21,6 +22,55 @@
     return $this;
 }
 
+sub finish {
+    my $this = shift;
+    $this->SUPER::finish();
+    undef $this->{valueMap};
+}
+
+sub getOptions {
+    my $this = shift;
+
+    return $this->{_options} if $this->{_options};
+
+    my $vals = $this->SUPER::getOptions(@_);
+    if ( $this->{type} =~ /\+values/ ) {
+
+        # create a values map
+
+        $this->{valueMap} = ();
+        $this->{_options} = ();
+        my $str;
+        foreach my $val (@$vals) {
+            if ( $val =~ /^(.*?[^\\])=(.*)$/ ) {
+                $str = TAINT($1);
+      my $descr = $this->{_descriptions}{$val};
+                $val = $2;
+      $this->{_descriptions}{$val} = $descr;
+                $str =~ s/\\=/=/g;
+            }
+            else {
+                $str = $val;
+            }
+            $this->{valueMap}{$val} = Foswiki::urlDecode($str);
+            push @{ $this->{_options} }, $val;
+        }
+    }
+
+    return $vals;
+}
+
+sub renderForDisplay {
+    my ( $this, $format, $value, $attrs ) = @_;
+
+    $this->getOptions();
+
+    if ( defined( $this->{valueMap}{$value} ) ) {
+        $value = $this->{valueMap}{$value};
+    }
+    return $this->SUPER::renderForDisplay( $format, $value, $attrs );
+}
+
 sub renderForEdit {
     my ( $this, $topicObject, $value ) = @_;
 
@@ -28,24 +78,28 @@
     my $session  = $this->{session};
     my %attrs;
     foreach my $item ( @{ $this->getOptions() } ) {
+        my $title = $item;
+        $title = $this->{_descriptions}{$item} if $this->{_descriptions}{$item};
         $attrs{$item} = {
             class => $this->cssClasses('foswikiRadioButton'),
-            title => $topicObject->expandMacros($item)
+            title => $topicObject->expandMacros($title)
         };
 
         $selected = $item if ( $item eq $value );
     }
 
-    return (
-        '',
-        CGI::radio_group(
+    my %params = (
             -name       => $this->{name},
             -values     => $this->getOptions(),
             -default    => $selected,
             -columns    => $this->{size},
-            -attributes => \%attrs
-        )
+            -attributes => \%attrs,
     );
+    if (defined $this->{valueMap}) {
+      $params{-labels} = $this->{valueMap};
+    }
+
+    return ('', CGI::radio_group(%params));
 }
 
 1;
Index: lib/Foswiki/Form/Select.pm
===================================================================
--- lib/Foswiki/Form/Select.pm   (revision 11867)
+++ lib/Foswiki/Form/Select.pm   (working copy)
@@ -44,9 +44,11 @@
 sub getOptions {
     my $this = shift;
 
-    return $this->{_options} if $this->{_options};
+    my $vals = $this->{_options};
+    return $vals if $vals;
 
-    my $vals = $this->SUPER::getOptions(@_);
+    $vals = $this->SUPER::getOptions(@_);
+
     if ( $this->{type} =~ /\+values/ ) {
 
         # create a values map
@@ -57,7 +59,9 @@
         foreach my $val (@$vals) {
             if ( $val =~ /^(.*?[^\\])=(.*)$/ ) {
                 $str = TAINT($1);
+      my $descr = $this->{_descriptions}{$val};
                 $val = $2;
+      $this->{_descriptions}{$val} = $descr;
                 $str =~ s/\\=/=/g;
             }
             else {
@@ -91,15 +95,30 @@
 
 sub isMultiValued { return shift->{type} =~ /\+multi/; }
 
+sub renderForDisplay {
+    my ( $this, $format, $value, $attrs ) = @_;
+
+    $this->getOptions();
+
+    if ( defined( $this->{valueMap}{$value} ) ) {
+        $value = $this->{valueMap}{$value};
+    }
+    return $this->SUPER::renderForDisplay( $format, $value, $attrs );
+}
+
 sub renderForEdit {
     my ( $this, $topicObject, $value ) = @_;
 
     my $choices = '';
 
     my %isSelected = map { $_ => 1 } split( /\s*,\s*/, $value );
-    foreach my $option ( @{ $this->getOptions() } ) {
+    foreach my $item ( @{ $this->getOptions() } ) {
+        my $option = $item;
         my %params = ( class => 'foswikiOption', );
         $params{selected} = 'selected' if $isSelected{$option};
+        if ($this->{_descriptions}{$option}) {
+          $params{title} = $this->{_descriptions}{$option};
+        }
         if ( defined( $this->{valueMap}{$option} ) ) {
             $params{value} = $option;
             $option = $this->{valueMap}{$option};

-- Contributors: MichaelDaum - 07 Jun 2011

Discussion

+1. It's worth noting that loading & parsing the form def topic incurs some overhead. But probably not as big as searching over other topics that are normally consulted in rendering a typical wiki-app.

-- PaulHarvey - 07 Jun 2011

+1 from me

-- KennethLavrsen - 07 Jun 2011

Shouldn't value be the data value, and label the display name (instead of origvalue and value)?

-- ArthurClemens - 07 Jun 2011

Stored values like 1,2,3 will not expand properly with the above patch. Try the patch given below.

diff --git lib/Foswiki/Form/Select.pm lib/Foswiki/Form/Select.pm
index 5e8500e..acf83c6 100644
--- lib/Foswiki/Form/Select.pm
+++ lib/Foswiki/Form/Select.pm
@@ -95,13 +95,25 @@ sub finish {
 
 sub isMultiValued { return shift->{type} =~ /\+multi/; }
 
+sub isValued { return shift->{type} =~ /\+values/; }
+
 sub renderForDisplay {
     my ( $this, $format, $value, $attrs ) = @_;
 
     $this->getOptions();
 
-    if ( defined( $this->{valueMap}{$value} ) ) {
-        $value = $this->{valueMap}{$value};
+    if ( $this->isValued() ) {
+        my @val = split /\s*,\s*/, $value;
+        my @map = ();
+        foreach my $value (@val) {
+            if ( exists( $this->{valueMap}{$value} ) ) {
+                push( @map, $this->{valueMap}{$value} );
+            }else{
+                push( @map, $value );
+       }
+       
+        }
+        $value = join ',', @map;
     }
     return $this->SUPER::renderForDisplay( $format, $value, $attrs );
 }

-- TemiVarghese - 08 Jun 2011

Thanks Temi, you are right. This kind of split before looking up the valueMap hash is done in Checkbox.pm but not yet in Select.pm.

Arthur, we don't have much choice how to name these variables due to backwards compatibility. We currently have A_TITLE, A_VALUE, ROWTITLE, ROWVALUE, $title and $value which more or less mean the same respectively.

-- MichaelDaum - 08 Jun 2011

Accepted by 14 days rule.

-- MichaelDaum - 23 Jul 2011
Topic revision: r9 - 11 Jan 2012, MichaelDaum
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy