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

make sure that all filters are serializable #819

Open
gordonwoodhull opened this issue Dec 30, 2014 · 12 comments
Open

make sure that all filters are serializable #819

gordonwoodhull opened this issue Dec 30, 2014 · 12 comments
Milestone

Comments

@gordonwoodhull
Copy link
Contributor

There is some nice general code floating around for creating permalinks with filter info, as requested in #490. However, it doesn't work in the general case because some filters do not serialize/deserialize properly.

In particular, the range filter needs to be special-cased like so:

   for (var i = 0; i< filterObjects.length; i++)
   {
       var filter = filterObjects[i].Filter;
       if(filter instanceof Array) filter = dc.filters.RangedFilter(filter[0], filter[1]);
     dc.chartRegistry.list()[filterObjects[i].ChartID-1].filter(filter);
   }

It would be much nicer if the filters all serialized correctly. I suspect this is a problem for all of the charts with specialized filters (using the objects in dc.filters)

I don't know exactly what is needed here, but thought I'd open an issue to track this. There are a lot of weird special cases in the filter arena.

@gordonwoodhull gordonwoodhull added this to the v2.1 milestone Dec 30, 2014
@gordonwoodhull
Copy link
Contributor Author

SO question that raised this, with before/after fiddles: http://stackoverflow.com/a/27709651/676195

@gordonwoodhull
Copy link
Contributor Author

I don't know the json serialization algo but my guess is that there are two problems with the current filters:

  1. They annotate arrays. There's no way to represent arrays with extra properties in json.
  2. They add functions. json can't represent that either.

Instead, it should probably be a slightly heavier implementation with an object that contains the array, with a type tag, and a filter factory.

I don't know what the best practices in JavaScript are.

The loss in efficiency will not be noticed (no one has thousands of filters and even then it might not matter). And I am 90% certain this is an encapsulated implementation detail which no one relies on.

@gordonwoodhull
Copy link
Contributor Author

Adding a filterType property to each of the dc.filters, for #478 / #989 - should help.

gordonwoodhull added a commit that referenced this issue Sep 18, 2015
@rsivek
Copy link

rsivek commented Dec 29, 2017

I needed to serialize/deserialize in an application I'm developing and I also noticed that additional logic is needed for Dates since they are serialized as ISO 8601 strings. My current solution is to use a regex to determine if a filter string is an 8601 date, and if so, replace it with a constructed Date object on deserialization.

@tttp
Copy link
Contributor

tttp commented Jul 9, 2018

Hi,

I was looking at this question to see if the latest version did solve the serialise question... it doesn't ;(

Wouldn't it be easier to add serialize() and deserialize() to each graph, and so it can be overwritten if a graph needs to handle a date (for instance) or make it fancier and for instance instead of adding the key as a numeric id would put a more human friendly label?

@gordonwoodhull
Copy link
Contributor Author

I think we could make each of the filter types serializable. There isn't really much state in the charts except for the filter. (The rest is initialization / configuration.)

@tttp the human friendly label sounds intriguing - do you mean some mapping from labels to the values that a dimension actually is keyed on?

@tttp
Copy link
Contributor

tttp commented Jul 9, 2018

Yeah, I'm thinking loud here, but on some graphs, the key is the status_id and I .label( return statusName[d.status_id] )
on the filter url, I'd rather have status=close rather than status=1
same thing with countries, with the key is the primary key (int), but I'd rather have country=us rather than country=42

if you want to filter on several values (say both switzerland and france), you can either url encode the json '["ch","fr"]' and get an unredable %5B%22ch%22%2C%22fr%22%5D or choose "-" as separator and have a country=ch-fr

Another case would be the date. you can either save it as number of ms since epoch (unredable), save it as a iso 8601 "2018-07-25T04:40:54Z" (slightly better) or, armed with the knowledge that your specific graph only cares about the month, have a neat "2018-07"

@tttp
Copy link
Contributor

tttp commented Jul 25, 2018

I have a working example, (used with a text filter, ie. you can type "xavier", it will filter to keep only the xaviers' in the list, add a ?name=xavier in the url and if you send the link to someone else, it will be applied when loading the page).

I wrote a helper function urlParam(name [value]) to get/set the url param + history.pushState it

after that:

  • when a filter is applied, add a urlParam("name", filterString)
  • when initialising, read urlParam("name") and if not null, apply the filter

so works well and could easily be made generic... as long as each graph is able to serialise itself.
I read the various comments on this issue again, and IMO, we should allow to have the url filters "human friendly", and for that we need to let each graph choose how to serialise/deserialise its filters.

what about we add in the baseMixin
.urlSave (name [,serializer, deserialiser])

each graph will have the freedom to avoid having to write a serializer/deserialiser (eg textFilterWidget is able to have a generic one) or a pieChart might have .urlSave(name, separator) while letting the user having a custom rangeFilter serialiser that works with date and whatever needed.

Side note: what is the "default" filter type used on rowChart or pieChart? (eg a single value, but you can apply multiple filters)

https://dc-js.github.io/dc.js/docs/html/dc.filters.html

@gordonwoodhull
Copy link
Contributor Author

IMO the nice way to do this would be to make sure that every chart uses a filter class.

You are right that the ordinal, toggling filters currently do not have a filter class but we could pull that logic out of the baseMixin's default filterHandler (all three lines of it) and create an OrdinalFilter.

Similarly we could create a TextFilter class and then textFilterWidget would not set the dimension filter directly - it would just use the mechanism from baseMixin. Nice side effect: user could set the text filter programmatically.

Finally once we have this in place then we just add a serialize method to each of the filter classes. Base mixin could have a serialize method too, but it's just for convenience, since it just serializes the array of filters.

Nothing preventing the filter serialization methods from using external tables if you want to nicify the output.

Glad to help with this if you want to start.

@tttp
Copy link
Contributor

tttp commented Jul 25, 2018

Aaah, it's getting clearer ;)

Not sure if it's directly related, but I found it weird to have to apply filter() several time if I want to select a several ordinal values/slices of a pie. Would it make sense to have the OrdinalFilter so it accepts both a single value (like today) or an array and applies all the values of that array?

@gordonwoodhull
Copy link
Contributor Author

Yes toggling lots of filters is slow too.

You can apply multiple filters in one go now - it's probably just that it wasn't documented when you started using dc.js

http://dc-js.github.io/dc.js/docs/html/dc.baseMixin.html#filter

The double-array syntax is a bit confusing. I think that must have been devised as a way to distinguish between multi-dimensional filters and arrays of filters. There are better ways to do that, but we're stuck with this now. Also, it works!

Likewise you are right that it might make more sense to have ordinal filter take an array since that is the only case where we have multiple filters. But I don't think we can change that now. The OrdinalFilter I'm describing should be backward compatible since it's just a formalization of what's already there.

@tttp
Copy link
Contributor

tttp commented Jul 25, 2018

mind blown.

I completely missed the [[trick]] indeed. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants