adding a column service to datagrid to track widths #3124

gnomeontherun · 2019-02-20T17:39:14Z

This will eventually be used to store several other stateful properties of columns, such as hidden and ordering information. For now, it is to give us a starting implementation point to store column states in a way that is consolidated and easily cached.

Internally, this lifts the state of the width calculations out of the render organizer into a centralized column service. This array of behaviorsubjects get passed into the column cells based on their index, and each cell subscribes to column changes. This privatizes some of the connection between the main/header and row/cell renderers. More work is to be done here, this is just a first pass at the basic architecture.

I also moved the SelectionType enum to its own file, since that is our pattern.

Internal change - no release notes necessary

Shijir

I have some hesitations to go forward with these changes. I might be very well overlooking things. So for now, I would like to hear from you and others on these changes and reasoning behind them.

Shijir · 2019-02-20T23:34:36Z

src/clr-angular/data/datagrid/enums/render-step.enum.ts

@@ -11,4 +11,5 @@ export enum DatagridRenderStep {
  CLEAR_WIDTHS, // Note this is listened to by both cells and columns
  COMPUTE_COLUMN_WIDTHS,
  DETECT_STRICT_WIDTHS,
+  RESET,


Why do we need this designated RESET step?

Previously, the render organizer reset the widths when a full resize was triggered. This is a new step to accomplish the same task by alerting the columns service to reset the calculated widths.

Shijir · 2019-02-21T00:20:20Z

src/clr-angular/data/datagrid/render/main-renderer.ts

@@ -100,6 +109,15 @@ export class DatagridMainRenderer<T = any> implements AfterContentInit, AfterVie
    }
  }

+  private setupColumns() {
+    this.columnsService.columns = this.headers.toArray().map(() => new BehaviorSubject<DatagridColumnState>({}));


I think you can directly transform QueryList with map() without turning it into array.

Shijir · 2019-02-21T00:20:48Z

src/clr-angular/data/datagrid/render/header-renderer.ts


 @Directive({ selector: 'clr-dg-column', providers: [ColumnResizerService] })
 export class DatagridHeaderRenderer implements OnDestroy {
+  stateSubscription: Subscription;


they should be private, right?

Shijir · 2019-02-21T00:40:05Z

src/clr-angular/data/datagrid/providers/columns.service.ts

+import { DatagridRenderOrganizer } from '../render/render-organizer';
+
+@Injectable()
+export class ColumnsService implements OnDestroy {


@gnomeontherun IMO, we are making it too complex. I was imagining a central state mangager class to be much simpler as holding properties for a very few things such as an array of column state objects and observable that any column, cell or whatever that need to subscribe to in order to get state changes.

Currently, this class holds BehaviorSubject for each columns state, and also looks like it's trying to manage the subscriptions in itself too. Also, any column or cell that tries to subsribe to the state changes needs to instantiate another BehaviorSubject as it looks in header-renderer.ts as well as cell-renderer.ts.

My questions is, why did we need to take this route instead of the way I mentioned in the beginning?

The idea is that a column is a thing that has state but that state matters to all of the cells in . that column, and only cells in that column need to know the current state of that column. There are only as many BehaviorSubjects as there are columns, and when the list of columns changes the corresponding column observable is passed to the cells (and subscribed to at the cell level). If there are 101 cells (header and 100 rows) in a column, they all subscribe to the same subject and it becomes simple to alert to a single column about a state change. For width changes from resizing, we only need to change the affected column instead of recompute the whole datagrid.

Having just a central service with an array of widths, an array of orderings, and so forth does not keep the column state as a single unit (which is basically what the current render organizer was doing for widths).

Maybe I'm not clear on your question, if so we can talk about it separately.

This approach makes sense to me. It collects the column states into one place. It also allows the main renderer to create/init the states when it it knows everything is ready for it.

It seems like a good approach to have cells/headers subscribe to the changes I wasn't sure about exposing the BehaviorSubjects directly as commented.

hippee-lee

👍 for the approach. I left a few comments about the implementation. Not approving or rejecting since it doesn't seem appropriate at this stage.

hippee-lee · 2019-02-22T16:31:57Z

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

+ */
+import { DatagridColumnChanges } from '../enums/column-changes.enum';
+
+export interface DatagridColumnState {


Is the intent with this to add other things here? e.g hidden state, master detail mode (if thats the correct description).

Master/Detail shouldn't use the column state, I would guess, but yes the goal is to use thing as a central state for column rendering properties. The ones we identified are:

width + strictness,

order once Shijir merges his reorderable columns feature in,

hidden/shown.

hippee-lee · 2019-02-22T16:38:36Z

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

+import { DatagridColumnChanges } from '../enums/column-changes.enum';
+
+export interface DatagridColumnState {
+  actions?: DatagridColumnChanges[];


This looks like it saves the specific states to replay them on demand. Two comments.

They way Header/Cell renderers react to changes is switching on each action. Is there any concern about that growing to verbose and or complicated requireing many helper utility methods in both renderers?

naming nit: is actions the best way to descibe these changes that need to be applied?

I was surprised by the array the first time I saw it too, but the goal isn't to replay changes. It's to be able to specify several changes at once.

On a column resize, we only need to recompute widths. Hide/show and order are safe to preserve.
On initialization, this array will contain all the enum values.
When hiding a column, this would probably contain HIDE and WIDTH, but not ORDER.

Would you prefer changes instead of actions?

changes makes more sense to me since its an array of DatagridColumnChanges's. It wouldn't be something that I would feel strong enough about to block if actions is more natural to everyone else.

hippee-lee · 2019-02-22T16:50:18Z

src/clr-angular/data/datagrid/render/header-renderer.ts

+      this.stateSubscription.unsubscribe();
+    }
+    this._columnState = columnState;
+    this.stateSubscription = columnState.asObservable().subscribe(state => this.stateChanges(state));


Would it be more natural to add a getter to the columnsState instead of directly exposing the BehaviorSubject? Same comment on cell-renderer

I'm not sure I understand, columnState is a BehaviorSubject. This is to autosubscribe to it anytime the state observable is passed back down into the cell (like if a column was hidden).

That makes sense when I see that columnsState comes from the array of columnService.columns.

hippee-lee · 2019-02-22T16:54:53Z

src/clr-angular/data/datagrid/render/header-renderer.ts

+
+  private stateChanges(state: DatagridColumnState) {
+    if (state.actions && state.actions.length) {
+      state.actions.forEach(action => {


Would it make sense to add a util method to the columns service that can centralize the case detection for DatagridColumnsChanges type instead of repeating them in all of the renderers?

There are some duplicated lines in the renderers now that I am thinking I'll extract over time as the need arises, for now I wasn't too worried about it as it makes this change more complex.

hippee-lee · 2019-02-22T16:55:21Z

src/clr-angular/data/datagrid/providers/columns.service.ts

+import { DatagridRenderOrganizer } from '../render/render-organizer';
+
+@Injectable()
+export class ColumnsService implements OnDestroy {


This approach makes sense to me. It collects the column states into one place. It also allows the main renderer to create/init the states when it it knows everything is ready for it.

It seems like a good approach to have cells/headers subscribe to the changes I wasn't sure about exposing the BehaviorSubjects directly as commented.

youdz · 2019-02-22T17:23:55Z

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

@@ -61,6 +61,7 @@ export class ClrDatagridColumnSeparator {

  private redFlagTracker(resizeTrackerEl: HTMLElement) {
    let isWithinMaxResizeRange: boolean;
+    // @TODO(JEREMY) Review this, it will always be true because above is always null
    if (isWithinMaxResizeRange !== this.columnResizerService.isWithinMaxResizeRange) {


You're right, how is this even working right now? Actually, is it working?

Hee sorry, overlooked this one. it should have been a property instead of a variable. It will work but we are setting style through Renderer2 constantly during resizing.

I figured it was meant to be a property, but didn't spend the time to dive deeper.

youdz · 2019-02-22T17:32:35Z

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

+ */
+import { DatagridColumnChanges } from '../enums/column-changes.enum';
+
+export interface DatagridColumnState {


Master/Detail shouldn't use the column state, I would guess, but yes the goal is to use thing as a central state for column rendering properties. The ones we identified are:

width + strictness,

order once Shijir merges his reorderable columns feature in,

hidden/shown.

youdz · 2019-02-22T17:46:39Z

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

+import { DatagridColumnChanges } from '../enums/column-changes.enum';
+
+export interface DatagridColumnState {
+  actions?: DatagridColumnChanges[];


I was surprised by the array the first time I saw it too, but the goal isn't to replay changes. It's to be able to specify several changes at once.

On a column resize, we only need to recompute widths. Hide/show and order are safe to preserve.
On initialization, this array will contain all the enum values.
When hiding a column, this would probably contain HIDE and WIDTH, but not ORDER.

youdz · 2019-02-22T18:09:08Z

src/clr-angular/data/datagrid/render/row-renderer.ts

-  ngAfterContentInit() {
-    this.cells.changes.subscribe(() => {
-      this.setWidths();
+      cell.columnState = this.columnsService.columns[index];


Do we have a case where the cells of a row can change (I mean this.cells.changes fires) without the headers of the datagrid changing? If so, then you're missing a subscription here for cell ContentChildren changes.

True, not a usual case I would guess but I'm sure it happens.

youdz · 2019-02-22T18:11:00Z

src/clr-angular/data/datagrid/render/cell-renderer.ts

+      this.stateSubscription.unsubscribe();
+    }
+    this._columnState = columnState;
+    this.stateSubscription = columnState.asObservable().subscribe(state => this.stateChanges(state));


asObservable() doesn't do anything here. It's only useful for type safety if you want to expose a public Observable and keep a private Subject. But the code for asObservable() in RxJS is really just:

asObservable() { return this as Observable<T>; }

youdz · 2019-02-22T18:12:35Z

src/clr-angular/data/datagrid/render/cell-renderer.ts

  }

  private clearWidth() {
    this.renderer.removeClass(this.el.nativeElement, STRICT_WIDTH_CLASS);
    this.renderer.setStyle(this.el.nativeElement, 'width', null);
  }

-  public setWidth(strict: boolean, value: number) {
-    if (strict) {
+  private setWidth(state) {


state: DatagridColumnState

youdz · 2019-02-22T18:14:31Z

src/clr-angular/data/datagrid/render/header-renderer.ts

+    return this._columnState;
+  }
+
+  set columnState(columnState: BehaviorSubject<DatagridColumnState>) {


Can you add a comment that we should figure out a way to avoid the code duplication between DatagridCellRenderer and DatagridHeaderRenderer in the future? They started with a few lines of common code, but it's now grown to dozens.

Not something we have to do now, but a TODO comment lets us remember it next time we add another feature in here.

youdz · 2019-02-22T18:19:32Z

src/clr-angular/data/datagrid/render/main-renderer.ts

@@ -100,6 +109,15 @@ export class DatagridMainRenderer<T = any> implements AfterContentInit, AfterVie
    }
  }

+  private setupColumns() {
+    this.columnsService.columns = this.headers.toArray().map(() => new BehaviorSubject<DatagridColumnState>({}));


Dynamic columns with *ngIf or changing *ngFor will lose all information of the current column state. I'm pretty sure in the current version this would be pretty bad anyway, but I figured I'd point it out if anyone has a good idea. I don't think it's critical, except maybe for columns that the user manually resized.

I had thought about this, but the current behavior will lose the column state too. We'd have to somehow know to cache that state and carry it over, something I can't see a good solution for without the app passing some configuration. Something for another day I guess.

youdz · 2019-02-22T18:23:12Z

src/clr-angular/data/datagrid/render/cell-renderer.ts

      this.renderer.addClass(this.el.nativeElement, STRICT_WIDTH_CLASS);
    } else {
      this.renderer.removeClass(this.el.nativeElement, STRICT_WIDTH_CLASS);
    }
-    this.renderer.setStyle(this.el.nativeElement, 'width', value + 'px');
+    this.renderer.setStyle(this.el.nativeElement, 'width', state.width + 'px');


In your new version, you're using a number strictWidth that overrides the width of the column state. Yet you're still using this logic as if strictWidth was a boolean, ignoring the actual number value and still setting the width value.

I would go back to a boolean because I think it's clearer, but right now you're in between and it's very confusing.

Ah this is a tricky one. The cell previously had a strictWidth property that got populated as well as a strict boolean. I was trying to condense two properties into one and move that up into the state instead. I haven't pulled this through yet fully (was trying to limit changes) but can work on that too.

Shijir · 2019-03-05T00:20:53Z

src/clr-angular/data/datagrid/providers/columns.service.ts

+      this.columns[columnIndex] = new BehaviorSubject<DatagridColumnState>({});
+    }
+    const current = this.columns[columnIndex].value;
+    const hasChange = Object.keys(diff).find(key => diff[key] !== current[key]);


I don't think .find() works in IE11. I've used filter() instead.

youdz · 2019-03-06T21:41:25Z

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

@@ -4,6 +4,4 @@
 * The full license information can be found in LICENSE in the root directory of this project.
 */

-export const NO_LAYOUT_CLASS = 'datagrid-no-layout';


The fact that this is now unused makes me think we might have some performance issues when computing columns width on older browsers. We used to hide everything while we moved cells in place, and then display everything at once. I remember Firefox would try to recompute all the flexboxes every time you moved a cell around.

Obviously not something to modify here, but it's definitely worth checking soon.

Do you want me to keep this with a TODO?

Sure, I think that's be helpful. Thanks.

youdz · 2019-03-06T21:48:41Z

src/clr-angular/data/datagrid/render/header-renderer.ts

  ngOnDestroy() {
    this.subscriptions.forEach(sub => sub.unsubscribe());
    if (this.stateSubscription) {
      this.stateSubscription.unsubscribe();
    }
  }

+  ngOnInit() {
+    this.detectStrictWidth();


I don't understand this. this.detectStrictWidth() returns a number and has absolutely no side effects that I can see, so this ngOnInit() doesn't do anything at all, unless I missed something.

This is an artifact of the refactoring that can go away.

youdz · 2019-03-06T22:35:12Z

src/clr-angular/data/datagrid/render/header-renderer.spec.ts

-      delete context.clarityDirective.strictWidth;
-      expect(context.clarityDirective.computeWidth()).toBe(123);
-      context.clarityDirective.setWidth(123);
+      expect(context.clarityElement.style.width).toBeFalsy();


Duplicate of line 123.

youdz · 2019-03-06T22:45:26Z

src/clr-angular/data/datagrid/providers/columns.service.ts

@@ -30,6 +30,9 @@ export class ColumnsService implements OnDestroy {

  // Helper method to emit a change to a column only when there is an actual diff to process for that column
  emitStateChange(columnIndex: number, diff: Partial<DatagridColumnState>) {
+    if (!this.columns[columnIndex]) {
+      this.columns[columnIndex] = new BehaviorSubject<DatagridColumnState>({});


I'm trying to understand the point of this. I'm guessing there might be a race condition somewhere where we try to change the state of a column before the datagrid's main renderer has the actual list of all the columns.

But shouldn't we just return in this case? We're creating a BehaviorSubject that no one subscribes to, since the setupColumns() method of the main renderer just replaces this subject with a new one.

Yeah we can just return. Fixed.

youdz · 2019-03-06T22:51:45Z

src/clr-angular/data/datagrid/render/main-renderer.spec.ts

@@ -95,14 +99,15 @@ export default function(): void {
        expect(computeWidthSpy.calls.count()).toBe(0);
        // Too lazy to do something other than casting right now.


I like old comments that don't make sense anymore. 😁

youdz · 2019-03-06T22:59:18Z

src/clr-angular/data/datagrid/render/row-renderer.ts

    });
  }
-
-  ngAfterContentInit() {
-    this.cells.changes.subscribe(() => {


You're still missing this dynamic cell change subscription.

This needs to happen now in the main renderer.

youdz · 2019-03-06T23:00:34Z

src/clr-angular/data/datagrid/render/row-renderer.spec.ts

-      expect(cellWidthSpy).not.toHaveBeenCalled();
-    });
-
-    it('sets the widths of the cells when created after the widths have been computed', function() {


Losing this test seems like a pretty big deal to me. It's important that rows added after the width have been computed correctly render with the right width. It's the basis of how our rendering works with pagination, for instance.

To be honest, right now I'm not even sure this works in the current state of this refactor. I do not see how moving to the next page preserves the computed columns width and renders them on the new rows.

I've put a test back in for something similar.

youdz · 2019-03-06T23:09:40Z

src/clr-angular/data/datagrid/enums/render-step.enum.ts

@@ -11,4 +11,5 @@ export enum DatagridRenderStep {
  CLEAR_WIDTHS, // Note this is listened to by both cells and columns
  COMPUTE_COLUMN_WIDTHS,
  DETECT_STRICT_WIDTHS,
+  RESET,


The more I read this, the more I think RESET and CLEAR_WIDTHS are exactly the same step. Not sure why we still have the two of them.

I added it per your request 🥇 but will remove.

Shijir · 2019-03-12T17:02:57Z

src/clr-angular/data/datagrid/render/cell-renderer.ts

+  private stateSubscription: Subscription;
+  private _columnState: BehaviorSubject<DatagridColumnState>;
+
+  get columnState(): BehaviorSubject<DatagridColumnState> {


Do we need to have a getter for this? Nothing should be subscribing to from outside, right?

The reason for this was that you could get the column state from here, though it is not in use right now. I can remove this for now unless it becomes needed.

Shijir · 2019-03-12T17:19:42Z

src/clr-angular/data/datagrid/providers/columns.service.ts

+    if (!this.columns[columnIndex]) {
+      return;
+    }
+    const current = this.columns[columnIndex].value;


I think we will try to access the .value many times from the ColumnService. So I think it's ideal to have a method for this like:

inspectStateOf(columnIndex: number) { return this.columns[columnIndex] && this.columns[columnIndex].value }

I think we can optimize this later if we do, otherwise its premature at this stage what we need here.

Shijir · 2019-03-12T17:49:34Z

src/clr-angular/data/datagrid/render/row-renderer.ts


-  private setWidths() {
-    if (this.organizer.widths.length !== this.cells.length) {


Have you checked it with dynamic columns? I think we need this guard. This will make datagrid widths broken in the case dynamics columns.

Yes I've tested with *ngIf on columns and toggling them. We reset the state when a column is removed and reattached, which is true before this change as well. It does calculate an initial strict width if it was set on the column like [style.width.px]="100", otherwise reiszed widths are cleared.

Sounds good. Thanks for confirming.

Shijir · 2019-03-12T17:54:01Z

src/clr-angular/data/datagrid/render/header-renderer.ts

+  private stateSubscription: Subscription;
+  private _columnState: BehaviorSubject<DatagridColumnState>;
+
+  get columnState(): BehaviorSubject<DatagridColumnState> {


Looks like we are not using this columnState property here either. Do we even need to save the BehaviorSubject to a property?

Shijir · 2019-03-12T18:01:26Z

src/clr-angular/data/datagrid/render/header-renderer.ts

+    return this._columnState;
+  }
+
+  set columnState(columnState: BehaviorSubject<DatagridColumnState>) {


In dynamic columns, if we toggle additional columns, we will run this method for every column. Should we have a guard to check their references, exit out early if the columnState argument has the same reference as whatever saved to _columnState?

When columns change, we do need to run this method to ensure they sync their latest states. If a new column arrives it might change the widths of other columns (such as last column has the flex column, so another one could take on a specific width defined). The frequency of which columns change is not a concern in performance, as the rows are more likely to change with pagination or data refreshing.

This will eventually be used to store several other stateful properties of columns, such as hidden and ordering information. For now, it is to give us a starting implementation point to store column states in a way that is consolidated and easily cached. Signed-off-by: Jeremy Wilken <gnomation@gnomeontherun.com>

gnomeontherun self-assigned this Feb 20, 2019

Shijir suggested changes Feb 21, 2019

View reviewed changes

hippee-lee reviewed Feb 22, 2019

View reviewed changes

youdz suggested changes Feb 22, 2019

View reviewed changes

gnomeontherun force-pushed the datagrid/column-refactor-v3 branch from 10ab254 to e1bd562 Compare February 22, 2019 22:41

Shijir reviewed Mar 5, 2019

View reviewed changes

gnomeontherun requested a review from youdz March 5, 2019 02:50

gnomeontherun changed the base branch from topic/datagrid-detail-pane to master March 5, 2019 18:33

youdz reviewed Mar 6, 2019

View reviewed changes

youdz suggested changes Mar 6, 2019

View reviewed changes

youdz reviewed Mar 6, 2019

View reviewed changes

youdz approved these changes Mar 12, 2019

View reviewed changes

hippee-lee approved these changes Mar 12, 2019

View reviewed changes

Shijir reviewed Mar 12, 2019

View reviewed changes

Shijir approved these changes Mar 12, 2019

View reviewed changes

gnomeontherun force-pushed the datagrid/column-refactor-v3 branch from 63782cf to cb46300 Compare March 13, 2019 15:07

gnomeontherun merged commit dc89f1e into vmware-archive:master Mar 13, 2019

gnomeontherun deleted the datagrid/column-refactor-v3 branch March 13, 2019 16:05

		@@ -95,14 +99,15 @@ export default function(): void {
		expect(computeWidthSpy.calls.count()).toBe(0);
		// Too lazy to do something other than casting right now.


		private setWidths() {
		if (this.organizer.widths.length !== this.cells.length) {

adding a column service to datagrid to track widths #3124

adding a column service to datagrid to track widths #3124

Conversation

gnomeontherun commented Feb 20, 2019 • edited

Shijir left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hippee-lee left a 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

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

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

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

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

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

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

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

Shijir Mar 12, 2019 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnomeontherun commented Feb 20, 2019 •

edited

Shijir left a comment •

edited

Shijir Mar 12, 2019 •

edited