-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Defer rendering for inactive tabs in open mct tabbed view #7149
Conversation
…g-for-inactive-tabs-in-open-mct-tabbed-view
Codecov Report
@@ 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
*This pull request uses carry forward flags. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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 DetailsRunning Workflow e2e-couchdb on Github Actions Commit: 25c457d Started: 11/13/2023 06:26:07pm UTC Current Playwright Test Results Summary✅ 157 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)
|
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 1 • Initial Attempt |
0% (0)0 / 127 runsfailed over last 7 days |
66.14% (84)84 / 127 runsflaked over last 7 days |
This is the right approach.
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. |
…open-mct-tabbed-view
…ed-view' of github.com:nasa/openmct into 7132-defer-rendering-for-inactive-tabs-in-open-mct-tabbed-view
…g-for-inactive-tabs-in-open-mct-tabbed-view
…open-mct-tabbed-view
@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 What's the difference between an enhancement and a feature? |
There was a problem hiding this 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
…open-mct-tabbed-view
…ed-view' of github.com:nasa/openmct into 7132-defer-rendering-for-inactive-tabs-in-open-mct-tabbed-view
…open-mct-tabbed-view
…open-mct-tabbed-view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing (as always) 👍
foundObject = await this.mutablePromise; | ||
} else { | ||
foundObject = await this.openmct.objects.get(this.item.identifier); | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…open-mct-tabbed-view
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boring 😀 but will do!
Closes #7132 #7181
Describe your changes:
v-model
and instead explicitly usingvalue
and the event like every other use of the search component.All Submissions:
Author Checklist
Reviewer Checklist