Select All / Deselect All fires multiple change events on multiple select [performance]

[petrdvorak]

Problem description

Currently, when user clicks "Select All" or "Deselect All" buttons, following code is called:

1052   selectAll: function () {
1053       this.findLis();
1054       this.$lis.not('.divider').not('.disabled').not('.selected').filter(':visible').find('a').click();
1055   },
1056 
1057   deselectAll: function () {
1058       this.findLis();
1059       this.$lis.not('.divider').not('.disabled').filter('.selected').filter(':visible').find('a').click();
1060   },

Basically, a click event is invoked multiple times and as a result, client JavaScript code receives multiple "change" events. If these change events are connected to some "slow task", such as Ajax calls to server (for example in order to fill in second select with data based on the selection in a first select), performance suffers and additional code must be written in order to better synchronize multiple Ajax calls.

Solution proposal

Adding a new option aggregateChangeEvents: true/false, default = true. If the property is set to false, the component keeps the current behavior (in case someone relies on it in code). If the property is set to true (default), only the code from the click handler is called on all items (no "virtual clicks" are invoked).

Solution affects only "multiselect" selects. However, looking at the code, this change will require some "careful surgery". I will take the issue over if required (just call "Go!" in case you cannot pull solution of the sleve:)) but will ask for a thorough code review after I submit pull request...

[Khrysller]

On friday I was using 1.5.3, if I am not mistake and the select all feature was in a flash, then I updated to this last version, and I saw the select all hangs with a lot of items.

+1

[maresh42]

Hi,

I fixed this problem.

Add this method:

toggleElements: function ($objs, select) {
      var that = this;

      that.$element.find('option').each(function () { $(this).prop('selected', select); });
      $objs.toggleClass('selected', select);

      that.$searchbox.focus();
      that.$element.change();
    },

And modify the select/deselect so that it looks like this:

    selectAll: function () {
      this.findLis();
      var c = this.$lis.not('.divider').not('.disabled').not('.selected').filter(':visible');
      this.toggleElements(c, true);

      //this.$lis.not('.divider').not('.disabled').not('.selected').filter(':visible').find('a').click();
    },

    deselectAll: function () {
      this.findLis();
      //this.$lis.not('.divider').not('.disabled').filter('.selected').filter(':visible').find('a').click();
      var c = this.$lis.not('.divider').not('.disabled').filter('.selected').filter(':visible');
      this.toggleElements(c, false);
    },

It is much faster than triggering a click on every one of them...

[petrdvorak]

... I wanted to fix this one. :( Well, first come first serve. :) Thank you very much.

Did you create a branch with the fix for a pull request, or should I do it?

[petrdvorak]

OK, I spoke too fast - the code, while fast, does not work properly.

Test case:

1. Have a select with 10 items, use data-live-search="true" to include live search.
2. Open the select, make sure all items are selected.
3. Enter a search string to the search box, so that some items are hidden.
4. Click "Deselect All"
   => While some items are marked as selected, the select/options underneath are not selected and select says "Nothing selected"

[caseyjhol]

Looks like this was caused by this commit: b78e9d1. The function was changed to prevent selecting hidden options.

I changed the functions from this:

    selectAll: function () {
      this.findLis();
      this.$lis.not('.divider').not('.disabled').not('.selected').filter(':visible').find('a').click();
    },

    deselectAll: function () {
      this.findLis();
      this.$lis.not('.divider').not('.disabled').filter('.selected').filter(':visible').find('a').click();
    },

back to a slightly modified version of the old code (to account for hidden options):

    selectAll: function () {
      this.findLis();
      this.$element.find('option:enabled').not('[data-divider]').not('[data-hidden]').prop('selected', true);
      this.$lis.not('.divider').not('.dropdown-header').not('.disabled').not('.hidden').addClass('selected');
      this.render(false);
    },

    deselectAll: function () {
      this.findLis();
      this.$element.find('option:enabled').not('[data-divider]').not('[data-hidden]').prop('selected', false);
      this.$lis.not('.divider').not('.dropdown-header').not('.disabled').not('.hidden').removeClass('selected');
      this.render(false);
    },

With this new method, a single change event is fired. I really can't think of any scenarios where you'd want a change event fired for each new value selected if you're simply clicking select all. And if you did wan't that, I'd have to imagine you'd be in the minority, and thus you'd be better off customizing it yourself.

[maresh42]

@caseyjhol Nice one thanks.

Although the code is repeated, just the boolean of the selected property changes. It also makes me realized that the code I pasted before could be even more factorized.

How about adding a generic method that takes the boolean as parameter to avoid redundancy ?

Cheers