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

[Telemetry Table] Address issues found during testing Table Performance #7529

Merged
merged 17 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions e2e/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,7 @@ async function setTimeConductorMode(page, isFixedTimespan = true) {
await page.getByRole('menuitem', { name: /Real-Time/ }).click();
await page.waitForURL(/tc\.mode=local/);
}
await page.getByLabel('Submit time offsets').or(page.getByLabel('Submit time bounds')).click();
}

/**
Expand Down Expand Up @@ -662,5 +663,6 @@ export {
setRealTimeMode,
setStartOffset,
setTimeConductorBounds,
setTimeConductorMode,
waitForPlotsToRender
};
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,31 @@
* at runtime from the About dialog for additional information.
*****************************************************************************/

import { createDomainObjectWithDefaults, setTimeConductorBounds } from '../../../../appActions.js';
import {
createDomainObjectWithDefaults,
setTimeConductorBounds,
setTimeConductorMode
} from '../../../../appActions.js';
import { expect, test } from '../../../../pluginFixtures.js';

test.describe('Telemetry Table', () => {
let table;
test.beforeEach(async ({ page }) => {
await page.goto('./', { waitUntil: 'domcontentloaded' });
table = await createDomainObjectWithDefaults(page, { type: 'Telemetry Table' });
});

test('Limits to 50 rows by default', async ({ page }) => {
await createDomainObjectWithDefaults(page, {
type: 'Sine Wave Generator',
parent: table.uuid
});
await page.goto(table.url);
await setTimeConductorMode(page, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine, but we should we try to avoid this appaction

const rows = page.getByLabel('table content').getByLabel('Table Row');
await expect(rows).toHaveCount(50);
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

});

test('unpauses and filters data when paused by button and user changes bounds', async ({
page
}) => {
Expand All @@ -34,7 +55,6 @@ test.describe('Telemetry Table', () => {

await page.goto('./', { waitUntil: 'domcontentloaded' });

const table = await createDomainObjectWithDefaults(page, { type: 'Telemetry Table' });
await createDomainObjectWithDefaults(page, {
type: 'Sine Wave Generator',
parent: table.uuid
Expand Down Expand Up @@ -78,7 +98,6 @@ test.describe('Telemetry Table', () => {
test('Supports filtering telemetry by regular text search', async ({ page }) => {
await page.goto('./', { waitUntil: 'domcontentloaded' });

const table = await createDomainObjectWithDefaults(page, { type: 'Telemetry Table' });
await createDomainObjectWithDefaults(page, {
type: 'Event Message Generator',
parent: table.uuid
Expand Down Expand Up @@ -121,7 +140,6 @@ test.describe('Telemetry Table', () => {
test('Supports filtering using Regex', async ({ page }) => {
await page.goto('./', { waitUntil: 'domcontentloaded' });

const table = await createDomainObjectWithDefaults(page, { type: 'Telemetry Table' });
await createDomainObjectWithDefaults(page, {
type: 'Event Message Generator',
parent: table.uuid
Expand Down
2 changes: 0 additions & 2 deletions e2e/tests/visual-a11y/imagery.visual.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,6 @@ test.describe('Visual - Example Imagery', () => {
await page.goto(exampleImagery.url, { waitUntil: 'domcontentloaded' });

await setRealTimeMode(page, true);
//Temporary to close the dialog
await page.getByLabel('Submit time offsets').click();

await expect(page.getByLabel('Image Wrapper')).toBeVisible();

Expand Down
6 changes: 5 additions & 1 deletion src/plugins/telemetryTable/TelemetryTable.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,11 @@ export default class TelemetryTable extends EventEmitter {
this.clearAndResubscribe();
}

updateRowLimit() {
updateRowLimit(rowLimit) {
if (rowLimit) {
this.rowLimit = rowLimit;
}

if (this.telemetryMode === 'performance') {
this.tableRows.setLimit(this.rowLimit);
} else {
Expand Down
18 changes: 9 additions & 9 deletions src/plugins/telemetryTable/TelemetryTableType.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
*****************************************************************************/

export default function getTelemetryTableType(options = {}) {
const { telemetryMode = 'performance', persistModeChanges = true, rowLimit = 50 } = options;
const { telemetryMode = 'performance', persistModeChange = true, rowLimit = 50 } = options;

return {
name: 'Telemetry Table',
Expand All @@ -32,12 +32,12 @@ export default function getTelemetryTableType(options = {}) {
form: [
{
key: 'telemetryMode',
name: 'Telemetry Mode',
name: 'Data Mode',
control: 'select',
options: [
{
value: 'performance',
name: 'Performance Mode'
name: 'Limited (Performance) Mode'
},
{
value: 'unlimited',
Expand All @@ -48,15 +48,15 @@ export default function getTelemetryTableType(options = {}) {
property: ['configuration', 'telemetryMode']
},
{
name: 'Persist Telemetry Mode Changes',
name: 'Persist Data Mode Changes',
control: 'toggleSwitch',
cssClass: 'l-input',
key: 'persistModeChanges',
property: ['configuration', 'persistModeChanges']
key: 'persistModeChange',
property: ['configuration', 'persistModeChange']
},
{
name: 'Performance Mode Row Limit',
control: 'toggleSwitch',
name: 'Limited Data Mode Row Limit',
control: 'numberfield',
cssClass: 'l-input',
key: 'rowLimit',
property: ['configuration', 'rowLimit']
Expand All @@ -68,7 +68,7 @@ export default function getTelemetryTableType(options = {}) {
columnWidths: {},
hiddenColumns: {},
telemetryMode,
persistModeChanges,
persistModeChange,
rowLimit
};
}
Expand Down
56 changes: 40 additions & 16 deletions src/plugins/telemetryTable/components/TableComponent.vue
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,8 @@
totalNumberOfRows: 0,
rowContext: {},
telemetryMode: configuration.telemetryMode,
persistModeChanges: configuration.persistModeChanges
persistModeChange: configuration.persistModeChange,
afterLoadActions: []
};
},
computed: {
Expand Down Expand Up @@ -458,10 +459,8 @@
},
loading: {
handler(isLoading) {
if (isLoading) {
this.setLoadingPromise();
} else {
this.loadFinishResolve();
if (!isLoading) {
this.runAfterLoadActions();
}

if (this.viewActionsCollection) {
Expand Down Expand Up @@ -538,6 +537,8 @@
this.table.on('outstanding-requests', this.outstandingRequests);
this.table.on('telemetry-staleness', this.handleStaleness);

this.table.configuration.on('change', this.handleConfigurationChanges);

this.table.tableRows.on('add', this.rowsAdded);
this.table.tableRows.on('remove', this.rowsRemoved);
this.table.tableRows.on('sort', this.throttledUpdateVisibleRows);
Expand Down Expand Up @@ -567,6 +568,8 @@
this.table.off('outstanding-requests', this.outstandingRequests);
this.table.off('telemetry-staleness', this.handleStaleness);

this.table.configuration.off('change', this.handleConfigurationChanges);

Check warning on line 571 in src/plugins/telemetryTable/components/TableComponent.vue

View check run for this annotation

Codecov / codecov/patch

src/plugins/telemetryTable/components/TableComponent.vue#L571

Added line #L571 was not covered by tests

this.table.tableRows.off('add', this.rowsAdded);
this.table.tableRows.off('remove', this.rowsRemoved);
this.table.tableRows.off('sort', this.throttledUpdateVisibleRows);
Expand All @@ -581,11 +584,34 @@
this.table.destroy();
},
methods: {
setLoadingPromise() {
this.loadFinishResolve = null;
this.isFinishedLoading = new Promise((resolve, reject) => {
this.loadFinishResolve = resolve;
});
addToAfterLoadActions(func) {
this.afterLoadActions.push(func);

Check warning on line 588 in src/plugins/telemetryTable/components/TableComponent.vue

View check run for this annotation

Codecov / codecov/patch

src/plugins/telemetryTable/components/TableComponent.vue#L588

Added line #L588 was not covered by tests
},
runAfterLoadActions() {
if (this.afterLoadActions.length > 0) {
this.afterLoadActions.forEach((action) => action());
this.afterLoadActions = [];

Check warning on line 593 in src/plugins/telemetryTable/components/TableComponent.vue

View check run for this annotation

Codecov / codecov/patch

src/plugins/telemetryTable/components/TableComponent.vue#L592-L593

Added lines #L592 - L593 were not covered by tests
}
},
handleConfigurationChanges(changes) {
const { rowLimit, telemetryMode, persistModeChange } = changes;

this.persistModeChange = persistModeChange;

if (this.rowLimit !== rowLimit) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akhenry this was previously this.rowLimit === rowLimit which was causing duplicate unsubscribes with the second unsubscribe failing because it was already removed in the realtime-provider.

this.rowLimit = rowLimit;
this.table.updateRowLimit(rowLimit);

if (this.telemetryMode !== telemetryMode) {
// need to clear and resubscribe, if different, handled below
this.table.clearAndResubscribe();

Check warning on line 607 in src/plugins/telemetryTable/components/TableComponent.vue

View check run for this annotation

Codecov / codecov/patch

src/plugins/telemetryTable/components/TableComponent.vue#L607

Added line #L607 was not covered by tests
}
}

if (this.telemetryMode !== telemetryMode) {
this.telemetryMode = telemetryMode;
this.table.updateTelemetryMode(telemetryMode);

Check warning on line 613 in src/plugins/telemetryTable/components/TableComponent.vue

View check run for this annotation

Codecov / codecov/patch

src/plugins/telemetryTable/components/TableComponent.vue#L612-L613

Added lines #L612 - L613 were not covered by tests
}
},
updateVisibleRows() {
if (!this.updatingView) {
Expand Down Expand Up @@ -1042,7 +1068,7 @@
let row = allRows[i];
row.marked = true;

if (row !== baseRow) {
if (row !== baseRow && this.markedRows.indexOf(row) === -1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we were getting dupes in markedRows which caused the count to be off

this.markedRows.push(row);
}
}
Expand Down Expand Up @@ -1166,11 +1192,9 @@
{
label,
emphasis: true,
callback: async () => {
callback: () => {
this.addToAfterLoadActions(callback);

Check warning on line 1196 in src/plugins/telemetryTable/components/TableComponent.vue

View check run for this annotation

Codecov / codecov/patch

src/plugins/telemetryTable/components/TableComponent.vue#L1196

Added line #L1196 was not covered by tests
this.updateTelemetryMode();
await this.isFinishedLoading;

callback();

dialog.dismiss();
}
Expand All @@ -1187,7 +1211,7 @@
updateTelemetryMode() {
this.telemetryMode = this.telemetryMode === 'unlimited' ? 'performance' : 'unlimited';

if (this.persistModeChanges) {
if (this.persistModeChange) {
this.table.configuration.setTelemetryMode(this.telemetryMode);
}

Expand Down
10 changes: 5 additions & 5 deletions src/plugins/telemetryTable/components/TableFooterIndicator.vue
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
:title="rowCountTitle"
class="c-table-indicator__elem c-table-indicator__row-count"
>
{{ rowCount }} Rows
{{ rowCount }}
</span>

<span
Expand Down Expand Up @@ -113,20 +113,20 @@ export default {
}
},
rowCount() {
return this.isUnlimitedMode ? this.totalRows : 'LATEST 50';
return this.isUnlimitedMode ? `${this.totalRows} ROWS` : `LATEST ${this.totalRows} ROWS`;
},
rowCountTitle() {
return this.isUnlimitedMode
? this.totalRows + ' rows visible after any filtering'
: 'performance mode limited to 50 rows';
},
telemetryModeButtonLabel() {
return this.isUnlimitedMode ? 'SHOW LATEST 50' : 'SHOW ALL';
return this.isUnlimitedMode ? 'SHOW LIMITED' : 'SHOW UNLIMITED';
},
telemetryModeButtonTitle() {
return this.isUnlimitedMode
? 'Change to Performance mode (latest 50 values)'
: 'Change to show all values';
? 'Change to Limited (Performance) Mode'
: 'Change to Unlimited Mode';
},
title() {
if (this.hasMixedFilters) {
Expand Down
4 changes: 4 additions & 0 deletions src/plugins/telemetryTable/components/TableRow.vue
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
:style="{ top: rowTop }"
class="noselect"
:class="[rowClass, { 'is-selected': marked }]"
:aria-label="ariaLabel"
v-on="listeners"
>
<component
Expand Down Expand Up @@ -99,6 +100,9 @@ export default {
};
},
computed: {
ariaLabel() {
return this.marked ? 'Selected Table Row' : 'Table Row';
},
listeners() {
let listenersObject = {
click: this.markRow
Expand Down
12 changes: 7 additions & 5 deletions src/styles/_controls.scss
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,9 @@ select {
color: $colorSelectFg;
box-shadow: $shdwSelect;
background-repeat: no-repeat, no-repeat;
background-position: right 0.4em top 80%, 0 0;
background-position:
right 0.4em top 80%,
0 0;
border: none;
border-radius: $controlCr;
padding: 2px 20px 2px $interiorMargin;
Expand Down Expand Up @@ -718,15 +720,15 @@ select {
.c-super-menu__item-description {
flex: 1 1 70%;

[class*="__icon"] {
[class*='__icon'] {
display: none !important;
}

[class*="__name"] {
[class*='__name'] {
margin-top: 0 !important;
}

[class*="__item-description"] {
[class*='__item-description'] {
min-width: 200px;
}
}
Expand Down Expand Up @@ -1133,7 +1135,7 @@ input[type='range'] {
// Hidden by default; requires a hover 1 - 3 levels above to display
@include transition(opacity, $transOutTime);
opacity: 0;
pointer-events: none;
pointer-events: auto;
}
}

Expand Down
Loading