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

Fix issue #265 #267

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Conversation

GregSavin
Copy link
Contributor

Wait for fileOpenHandler to finish, before refreshing the trace explorer context.

Wait for fileOpenHandler to finish, before refreshing the trace explorer
context.

Signed-off-by: Greg Savin <[email protected]>
@bhufmann
Copy link
Collaborator

Thanks for the contribution. In order to merge this PR you need to sign the ECA as described in the contribution guide. Please follow the instructions there. Thanks.

@marcdumais-work
Copy link
Contributor

Thanks for the contribution. In order to merge this PR you need to sign the ECA as described in the contribution guide. Please follow the instructions there. Thanks.

The ECA check now passes

@GregSavin
Copy link
Contributor Author

Thanks, yes I signed the ECA yesterday after first seeing the check not passing; glad that the check is now passing.

@marcdumais-work
Copy link
Contributor

@bhufmann
There is another similar case of a call to fileOpenHandler() without an "await", a bit down in the code in extension.ts:
https://github.com/eclipse-cdt-cloud/vscode-trace-extension/pull/267/files#diff-4decf63ca1dab51f824c7ff2fd1ffb65bc509e91da293715b643854255f7d76aR170

That one seems used when using the "Open Trace" command. That one seems to work correctly, but I';m not sure why?

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

The code change is small and elegant! I tested with my own extension that contributes a custom command that executes command "traces.openTraceFile" to open a trace file I have locally. I also tested without an extension, as suggested by @bhufmann in the comments on the corresponding issue. In both cases I was able reproduce the issue using the latest master branch, and in both cases, this PR branch fixed the problem.

Thanks @GregSavin for this contribution ! I believe we will need a second committer approval before we can merge.

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me, too. Thanks!

@bhufmann bhufmann merged commit bb41548 into eclipse-cdt-cloud:master Aug 28, 2024
6 checks passed
@marcdumais-work
Copy link
Contributor

@GregSavin I'm not sure if you consume this extension from the VS Marketplace and/or openvsx, but FYI I think we will likely do a release soon that includes your fix. Probably after the following PR is merged:
#262

@GregSavin
Copy link
Contributor Author

Thanks all!

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.

3 participants