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

Defer rendering for inactive tabs in open mct tabbed view #7149

Merged

Conversation

scottbell
Copy link
Contributor

@scottbell scottbell commented Oct 19, 2023

Closes #7132 #7181

Describe your changes:

  • Add an API for nicely deferring rendering until we're visible.
  • Fixed issue with table column filters not working by removing the v-model and instead explicitly using value and the event like every other use of the search component.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #7149 (25c457d) into master (29b7c38) will decrease coverage by 0.24%.
The diff coverage is 73.80%.

@@            Coverage Diff             @@
##           master    #7149      +/-   ##
==========================================
- Coverage   56.13%   55.89%   -0.24%     
==========================================
  Files         652      653       +1     
  Lines       26178    26213      +35     
  Branches     2525     2525              
==========================================
- Hits        14694    14651      -43     
- Misses      10787    10856      +69     
- Partials      697      706       +9     
Flag Coverage Δ *Carryforward flag
e2e-full 41.84% <ø> (ø) Carriedforward from 29b7c38
e2e-stable 58.10% <100.00%> (+0.02%) ⬆️
unit 49.26% <66.66%> (-0.25%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
src/api/nice/NicelyCalled.js 100.00% <100.00%> (ø)
src/plugins/gauge/components/GaugeComponent.vue 61.93% <100.00%> (-0.36%) ⬇️
src/plugins/tabs/components/TabsComponent.vue 36.84% <ø> (ø)
src/plugins/LADTable/components/LadRow.vue 52.52% <0.00%> (-0.54%) ⬇️
src/plugins/plot/chart/MctChart.vue 43.82% <66.66%> (+0.26%) ⬆️
...ugins/telemetryTable/components/TableComponent.vue 41.84% <71.42%> (-0.35%) ⬇️
...plugins/displayLayout/components/TelemetryView.vue 2.38% <0.00%> (-0.10%) ⬇️

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29b7c38...25c457d. Read the comment docs.

@deploysentinel
Copy link

deploysentinel bot commented Oct 19, 2023

Current Playwright Test Results Summary

✅ 14 Passing

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 11/13/2023 06:27:37pm UTC)

Run Details

Running Workflow e2e-couchdb on Github Actions

Commit: 25c457d

Started: 11/13/2023 06:26:07pm UTC

View Detailed Build Results


Current Playwright Test Results Summary

✅ 157 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 11/13/2023 06:27:37pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: 25c457d

Started: 11/13/2023 05:53:12pm UTC

⚠️ Flakes

📄   functional/plugins/notebook/notebookSnapshots.e2e.spec.js • 1 Flake

Test Case Results

Test Case Last 7 days Failures Last 7 days Flakes
Snapshot image tests Can drop an image onto a notebook and create a new entry
Retry 1Initial Attempt
0% (0) 0 / 127 runs
failed over last 7 days
66.14% (84) 84 / 127 runs
flaked over last 7 days

View Detailed Build Results


@akhenry
Copy link
Contributor

akhenry commented Oct 20, 2023

The requestAnimationFrame fires everything that's passed to it. We're not doing this, and instead just remembering the last non-fired function, and firing that when becoming visible. Is that good enough? Or should we have an option to queue the un-fired stuff?

This is the right approach.

For the WebGL stuff, I haven't implemented it yet, but was looking at add it to the DrawWebGL context to separately listen to the canvas intersection with the viewport. Is that acceptable?

Check in with @shefalijoshi for specifics. The Plots have logic for falling back on Canvas2D rendering when a WebGL context is lost which we should be able to use here. I don't know if we have logic for going the opposite direction though. When a plot switches from inactive to active, we should have it try and get a WebGL context. This way we can use all of our available WebGL contexts in the active tab rather than wasting them in inactive tabs.

@scottbell
Copy link
Contributor Author

scottbell commented Oct 26, 2023

@akhenry I'm going to split the WebGL stuff into another branch. The WebGL context loss/gain is a bit involved, and also somewhat browser dependent. Some browsers use it for testing of a synthetic context lost for testing (e.g., Webkit and sometimes Chrome), and some use it to initiate context loss (e.g., Firefox).

@unlikelyzero unlikelyzero added performance impacts or improves performance type:feature Feature. Required intentional design labels Oct 26, 2023
@scottbell
Copy link
Contributor Author

@unlikelyzero What's the difference between an enhancement and a feature?

Copy link
Contributor

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Really great work here. I just have some minor change requests

…ed-view' of github.com:nasa/openmct into 7132-defer-rendering-for-inactive-tabs-in-open-mct-tabbed-view
@scottbell scottbell requested a review from ozyx November 7, 2023 18:28
@scottbell scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 7, 2023
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 7, 2023
…ed-view' of github.com:nasa/openmct into 7132-defer-rendering-for-inactive-tabs-in-open-mct-tabbed-view
@scottbell scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 7, 2023
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 7, 2023
@scottbell scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 8, 2023
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 8, 2023
@scottbell scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 9, 2023
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 9, 2023
Copy link
Contributor

@unlikelyzero unlikelyzero left a comment

Choose a reason for hiding this comment

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

Amazing (as always) 👍

@unlikelyzero unlikelyzero modified the milestones: Target:3.3.0, Target:3.2.0 Nov 9, 2023
foundObject = await this.mutablePromise;
} else {
foundObject = await this.openmct.objects.get(this.item.identifier);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@scottbell any idea why this isn't being called in our tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think because the objects we're dealing with support mutation.

@ozyx ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 13, 2023
@ozyx ozyx enabled auto-merge (squash) November 13, 2023 18:23
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Nov 13, 2023
@ozyx ozyx merged commit deacd91 into master Nov 13, 2023
@ozyx ozyx deleted the 7132-defer-rendering-for-inactive-tabs-in-open-mct-tabbed-view branch November 13, 2023 18:27
Copy link
Contributor

@akhenry akhenry left a comment

Choose a reason for hiding this comment

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

This looks really awesome, and from all accounts makes a big difference to rendering performance.

I'd actually love to make this API so that third parties can build performant Open MCT components. I've filed a followup for promoting this to API - #7235

* @returns {boolean} True if the function was executed immediately, false otherwise.
*/
execute(func) {
if (this.#isIntersecting) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How is initial state established here? From my reading of the API the IntersectionObserver detects changes in inersection, but how do we know whether or not the element is intersecting when we create the observer?

Copy link
Contributor Author

@scottbell scottbell Nov 14, 2023

Choose a reason for hiding this comment

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

We assume it's visible until the IntersectionObserver tells us otherwise. The IntersectionObserver, upon registering, will eventually tell us via callback its initial state (see second bullet):

The Intersection Observer API allows you to configure a callback that is called when either of these circumstances occur:
* A target element intersects either the device's viewport or a specified element. That specified element is called the root element or root for the purposes of the Intersection Observer API.
* The first time the observer is initially asked to watch a target element.

so we'll eventually know whether it's visible or not, and stop future calls to requestAnimationFrame.

/**
* Optimizes `requestAnimationFrame` calls to only execute when the element is visible in the viewport.
*/
export default class NicelyCalled {
Copy link
Contributor

Choose a reason for hiding this comment

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

As much as I love this name, is there a more descriptive name we could give it? Maybe "VisibilityObserver" with a function called renderWhenVisible or something?

Copy link
Contributor Author

@scottbell scottbell Nov 14, 2023

Choose a reason for hiding this comment

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

Boring 😀 but will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance impacts or improves performance type:feature Feature. Required intentional design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TelemetryTable filters are broken Defer rendering for inactive tabs in Open MCT Tabbed View
4 participants