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

Add parameter to EditingTarget.onActivated indicating whether it's being activated for user interaction #6523

Merged
merged 5 commits into from Mar 25, 2020

Conversation

jjallaire
Copy link
Member

@jjallaire jjallaire commented Mar 25, 2020

This PR adds an argument to the EditingTarget.onActivated() method that indicates whether the editor tab is being activated for the purpose of user interaction (during startup onActivated() is called for each tab being restored irrespective of whether it will actually end up active at the end of startup).

The motivation is that panmirror does some potentially expensive (depending on the platform) work at startup (calls pandoc twice). On Linux/OSX these calls take sub 10ms however on Windows they can take 100ms+. If a user has dozens of tabs w/ panmirror to be restored, the cost of doing panmirror initialization eagerly could become prohibitive. With this change we can defer this work until the user is actually going to interact with the document.

I could have "fixed" onActivated() to not be called during IDE tabset restoration, however I'm sure we have implicit dependencies on this elsewhere and unwinding them seems risky / cost prohibitive.

@jjallaire jjallaire requested a review from jmcphers March 25, 2020 15:39
@jjallaire jjallaire changed the title Provide onActivatedForUser method in editing target Add parameter to EditingTarget.onActivated indicating whether it's being activated for user interaction Mar 25, 2020
Copy link
Member

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

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

LGTM! I think in other situations we deal with this on focus (which IIRC doesn't happen during startup) so might be worth exploring if this doesn't behave as you expect.

@jmcphers jmcphers merged commit 7c369ad into master Mar 25, 2020
@jjallaire
Copy link
Member Author

Yeah, I thought of focus but in this case the editor component is created on demand and then focused, so I need to know about the activation a bit earlier in the control flow.

@jjallaire jjallaire deleted the feature/on-activated-for-user branch April 3, 2020 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants