Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FilterableMixin #1288

Closed
wants to merge 2 commits into from
Closed

Add FilterableMixin #1288

wants to merge 2 commits into from

Conversation

crofty
Copy link
Contributor

@crofty crofty commented Aug 16, 2012

Here is my pass at a filterable mixin. I've had a need for something like this in a couple of projects and so I'm putting it here to get some feedback.

The aim is to have an array proxy that can produce a filtered array of items based on a provided filter. The filtering needs to be done intelligently so that as items are added and removed, the entire list is not replaced.

I've put together a fiddle showing the filtered mixin working: http://jsfiddle.net/ydXrU/10/
As items are removed/added to the filtered list, only the newly added item gets rendered.

Compare this to a naive implementation using Enumerable#filter: http://jsfiddle.net/bbjaA/1/
The entire list is rerendered each time an item is added/removed.

API

I'd appreciate thoughts & suggestions on the API.

By default, the items are included in the filtered array if their filterProperties are truthy

App.filteredCollection =  Ember.ArrayProxy.create(Ember.FilterableMixin, {
    filterProperties: ['display'], // Any item with a truthy display property will be included
    content: content
});

More advanced filtering can be done by specifying a filterCondition. However, any of the properties used in the filterCondition will need to be defined in the filterProperties array.

App.filteredCollection =  Ember.ArrayProxy.create(Ember.FilterableMixin, {
    filterProperties: ['id', 'name'],
    filterCondition: function(item){
      return item.get('id') && item.get('name') === 'James';
    },
    content: content
});

I not 100% happy with this API for the following reasons:

  • if you are using a custom filterCondition, you need to make sure all the properties used in the filterCondition are listed in filterProperties array. If they are not, then the observers won't all be set up correctly.
  • if the filterCondition is swapped out for a different filterCondition, nothing is notified and so the array will not be refiltered.

Implementation

The implementation of FilterableMixin is largely the same as the SortableMixin.

Works in a similar way to the SortableMixin.

Mix this into an ArrayProxy, define the filterProperties and the array
will be filtered to only include the objects whose filterProperties are
truthy.

You can get more advanced filtering by defining a custom
filterCondition. The filterCondition is used to determine if the object is
to be included in the filtered list.  Any item properties used in
the filterCondition will need to be lised in filterProperties.
@travisbot
Copy link

This pull request passes (merged 757bea3 into 138fbdf).

@@ -7,6 +7,7 @@
require('ember-runtime/system/array_proxy');
require('ember-runtime/controllers/controller');
require('ember-runtime/mixins/sortable');
require('ember-runtime/mixins/filterable');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the require, but the ArrayController does't not implement the mixin, it's just a miss ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to require it somewhere so that it got picked up for the tests. ArrayController seemed the logical place as that is where sortable was required.

Any suggestions on another way to get the files picked up by the test runner?

@michaelbaudino
Copy link

I totally miss such a feature. It's great !
I specially love the filterCondition attribute that should let us filter based on a regexp.

And you're right, it would be even better if we could change filterProperties or filterCondition and that the array was automatically re-filtered (like the arrangedContent is automatically re-sorted when changing sortProperties or sortAscending in an object implementing Ember.SortableMixin)

@crofty
Copy link
Contributor Author

crofty commented Aug 16, 2012

It would be nice if the content refiltered if you changed the filterCondition but I don't think this is possible without making the function into a property and forcing it to go through get / set which I didn't want to do.

The analogous example in SortableMixin is swapping out the orderBy function. If you swap this out for a different sorting mechanism function, then the content doesn't get automatically resorted. It only gets resorted if you change sortProperties or sortAscending which are both properties.

These verify that the FilterableMixin works with bound content and updates correctly when the content is swapped out.
@ahawkins
Copy link

@crofty what's going on with this?

@crofty
Copy link
Contributor Author

crofty commented Nov 14, 2012

@twinturbo not sure. I'm using it and I have been contacted by others who are using it. I assume we are waiting to see if it is something that the core team want to include in core. @wagenet thoughts?

@ahawkins
Copy link

@crofty @wycats said he was interested in this. I was shocked to see that there was SortabledMixin but not a FilterableMixin.

On a separate note, how could these two be combined?

@crofty
Copy link
Contributor Author

crofty commented Nov 14, 2012

@twinturbo I normally combine them through an intermediary array, see: http://matchingnotes.com/ember-array-proxy/multiple-mixins

Since ArrayProxy mixins tend to override the arrangedContent property to achieve their behaviour, using multiple mixins in a single proxy will cause problems as each mixin will be overriding the same property.

@sly7-7
Copy link
Contributor

sly7-7 commented Nov 14, 2012

I think there is an other feature linked to thes ones for an array. This is the selection. Behind these three things now, we have to find something consistent in order to be able to link them together, or just using some of them.

@ahawkins
Copy link

@sly7-7 would that even matter? Assume arrangedContent is the API point for all the selection index stuff. It doesn't matter what actually happens inside arrangedContent. Does _super work accordingly when using multiple mixins (ala Ruby)?

@sly7-7
Copy link
Contributor

sly7-7 commented Nov 14, 2012

To be honest I don't know, but there seems to have some conflicts using both Filterable/Sortable on the same Array(Proxy).
All I wanted to say is that for me all these three things should be working together, because we can imagine something like multiple selection in an array, then filtering, then un-filtering, should the previous selection be kept ? What if you remove selected object in the array ? the selection should be empty ? or just going to the previous or next index ? (but what is this index, if the array is filtered, or sorted ?) Do you see what I mean ? For me it's more complex that it seems... :s

@ahawkins
Copy link

@wycats @tomdale @wagenet @ebryn what do you think of a proposal for combining SortableMixing and FilterableMixin into one single ArrangableMixin with a unified interface?

@wagenet
Copy link
Member

wagenet commented Nov 16, 2012

@twinturbo That seems like it could be useful if the overlap was great enough.

@ahawkins
Copy link

I suggest unifying that way you can use both at once and not either or.
Using both would be hard: c/d
On Nov 16, 2012 7:19 PM, "Peter Wagenet" [email protected] wrote:

@twinturbo https://github.com/twinturbo That seems like it could be
useful if the overlap was great enough.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1288#issuecomment-10456478.

@ebryn
Copy link
Member

ebryn commented Nov 16, 2012

If you want to use both you have to wrap in another ArrayProxy

@ahawkins
Copy link

If you want to use both you have to wrap in another ArrayProxy

Ya I thought of that too. I think having one mixin with the sami API for sorting and filtering would be more usable than the alternatives.

@ahawkins
Copy link

@crofty what do you think of closing this in favor of the arrangeable mixin PR?

@crofty
Copy link
Contributor Author

crofty commented Dec 13, 2012

@twinturbo I can see that ArrangeableMixin is convenient if you need to do sorting and filtering, however, this can be easily introduced at the application level by wrapping array proxies around each other. At the framework level, it feels to me as if the mixins should be kept separate. I guess I like the idea of having mixins that are focussed on one specific thing and that can be chained, in any order, to produce the desired behaviour.

Some of my reasoning behind this is just that sorting and filtering are such clear concepts and one is often needed without the other. To me, sticking the two together and calling it 'Arrangeable' makes it less clear what it does. I'd prefer to stick with the well established terminology of filtering and sorting and have an easy way to combine the two if needed.

Would be interested to hear the thoughts of others on this though.

@wagenet
Copy link
Member

wagenet commented Dec 14, 2012

@crofty While it may be common to use them separately, it seems like it will also be pretty common to use them together. Having to do two proxies to handle this seems like unnecessary work for the user.

@crofty
Copy link
Contributor Author

crofty commented Dec 14, 2012

@wagenet if the framework wants to provide something that does both sorting and filtering then I think it's better to do this by combining the Sortable and Filterable mixins rather than getting rid of them for something that does both.

Can't the framework provide something like the following? (note, this is just sketched out, I haven't tested it).

var ArrangedProxy = Ember.ArrayProxy.extend({
  init: function(){
    this._super()
    this.set('_filtered', Ember.ArrayProxy.createWithMixins(Ember.FilterableMixin));
    Ember.bind(this, '_filtered.content', 'content');
    this.set('_sorted', Ember.ArrayProxy.createWithMixins(Ember.SortableMixin));
    Ember.bind(this, '_sorted.content', '_filtered.arrangedContent');
  },
  sortPropertiesBinding: '_sorted.sortProperties',
  sortAscendingBinding: '_sorted.sortAscending',
  filterPropertiesBinding: '_filtered.filterProperties',
  arrangedContentBinding: '_sorted.arrangedContent'
});

It seems a shame to lose the clarity of the sorting and filtering mixins by rolling them into one and calling it 'arrangeable'. I'd prefer it if the framework provided it's 'arrangeable' concept by just combing the two underlying mixins.

@ahawkins
Copy link

@crofty I think that would less performant because you need to loop two
arrays and use twice as many observers. Also this way gives us one single
optimization point. We can easily use crossfilter to make this faster where
using a composed version is more difficult.

Composing works fine for us but I wouldn't want noobs to figure that kinda
stuff out.
On Dec 14, 2012 5:02 PM, "James Croft" [email protected] wrote:

@wagenet https://github.com/wagenet if the framework wants to provide
something that does both sorting and filtering then I think it's better to
do this by combining the Sortable and Filterable mixins rather than getting
rid of them for something that does both.

Can't the framework provide something like the following? (note, this is
just sketched out, I haven't tested it).

var ArrangedProxy = Ember.ArrayProxy.extend({
init: function(){
this._super()
this.set('_filtered', Ember.ArrayProxy.createWithMixins(Ember.FilterableMixin));
Ember.bind(this, '_filtered.content', 'content');
this.set('_sorted', Ember.ArrayProxy.createWithMixins(Ember.SortableMixin));
Ember.bind(this, '_sorted.content', '_filtered.arrangedContent');
},
sortPropertiesBinding: '_sorted.sortProperties',
sortAscendingBinding: '_sorted.sortAscending',
filterPropertiesBinding: '_filtered.filterProperties',
arrangedContentBinding: '_sorted.arrangedContent'});

It seems a shame to lose the clarity of the sorting and filtering mixins
by rolling them into one and calling it 'arrangeable'. I'd prefer it if the
framework provided it's 'arrangeable' concept by just combing the two
underlying mixins.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1288#issuecomment-11381173.

@wagenet
Copy link
Member

wagenet commented Dec 19, 2012

Some more discussion on this overall problem is here: #1562 (comment)

@wagenet
Copy link
Member

wagenet commented Jul 19, 2013

@crofty Thanks for the work on this. It looks like #2711 is going to get merged instead which will also address this use case.

@wagenet wagenet closed this Jul 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants