bugfix/20968 synchronization of connector values - old PR #21242

jomar-honsi · 2024-05-29T09:39:34Z

Tested with JSON connector and RangeModifier. Similar issues with other combinations of connectors/modfiers to be reported in separate tickets.

highsoft-bot · 2024-05-29T09:42:46Z

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

	master	candidate	difference
dashboards/dashboards.js	43.2 kB 148.7 kB	43.5 kB 149.6 kB	260 B 911 B
datagrid/datagrid.js	24.8 kB 76.9 kB	25.0 kB 77.8 kB	261 B 946 B

highsoft-bot · 2024-05-29T09:43:40Z

Dashboard visual diffs

Visual differences found in:

/home/runner/work/highcharts/highcharts/cypress/snapshots/actual/test/cypress/dashboards/visual/dashboards-demo-minimal-loaded.png

See artifacts in the run summary for screenshots

highsoft-bot · 2024-05-29T09:43:41Z

Lighthouse report

dashboards-demo-minimal.json

	Reference	Proposed	Diff
performance – score	1	1	0.00
first-contentful-paint – score	1	1	0.00
first-contentful-paint – milliseconds	187.51	187.43	-0.08
first-meaningful-paint – score	1	1	0.00
first-meaningful-paint – milliseconds	446.64	440.19	-6.45
dom-size – score	1	1	0.00
dom-size – elements	344	344	0.00

highsoft-bot · 2024-05-29T09:46:24Z

Visual test results - No difference found

highsoft-bot · 2024-05-29T10:02:14Z

Benchmark report - Dashboards

benchmarks/Dashboards/DataPool-CSV-constructor.bench.ts

Sample size	This PR avg (ms)	master avg (ms)	Diff	Percent diff
2500000	0.17	0.18	0	-3%

See all

Sample size	This PR avg (ms)	master avg (ms)	Diff	Percent diff
100	0.18	0.18	0	-1%
1000	0.17	0.17	0	2%
10000	0.17	0.17	0	1%
100000	0.18	0.17	0.01	4%
1000000	0.17	0.17	0	0%
2500000	0.17	0.18	0	-3%

benchmarks/Dashboards/DataTable-loading-rows.bench.ts

Sample size	This PR avg (ms)	master avg (ms)	Diff	Percent diff
2500000	755.93	753.52	2.42	0%

See all

Sample size	This PR avg (ms)	master avg (ms)	Diff	Percent diff
100	0.32	0.29	0.04	12%
1000	0.58	0.56	0.03	5%
10000	4.94	4.02	0.91	23%
100000	22.86	23.06	-0.2	-1%
1000000	294.73	295.32	-0.59	0%
2500000	755.93	753.52	2.42	0%

benchmarks/Dashboards/DataTable-loading-columns.bench.ts

Sample size	This PR avg (ms)	master avg (ms)	Diff	Percent diff
2500000	16.34	16.22	0.12	1%

See all

Sample size	This PR avg (ms)	master avg (ms)	Diff	Percent diff
100	0.28	0.25	0.03	11%
1000	0.28	0.26	0.02	6%
10000	0.31	0.28	0.02	9%
100000	2.34	2.3	0.04	2%
1000000	9.2	9.22	-0.02	0%
2500000	16.34	16.22	0.12	1%

TorsteinHonsi · 2024-05-29T11:15:19Z

(DO NOT MERGE )

You can use draft PRs to signalize that it is a work in progress

bre1470

Nice! Looks good so far. 👍

Generating new row keys during setRows is missing.
And some things to consider below...

ts/DataGrid/DataGrid.ts

ts/Data/DataTableOptions.ts

vazonik

Wow, such a long branch name :D

Anyway, very good job! I only have a suggestion below that may slightly improve the performance.

And not so important, but for the future: branch names are included by default in messages of merge commits, so the shorter the better. Besides, most of us have a rule that PR name = branch name, I think it's worth using. (eg. dash/00000-short-description)

ts/Data/Connectors/JSONConnector.ts

bre1470

Looking into the rowKeysId filtering it seems like there are some spots missing that need awareness for the hidden column:

DataTable.deleteColumnAlias
DataTable.deleteColumns
DataTable.renameColumn
DataTable.setCell
DataTable.setColumns

ts/Data/DataTable.ts

jomar-honsi · 2024-06-19T09:08:37Z

Looking into the rowKeysId filtering it seems like there are some spots missing that need awareness for the hidden column:

DataTable.deleteColumnAlias

DataTable.deleteColumns

DataTable.renameColumn

DataTable.setCell

DataTable.setColumns

Will fix.

bre1470

Looks good! 👍

Just some nitpicking from my side...

ts/Data/DataTable.ts

jomar-honsi · 2024-06-24T18:30:04Z

Wow, such a long branch name :D

Anyway, very good job! I only have a suggestion below that may slightly improve the performance.

And not so important, but for the future: branch names are included by default in messages of merge commits, so the shorter the better. Besides, most of us have a rule that PR name = branch name, I think it's worth using. (eg. dash/00000-short-description)

I'll remember the branch length name next time.

TorsteinHonsi

Looks good to me, just added some inline nitpicking 👍

ts/Data/DataTable.ts

sebastianbochan · 2024-06-26T14:17:56Z

ts/Data/DataTable.ts

+        const id = this.rowKeysId;
+        if (id) {
+            return this.columns[id];
+        }


In this case we do not return anything.
IMO: return this.columns[id]; will return value or undefined, so the IF is unecessary.

I use the if because it gets rid of a lint error:
Type 'undefined' cannot be used as an index type.ts(2538)

Perhaps there is a more efficient solution?

sebastianbochan · 2024-06-26T14:18:07Z

ts/Data/DataTable.ts

+            const idxOrig = '' + rowKeyCol[idx];
+
+            return idxOrig.split('_')[1];
+        }


The same as above

ts/Data/DataTable.ts

sebastianbochan · 2024-06-26T14:21:13Z

ts/Data/DataTable.ts

@@ -681,6 +697,7 @@ class DataTable implements DataEvent.Emitter {

        const column = table.columns[columnNameOrAlias];

+        // eslint-disable-next-line @typescript-eslint/restrict-template-expressions


What is the reason for it? I saw above that there is a problem with the linter, but what kind of issue?

The issue is described here.

jomar-honsi · 2024-06-28T14:13:43Z

The branch an PR have been renamed.

jomar-honsi added 9 commits May 13, 2024 13:01

Demo/test DataModifiers with synchronisation.

01d08b2

Made demo more suited for debugging the core issue.

f571035

Merge

d4199f1

Updated demo.

8dc279c

Workaround for DataGrid/RangeModifier.

9e028cb

Apply workaround only to RangeModifier.

49b864c

Row index as column

35a5d14

Hide keyId column in DataGrid.

cb124b5

Used RangeModifier in examples.

2c45a7a

jomar-honsi requested review from bre1470 and vazonik May 29, 2024 09:39

jomar-honsi self-assigned this May 29, 2024

jomar-honsi linked an issue May 29, 2024 that may be closed by this pull request

Synchronization of connector values editing between components does not work correctly for connectors with modifier. #20968

Open

Fixed lint issues.

35003b5

jomar-honsi marked this pull request as draft May 30, 2024 08:53

bre1470 reviewed May 30, 2024

View reviewed changes

ts/DataGrid/DataGrid.ts Outdated Show resolved Hide resolved

ts/DataGrid/DataGrid.ts Outdated Show resolved Hide resolved

ts/Data/DataTableOptions.ts Outdated Show resolved Hide resolved

Cosmetics changes after code review + improved sample.

e740b58

vazonik requested changes May 31, 2024

View reviewed changes

ts/Data/Connectors/JSONConnector.ts Outdated Show resolved Hide resolved

jomar-honsi added 6 commits June 3, 2024 14:55

Updated after code review.

2a52952

Implemented rowkeys addition as DataTable method.

bf89945

Added Karma test.

345dae1

Added basic unit test and fixed bugs in implementation.

5d0f7d0

Added more unit tests

d369460

Added unit test for cell modification.

2a42ea0

bre1470 requested changes Jun 11, 2024

View reviewed changes

ts/Data/DataTable.ts Show resolved Hide resolved

ts/Data/DataTable.ts Outdated Show resolved Hide resolved

jomar-honsi added 3 commits June 19, 2024 19:32

Added unit tests for column manipulation operations.

abf45d5

Extended unit test to cover more table operations.

16a5192

Merge branch 'master' into 20968-synchronization-of-connector-values

cfce217

jomar-honsi requested a review from bre1470 June 20, 2024 17:08

bre1470 approved these changes Jun 22, 2024

View reviewed changes

ts/Data/DataTable.ts Outdated Show resolved Hide resolved

ts/Data/DataTable.ts Outdated Show resolved Hide resolved

Cosmetics fixed after code review.

eb9ac44

TorsteinHonsi approved these changes Jun 25, 2024

View reviewed changes

ts/Data/DataTable.ts Outdated Show resolved Hide resolved

ts/Data/DataTable.ts Outdated Show resolved Hide resolved

Fixed typos.

2596e8b

vazonik approved these changes Jun 25, 2024

View reviewed changes

Reverted accidental commit

bed7bb4

sebastianbochan requested changes Jun 26, 2024

View reviewed changes

jomar-honsi changed the title ~~20968 synchronization of connector values editing between components does not work correctly for connectors with modifier~~ 20968 synchronization of connector values Jun 26, 2024

Update after code review.

7609cc6

jomar-honsi requested a review from sebastianbochan June 26, 2024 16:34

jomar-honsi changed the title ~~20968 synchronization of connector values~~ bugfix/20968 synchronization of connector values Jun 28, 2024

jomar-honsi closed this Jun 28, 2024

jomar-honsi deleted the 20968-synchronization-of-connector-values-editing-between-components-does-not-work-correctly-for-connectors-with-modifier branch June 28, 2024 14:03

jomar-honsi restored the 20968-synchronization-of-connector-values-editing-between-components-does-not-work-correctly-for-connectors-with-modifier branch June 28, 2024 14:11

jomar-honsi reopened this Jun 28, 2024

jomar-honsi closed this Jun 28, 2024

jomar-honsi deleted the 20968-synchronization-of-connector-values-editing-between-components-does-not-work-correctly-for-connectors-with-modifier branch June 28, 2024 14:15

jomar-honsi changed the title ~~bugfix/20968 synchronization of connector values~~ bugfix/20968 synchronization of connector values - old Jun 28, 2024

jomar-honsi changed the title ~~bugfix/20968 synchronization of connector values - old~~ bugfix/20968 synchronization of connector values - old PR Jun 28, 2024

jomar-honsi restored the 20968-synchronization-of-connector-values-editing-between-components-does-not-work-correctly-for-connectors-with-modifier branch June 28, 2024 14:22

jomar-honsi deleted the 20968-synchronization-of-connector-values-editing-between-components-does-not-work-correctly-for-connectors-with-modifier branch June 28, 2024 14:31

jomar-honsi mentioned this pull request Jun 28, 2024

bugfix/20968 synchronization of connector values #21426

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix/20968 synchronization of connector values - old PR #21242

bugfix/20968 synchronization of connector values - old PR #21242

jomar-honsi commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024

highsoft-bot commented May 29, 2024 •

edited

Loading

TorsteinHonsi commented May 29, 2024

bre1470 left a comment

vazonik left a comment

bre1470 left a comment •

edited

Loading

jomar-honsi commented Jun 19, 2024

bre1470 left a comment

jomar-honsi commented Jun 24, 2024

TorsteinHonsi left a comment

sebastianbochan Jun 26, 2024

jomar-honsi Jun 26, 2024 •

edited

Loading

sebastianbochan Jun 26, 2024

sebastianbochan Jun 26, 2024

jomar-honsi Jun 26, 2024

jomar-honsi commented Jun 28, 2024

		@@ -681,6 +697,7 @@ class DataTable implements DataEvent.Emitter {

		const column = table.columns[columnNameOrAlias];

		// eslint-disable-next-line @typescript-eslint/restrict-template-expressions

bugfix/20968 synchronization of connector values - old PR #21242

bugfix/20968 synchronization of connector values - old PR #21242

Conversation

jomar-honsi commented May 29, 2024 • edited Loading

highsoft-bot commented May 29, 2024 • edited Loading

File size comparison

highsoft-bot commented May 29, 2024 • edited Loading

Dashboard visual diffs

highsoft-bot commented May 29, 2024 • edited Loading

Lighthouse report

dashboards-demo-minimal.json

highsoft-bot commented May 29, 2024

Visual test results - No difference found

highsoft-bot commented May 29, 2024 • edited Loading

Benchmark report - Dashboards

benchmarks/Dashboards/DataPool-CSV-constructor.bench.ts

benchmarks/Dashboards/DataTable-loading-rows.bench.ts

benchmarks/Dashboards/DataTable-loading-columns.bench.ts

TorsteinHonsi commented May 29, 2024

bre1470 left a comment

Choose a reason for hiding this comment

vazonik left a comment

Choose a reason for hiding this comment

bre1470 left a comment • edited Loading

Choose a reason for hiding this comment

jomar-honsi commented Jun 19, 2024

bre1470 left a comment

Choose a reason for hiding this comment

jomar-honsi commented Jun 24, 2024

TorsteinHonsi left a comment

Choose a reason for hiding this comment

sebastianbochan Jun 26, 2024

Choose a reason for hiding this comment

jomar-honsi Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

sebastianbochan Jun 26, 2024

Choose a reason for hiding this comment

sebastianbochan Jun 26, 2024

Choose a reason for hiding this comment

jomar-honsi Jun 26, 2024

Choose a reason for hiding this comment

jomar-honsi commented Jun 28, 2024

jomar-honsi commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024 •

edited

Loading

highsoft-bot commented May 29, 2024 •

edited

Loading

bre1470 left a comment •

edited

Loading

jomar-honsi Jun 26, 2024 •

edited

Loading