Grails

Allow multiple selected values in a <g:select multiple="multiple" /> tag

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 0.5
  • Fix Version/s: 1.0
  • Component/s: TagLib
  • Labels:
    None
  • Patch attached:
    Yes

Description

When using a <g:select /> with multiple selection enabled, there is no way at the moment to tell grails to "pre-select" multiple values. Changing the following (line 513 in 5.0):

if(keyValue==value) {
out << 'select="selected" '
}

to:

if(attrs.multiple) {
if((value instanceof ArrayList && value.contains(keyValue)) || (!(value instanceof ArrayList) && keyValue == value)) { out << 'selected="selected" ' }
}
else {
if(keyValue==value) { out << 'select="selected" ' }
}

allows doing something like this:
<g:select from="[1, 2, 3, 4]" value="[2, 4]" />

(I guess there might be a cleaner way to do that, though)

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Alain Perry added a comment -

Grmbl, how clumsy can one get... I failed on copy-pasting...

Correct (and nicer), version:

FormTagLib.groovy
//Line 513
if(keyValue==value) {
  out << 'selected="selected" '
}

changed to

FormTagLib.groovy
//Line 513
if(attrs.multiple) {
  if((value instanceof ArrayList && value.contains(keyValue)) || (!(value instanceof ArrayList) && keyValue == value)) {
    out << 'selected="selected" '
  }
}
else {
  if(keyValue==value) {
    out << 'selected="selected" '
  }
}
Show
Alain Perry added a comment - Grmbl, how clumsy can one get... I failed on copy-pasting... Correct (and nicer), version:
FormTagLib.groovy
//Line 513
if(keyValue==value) {
  out << 'selected="selected" '
}
changed to
FormTagLib.groovy
//Line 513
if(attrs.multiple) {
  if((value instanceof ArrayList && value.contains(keyValue)) || (!(value instanceof ArrayList) && keyValue == value)) {
    out << 'selected="selected" '
  }
}
else {
  if(keyValue==value) {
    out << 'selected="selected" '
  }
}
Hide
Permalink
Michael Kimsal added a comment -

Voted for this, and am attaching a patch which accomplishes pretty much the same thing as the previous commenter's code did. This was done against an SVN checkout - r6405.

Please consider adding this in, either with this patch directly or by similar code. It's a pretty easy thing to add, and will make using the g:select tag much more robust for lists.

Show
Michael Kimsal added a comment - Voted for this, and am attaching a patch which accomplishes pretty much the same thing as the previous commenter's code did. This was done against an SVN checkout - r6405. Please consider adding this in, either with this patch directly or by similar code. It's a pretty easy thing to add, and will make using the g:select tag much more robust for lists.
Hide
Permalink
Peter Ledbrook added a comment -

Not so easy because of GRAILS-1250. I'm not that keen on the fix provided for that issue, but I can understand why it was implemented. It does make me worry about the side-effects of any change to allow the preselecting of multiple values though. I'll think some more on it.

Show
Peter Ledbrook added a comment - Not so easy because of GRAILS-1250. I'm not that keen on the fix provided for that issue, but I can understand why it was implemented. It does make me worry about the side-effects of any change to allow the preselecting of multiple values though. I'll think some more on it.
Hide
Permalink
Michael Kimsal added a comment -

What would the side effects possibly be? This is functionality that's necessary when building HTML select elements with multiple elements preselected. I don't particularly care how it's accomplished, though I haven't seen any problems yet with the patch I submitted (not been in production use, but haven't seen any errors in my development yet).

Show
Michael Kimsal added a comment - What would the side effects possibly be? This is functionality that's necessary when building HTML select elements with multiple elements preselected. I don't particularly care how it's accomplished, though I haven't seen any problems yet with the patch I submitted (not been in production use, but haven't seen any errors in my development yet).
Hide
Permalink
Peter Ledbrook added a comment -

I'm simply referring to the problem described in GRAILS-1250 where the key value is a string and the value is a number, or vise-versa. In fact, if you look at this example:

<g:select from="[1, 2, 3, 4]" value="[2, 4]" />

you will see that you cannot simply do a toString() call on the key value since value is a list of numbers not a list of Strings.

The current fix for GRAILS-1250 of converting the value and key to strings before the comparison works for numbers, but will it work for all object types? There is no guarantee that toString().equals() will have the same behaviour as equals(). This may be more of a theoretical problem than otherwise, but it still worries me.

Note that I have no objection to the requested functionality, I simply have concerns about how best to implement it .

Show
Peter Ledbrook added a comment - I'm simply referring to the problem described in GRAILS-1250 where the key value is a string and the value is a number, or vise-versa. In fact, if you look at this example: 
<g:select from="[1, 2, 3, 4]" value="[2, 4]" />
you will see that you cannot simply do a toString() call on the key value since value is a list of numbers not a list of Strings. The current fix for GRAILS-1250 of converting the value and key to strings before the comparison works for numbers, but will it work for all object types? There is no guarantee that toString().equals() will have the same behaviour as equals(). This may be more of a theoretical problem than otherwise, but it still worries me. Note that I have no objection to the requested functionality, I simply have concerns about how best to implement it .
Hide
Permalink
Michael Kimsal added a comment -

Thanks Peter. I saw the 1250 issue and still didn't quite understand, given the fix that's in there. Yes, it might not work for all types. Having a toString() will probably fix the issue in almost all cases, however, and give people a defined way to deal with the issue for other types in a list.

I wasn't suggesting that you didn't want the functionality, but perhaps text only doesn't always get across the whole tone of a post.

Thanks for the attention on this one!

Show
Michael Kimsal added a comment - Thanks Peter. I saw the 1250 issue and still didn't quite understand, given the fix that's in there. Yes, it might not work for all types. Having a toString() will probably fix the issue in almost all cases, however, and give people a defined way to deal with the issue for other types in a list. I wasn't suggesting that you didn't want the functionality, but perhaps text only doesn't always get across the whole tone of a post. Thanks for the attention on this one!
Hide
Permalink
Graeme Rocher added a comment -

The fix for GRAILS-1250 was incorrect and hacky and needs to be reverted so this can be applied an another solution found for it. It may be just a matter of throwing a better error message

Show
Graeme Rocher added a comment - The fix for GRAILS-1250 was incorrect and hacky and needs to be reverted so this can be applied an another solution found for it. It may be just a matter of throwing a better error message
Hide
Permalink
Graeme Rocher added a comment -

Note, Alain's solution is actually better as the attached patch seems to just call contains on the value. If its meant to use String.contains that is a JDK 1.5 method

Show
Graeme Rocher added a comment - Note, Alain's solution is actually better as the attached patch seems to just call contains on the value. If its meant to use String.contains that is a JDK 1.5 method
Hide
Permalink
Graeme Rocher added a comment -

Ok so this is the tests I have passing now for this:

void testSimpleSelect() {
        def template = '<g:select name="foo" from="[1,2,3]" value="1" />'

assertOutputEquals('''<select name="foo" id="foo" >
<option value="1" selected="selected" >1</option>
<option value="2" >2</option>
<option value="3" >3</option>
</select>''', template)

    }


    void testMultiSelect() {
        def template = '<g:select name="foo" from="[1,2,3]" value="[2,3]" />'

assertOutputEquals('''<select name="foo" id="foo" multiple="true" >
<option value="1" >1</option>
<option value="2" selected="selected" >2</option>
<option value="3" selected="selected" >3</option>
</select>''', template)

    }

Note you dont have to set multiple=true it just assumes that the case if the value is a collection (i don't see what else it could be other than a multi select in this case)

Show
Graeme Rocher added a comment - Ok so this is the tests I have passing now for this: 
void testSimpleSelect() {
        def template = '<g:select name="foo" from="[1,2,3]" value="1" />'

assertOutputEquals('''<select name="foo" id="foo" >
<option value="1" selected="selected" >1</option>
<option value="2" >2</option>
<option value="3" >3</option>
</select>''', template)

    }


    void testMultiSelect() {
        def template = '<g:select name="foo" from="[1,2,3]" value="[2,3]" />'

assertOutputEquals('''<select name="foo" id="foo" multiple="true" >
<option value="1" >1</option>
<option value="2" selected="selected" >2</option>
<option value="3" selected="selected" >3</option>
</select>''', template)

    }
Note you dont have to set multiple=true it just assumes that the case if the value is a collection (i don't see what else it could be other than a multi select in this case)
Hide
Permalink
Alexandre Masselot added a comment -

a short low invasive fix:
my g:select name is remoteService, and I want to have default selecteion to 'neXtProt','IntAct':

<jq:jquery>
var sel = $('#remoteServices');
<g:each in ="${(params.remoteServices in String)?[params.remoteServices]:params.remoteServices?.toList()?:['neXtProt', 'IntAct'] }" var="s">
sel.find('option[value="${s }"]').attr('selected', 'selected');
</g:each>
</jq:jquery>

Show
Alexandre Masselot added a comment - a short low invasive fix: my g:select name is remoteService, and I want to have default selecteion to 'neXtProt','IntAct': <jq:jquery> var sel = $('#remoteServices'); <g:each in ="${(params.remoteServices in String)?[params.remoteServices]:params.remoteServices?.toList()?:['neXtProt', 'IntAct'] }" var="s"> sel.find('option[value="${s }"]').attr('selected', 'selected'); </g:each> </jq:jquery>

People

Vote (4)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.1#660-r161644) | Report a problem
Powered by a free Atlassian JIRA open source license for Grails project. Try JIRA - bug tracking software for your team.