Filter state two-way binding, custom equals #1

speti43 · 2018-11-29T13:09:07Z

Here is a working version of the filter two-way binding, it's not ready to merge yet, because there are serious memory leaks (and some coding guideline issues), I justed wanted to ask you to take a look that the approach is fine, and if you have a suggestion to eliminate the memory leak, then please share the idea :).

audax · 2018-11-29T13:40:34Z

src/dev/src/app/datagrid/utils/color-filter.ts

+    /**
+       * Compare objects by reference
+       */
+    public equals(objectToCompare: ColorFilter): boolean {


This problematic and shows the main difficulty with this approach. The filter object that is passed in the input state must be the same as the registered one, otherwise a new filter is added. You therefore cannot pass a differently configured filter in the input state, because the only the way to do this would be to modify the existing filter, which already changes the state.

For this to work, filters must be serializable or have a state input and output.

For the string filter the equals is ok to check prop name? For color filters may also need an identifier prop name, to determine which filter uses it, and with that we could implement the equals. Or am I wrong?

The string filters also needs to compare the value and, if the value is not the same, you must set the value in the string filter to activate or deactivate it. Basically, you need to have getters and setters for the internal state of the filter.

audax · 2018-11-29T13:45:56Z

You cannot activate or deactive filters using this approach, because filters have to standardized way to do that. The filter interface is just too small. The comparator interface the same issue.

Including these methods/inputs/outputs int the filter and comparator interfaces would a very backwards incompatible change and increase the difficulty for those that do not care about the state input. My approach would be to extend the filter and comparator interfaces as SerializableFilter and SerializableComparator and only let the state input affect those. The builtin filters could already easily become serializable and if someone creates new custom filters and want to use the state input, they have to implement these extended interfaces.

speti43 · 2018-11-29T13:59:21Z

So if I get it right, I should create a new interface which is called SerializableFilter and extends ClrDatagridFilterInterface, and I should create a new implementation as well which will implement this new interface, and i think it can extend the string filter implementation class. In this case we wont affect custom filters like color filter. What about the equals logic, is it fine to you to check the property names?

audax · 2018-11-29T14:57:53Z

Yes, but with one difference: Just patch the builtin sting filter implementation to extend the ClrDatagridFilterInterface because just about everything useful there is private anyway.

Some pseudo-code for setting the state:

 for (const filterState of state.filters) {
        const filter = this.filters.findRegisteredFilterFromState(filterState);
        if (!filter) {
            throw 'unregistered filter in filter state';
        }
        filter.state = filterState;

For implementing the method in the filters provider, the SerializableFilter interface provides:

compatibleToState(state: any): boolean
get state: any;
set state: any;

The state of the builtin string filter for example would be { type: 'BuiltinStringFilter', property: string, value: string} and therefor for the methods would be something like

compatibleToState(state: any): boolean {
    return state.type === 'BuiltinStringFilter' && state.property === this.property
}
get state(): any {
    return { type: 'BuiltinStringFilter', property: this.property, value: this._rawValue}
}
set state(state: any): any {
    this._rawValue = state.value;
    this._changes.next();
}

I recommend to have a type field in the state because all type information is lost at runtime in typescript, so you need to identify the different state types somehow.

speti43 · 2018-11-29T16:53:44Z

Yes, but with one difference: Just patch the builtin sting filter implementation to extend the ClrDatagridFilterInterface because just about everything useful there is private anyway.

You mean to patch the builtin sting filter implementation to implement the new SerializableFilter interface.

audax · 2018-11-30T12:44:20Z

Yes, but with one difference: Just patch the builtin sting filter implementation to extend the ClrDatagridFilterInterface because just about everything useful there is private anyway.

You mean to patch the builtin sting filter implementation to implement the new SerializableFilter interface.

Yes, of course. So many interfaces confuse me!

speti43 · 2018-11-30T12:44:41Z

@audax The refactor is committed, One thing is different, I don't set the filterstate directly, I just set it by the value setter, and via the constructor.

audax · 2018-11-30T12:46:17Z

src/clr-angular/data/datagrid/interfaces/filter.state.interface.ts

+ * The full license information can be found in LICENSE in the root directory of this project.
+ */
+
+export interface FilterStateInterface {


I think this needs to be specific for each filter. There are filters that span multiple properties or that do not have a string value. Each filter therefore needs to provide it's own filter interface and the compatibleToState method needs to accept any.

Or maybe, instead of any, just a very basic interface with only the type which state interface of the individual interfaces then extend. We always lose the type safety anyway when serializing the stuff.

I think every filter has a type, a property name and the value itself, so we should not remove any of these just we should go a generic type parameter, which gives the type of the value, and it should be the same type which is given in the type property in string format.
Pseudo code:
FilterStateInterface <T>{
type: string; //if T is Date then this value should be "Date", so we can preserve the type after serialization
property: string;
value: T;
}

What's your opinion?

I don't think that every filter has a property and the value of a filter is not usually a T, because T is the value of the row and not the column.

FilterStateInterface { readonly type: string }

is the only thing I would require and then the builtin string would have it's own interface:

StringFilterStateInterface extends FilterStateInterface { property: string, value: string }

You can then cast the state here:

compatibleToState(state: any): boolean { if nstate.type === 'BuiltinStringFilter') { return state.property === this.property } else { return false } }

This is semantically the same as my code snippet in #1 (comment) , I just unrolled the code.

It's pushed now. I think instead of any we need FilterStateInterface. Please check the last commit.

audax · 2018-11-30T12:51:42Z

src/clr-angular/data/datagrid/providers/state.provider.ts

@@ -102,17 +103,17 @@ export class StateProvider<T> {
    if (state.filters) {
      const activeFilters = this.filters.getActiveFilters();
      for (const filter of state.filters) {
-        let filterObject: ClrDatagridFilterInterface<T>;
+        let filterObject: SerializableFilter<T>;
        if (filter.hasOwnProperty('property') && filter.hasOwnProperty('value')) {


This special case should be removed in favor of the new state interface of filters. The special case don't work properly because it creates new filters objects instead of updating the existing ones.

New filter creation happens after this "if else" block, in this block we are converting the old filter prop value into the new filterstate object, which is essential, if we omit this block, the functionality will be broken. Can you specify the details of this refactor, because I can't see what exactly do you mean?

Is missed the piece in the code where const existing = activeFilters.findIndex(value => value.compatibleToState(filterObject.filterState)); is called even for the builtin filters.

So the code looks correct, but I would eliminate the special case anyway by modifying the state getter of the state provider to just emit the filterState for builtin string filters as well.

So I should move the filterstate field to ClrDatagridFilterInterface from SerializableFilter? So serializable filter will only contain the compatibleToState method?

This would break existing filters. But you need to check filters if the filters are SerializableFilter and if so, use the state getter. So instead of having this special case, you can use the state getter for all serializable filters.

We can't check that, because interfaces exists only at compile time, so there is no solution to check an object runtime whether it implements an interface. However you can do it with abstract classes.

Yes, or by checking the interface manually. The abstract class would be a nicer solution in that regard, but I don't know which problems this creates in the long term. I guess it would be a good start and then you can see what the clarity maintaners think about that approach.

Just checked since getActiveFilters() already returns SerializableFilters, we don't need to check the type, special case is eliminated. Check the last commit.

audax · 2018-12-04T09:38:02Z

src/clr-angular/data/datagrid/providers/state.provider.ts

+                    this.filters.add(filterObject);
+                }
+            }
+            activeFilters.forEach(filter => this.filters.remove(filter));


Maybe serializable filters should have an option to just deactivate them instead of removing them. Removing filters also clears all bindings to UI elements, so passing a state without an active string filter and then passing a state with an active string filter probably breaks the whole thing. Did you test this somewhere?

it's tested on the UI, it works, but still leaks the memory somewhere.

audax · 2018-12-04T09:40:44Z

So find all the pitfalls of the state setting stuff, you probably should build an end-to-end test that loads a predetermined state and changes filter values in the UI to be sure that no UI bindings are broken when setting a state that deactivates, modifies or activates filters. My test cases have shown that integration tests are not enough to find these quirks.

speti43 · 2018-12-04T11:15:18Z

Memory leak is fixed, now it works fine for built in string filter.

speti43 · 2018-12-05T08:01:41Z

@audax are there any e2e tests (protractor) in the project, is it runned by the pipeline? Or it is enough to make a dedicated page under the datagrid section to demonstrate the usage, with the built in filters, and the custom ones?

audax · 2018-12-05T10:07:20Z

I have no idea, sorry. I am just a user of Clarity poked a bit around in the source. There are some gemini tests, but the datagrid gemini tests are disabled.

speti43 · 2018-12-05T10:20:03Z

All right, I'll prepare demo site, with different filters, and presorted columns, prepaged grid. Is that enough to accept the pull request?

audax · 2018-12-06T10:26:14Z

and you should update the tests that I made and create new tests for the new methods in the filter provider.

…rity into datagrid-state-input

speti43 · 2018-12-06T15:28:57Z

I'm just wondering, how could we restore the state of a custom filter, because it is a content child of the grid, so somehow we have to deliver the state info to the content child, which should implement the SerializableFilter interface, and restore the values in the component. How should we do this, or we should let the user to handle it locally?

audax · 2018-12-07T10:45:08Z

src/clr-angular/data/datagrid/providers/state.provider.ts

+                }
+                const existing = activeFilters.findIndex(value => value.compatibleToState(filterObject.filterState));
+                if (existing !== -1) {
+                    activeFilters.splice(existing, 1);


At this point we only found a compatible filter, but now we must set the state of that filter with existing.filterState = filter.

audax · 2018-12-07T10:48:29Z

The SerializableFilters should be responsible to restore their state by setting their filterState property. Since non-active filters are not included in the datagrid state, we must iterate all registered filters and deactivate those, that have no compatible filterState in the datagrid state that we want to set.

collect all registered filters
iterate for the filters of the state we want to activate
for each filter state we find the compatible registered filter and set its state
deactivate all registered filters that were not assigned a state

speti43 · 2018-12-07T15:20:05Z

It seems, it works now, but we have to identify the filters somehow, we identifies string filters by type and property name, but for custom filters like color filter we also have to use type and id as well, because there can be two different colorfilters on the same grid, so I think we should update FilterStateInterface to include some kind of id beside the type field.

export interface FilterStateInterface {
    id: string;
    type: string;
}

speti43 · 2018-12-07T16:39:58Z

Now the preset for custom filters works, but I still don't know, how to pass an identifier for the custom filters, because we have to differentiate if multiple instance exists of the same type of the custom filter. Given this filter:

<clr-dg-filter [clrDgFilter]="colorFilter">
            <clr-datagrid-color-filter-demo #colorFilter class="color-filter">
            </clr-datagrid-color-filter-demo>
</clr-dg-filter>

When this filter is registered in the grid, we cannot access input properties, because they are null, because the filter component is not in that lifecycle to access these fields, so we need something which is available in the constructor, can we get #colorFilter identifier?
constructor of color-filter:

constructor() {
        this._state =
            {
                id: null, //Need the id here which is provided by the user in the template
                type: 'ColorFilter',
                allColors: this.allColors,
                selectedColors: {}
            };
    }

Any idea is welcome.

audax · 2018-12-10T08:05:04Z

This is an annoying issue. Maybe this would work, even though it is a bit convoluted:

The custom filter generates a random id uniqueId and passes it to the grid
As soon as the custom filters setter for the id-input is called, it passes the real id to the grid
The grid (or the state provider) manages a lookup table of id -> uniqueId

All state setting and getting is queued until all custom filters are registered in the lookup table, e.g. in a replay-subject.

That is probably the point where we should require custom filters to extend an abstract class were all this is defined.

speti43 · 2018-12-13T11:12:05Z

@Wallabeng helped me with the solution, which was obvious, but I didn't think about that. He sets the custom filter ids in the binding-state component's onInit, not in the HTML. It works perfectly, but that means the user have to pay attention to set it.

Date interval filter, Number interval filter, List filter Signed-off-by: Daniel Szegvari <[email protected]>

dszegvari · 2019-01-15T14:50:34Z

Hi @audax , I took over @speti43 's task. I pushed some changes about the custom filters. Please review it when you have the time.
After that, I will continue with the pagination refactor.

audax · 2019-01-15T15:02:56Z

Ok, but just to clarify: This is not the official clarity project, but just my fork which currently has no changes whatsoever.
But since you asked for a review: I don't see any new tests, but lots of new code. I think the clarity project would want to see a good test coverage for this new functionality, especially because it is a big change. Other than that: Overall the direction looks good, I'll probably make a few comments in the commit.

Also, I decided to not use the a writable state api of the datagrid, but instead to bind all writes to properties myself and write a custom filter component that is not integrated with the datagrid at all. I only use the exported state to set the pagination and sorting options, all external writes to those properties are done my binding to the component itself:
<clr-dg-column [clrDgSortOrder]="sortId$ | async" [clrDgSortBy]="'id'">Id</clr-dg-column>

                       [clrDgTotalItems]="total | async"></clr-dg-pagination>```

I use an ngrx store as my point of truth and sync all changes to that and use selectors from that store as input to the `clr-dg-column` and `clr-dg-pagination`. I also sync the state with url parameters and other various sources. All in all, this was much easier with just a hint more boilerplate code in my app.

audax · 2019-01-15T15:06:38Z

src/dev/src/app/datagrid/utils/date-interval-filter.ts

+    this.to = state.to;
+  }
+
+  setId(id: string): void {


I would use a property and not a set-method, since it doesn't do anything anyway. Less code is better and properties are easy.

audax · 2019-01-15T15:09:48Z

src/dev/src/app/datagrid/utils/color-filter.ts

-    }
+  isActive(): boolean {
+    return this.nbColors > 0;
+  }


This file is just full of indentation and formatting changes which clutters up the diff

There is a pre-commit git-hook, that formats the code. But I can disable it, if you would like to.

dszegvari · 2019-01-16T11:58:42Z

Also, I decided to not use the a writable state api of the datagrid, but instead to bind all writes to properties myself and write a custom filter component that is not integrated with the datagrid at all.

I may not understand this, could you please show me an example how you use the custom filters?

audax · 2019-01-16T13:48:28Z

The custom filters are only interacting with the NgRx Store and my DataSource reads the filter option and every other option, like pagination and sorting from the store. The Datagrid just displays the data that is in the store.

                              +--------------------------------+
                              |Datagrid Element @Input Bindings|
                              +---------------^----------------+
                                              |
                                              |
                                              |
+-----------------------+          +----------+-------+        +---------------------+
|Custom Filter Component<---------->NgRx Options Store<--------+Datagrid State export|
+-----------------------+          +----------+-------+        +---------------------+
                                              |
                                              |
                                              |
                                        +-----v----+
                                        |DataSource|
                                        +-----+----+
                                              |
                                              |
                                              |
                                     +--------v------+
                                     |NgRx Data Store|
                                     +--------+------+
                                              |
                                              |
                                              |
                                              |
                                         +----v---+
                                         |Datagrid|
                                         +--------+

Minor refactors, Write new tests, Fix failing tests Signed-off-by: Daniel Szegvari <[email protected]>

dszegvari · 2019-02-05T11:56:11Z

Hi @audax . Sorry for the delay. I had a couple of minor refactors, and wrote some tests. Please review it, if you have time.

audax

Now that vmware-archive#2846 has been implemented, the two-way binding just extend that interface and integrate in the same way.

audax · 2019-02-07T11:16:55Z

package-lock.json

@@ -93,7 +93,7 @@
          "dependencies": {
            "source-map": {
              "version": "0.5.6",
-              "resolved": "https://registry.npmjs.org/source-map/-/source-map-0.5.6.tgz",
+              "resolved": "http://registry.npmjs.org/source-map/-/source-map-0.5.6.tgz",


Why, oh why?

src/clr-angular/data/datagrid/datagrid-column.spec.ts

audax · 2019-02-07T11:17:57Z

src/clr-angular/data/datagrid/datagrid.ts

-    Renderer2,
-    ViewChild,
-    ViewContainerRef,
+  AfterContentInit,


Re-indenting the whole file is not a good idea. Makes for terrible diffs.

Minor refactors Signed-off-by: Daniel Szegvari <[email protected]>

amellnik · 2019-02-21T18:03:28Z

I'm interested in getting this across the finish line -- let me know if there's anything I can help with. Should it be rebased on top of vmware-archive#2846?

audax · 2019-02-21T18:55:35Z

I guess that would be reasonable to decrease the amount of changes that this PR introduces.

Filter state two-way binding, custom equals

83f3e37

speti43 mentioned this pull request Nov 29, 2018

[NG] Ability to set the state of a datagrid with a single input vmware-archive/clarity#2094

Closed

audax reviewed Nov 29, 2018

View reviewed changes

Serializable filter refactor

3fc3f82

audax reviewed Nov 30, 2018

View reviewed changes

no special case for filterstate

63ec41a

audax reviewed Dec 4, 2018

View reviewed changes

speti43 added 2 commits December 4, 2018 11:07

Dedicated StringFilterStateInterface

dbb4ed9

memory leak fix

96f8169

speti43 added 5 commits December 6, 2018 13:21

Filter bugfix, dedicated state binding demo

a12498e

Filter bugfix, dedicated state binding demo

a9db687

Merge branch 'datagrid-state-input' of https://github.com/speti43/cla…

4b8a4fb

…rity into datagrid-state-input

debugger; remove

ab97210

Color filter demo in progress

5c1ec90

audax reviewed Dec 7, 2018

View reviewed changes

Custom filter state binding fix

f55de8f

wallabeng's refactor

f26d7a6

Filter state two-way binding, custom equals

c4cb18d

Date interval filter, Number interval filter, List filter Signed-off-by: Daniel Szegvari <[email protected]>

audax reviewed Jan 15, 2019

View reviewed changes

Filter state two-way binding, custom equals

60e250d

Minor refactors, Write new tests, Fix failing tests Signed-off-by: Daniel Szegvari <[email protected]>

audax requested changes Feb 7, 2019

View reviewed changes

Filter state two-way binding, custom equals

77f3925

Minor refactors Signed-off-by: Daniel Szegvari <[email protected]>

Filter state two-way binding, custom equals #1

Are you sure you want to change the base?

Filter state two-way binding, custom equals #1

Conversation

speti43 commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audax commented Nov 29, 2018

speti43 commented Nov 29, 2018 • edited Loading

audax commented Nov 29, 2018

speti43 commented Nov 29, 2018

audax commented Nov 30, 2018

speti43 commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speti43 Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speti43 Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speti43 Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audax commented Dec 4, 2018

speti43 commented Dec 4, 2018

speti43 commented Dec 5, 2018

audax commented Dec 5, 2018

speti43 commented Dec 5, 2018 • edited Loading

audax commented Dec 6, 2018

speti43 commented Dec 6, 2018

Choose a reason for hiding this comment

audax commented Dec 7, 2018

speti43 commented Dec 7, 2018

speti43 commented Dec 7, 2018 • edited Loading

audax commented Dec 10, 2018

speti43 commented Dec 13, 2018 • edited Loading

dszegvari commented Jan 15, 2019

audax commented Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dszegvari commented Jan 16, 2019

audax commented Jan 16, 2019

dszegvari commented Feb 5, 2019

audax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amellnik commented Feb 21, 2019

audax commented Feb 21, 2019

speti43 commented Nov 29, 2018 •

edited

Loading

speti43 Dec 3, 2018 •

edited

Loading

speti43 Dec 3, 2018 •

edited

Loading

speti43 Dec 3, 2018 •

edited

Loading

speti43 commented Dec 5, 2018 •

edited

Loading

speti43 commented Dec 7, 2018 •

edited

Loading

speti43 commented Dec 13, 2018 •

edited

Loading

audax commented Jan 15, 2019 •

edited

Loading