Fix trace explorer / welcome page refresh corner cases #230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bhufmann
force-pushed
the
improve_refresh_detailed
branch
from
306f539
to
901c9b2
Compare
|
Video showing fix for #227:
|
|
Video showing fix for #228
|
|
Video showing fix for #229
|
bhufmann
force-pushed
the
improve_refresh_detailed
branch
from
901c9b2
to
0b9403e
Compare
|
@williamsyang-work I found some corner cases for the refresh of the trace-explorer/welcome-view that this PR addresses. Please let me know if have any comments for this PR. |
bhufmann
force-pushed
the
improve_refresh_detailed
branch
from
0b9403e
to
3655854
Compare
Comment on lines
20
to
27
|
|
||
| RestClient.addConnectionStatusListener(status => { | ||
| // Ignore the first update that is sent when calling addConnectionStatusListener | ||
| if (this._initialized) { | ||
| this._signalHandler?.notifyConnection(status); | ||
| } | ||
| }); | ||
| this._initialized = true; |
There was a problem hiding this comment.
Similar pattern to this: f565c23.
I wonder if there's a cleaner way to do this.
There was a problem hiding this comment.
@williamsyang-work I'm not sure what you mean. What do you suggest to make it cleaner? Please let me know
There was a problem hiding this comment.
I do like the implementation that William points-out slightly better. However this is simple enough that I don't mind either way.
There was a problem hiding this comment.
Ok, I think I understand how to change it. I'll update the PR (including a rebase).
|
Thanks for the assist on this @bhufmann :) |
marcdumais-work
approved these changes
Apr 10, 2024
Call it from the webview dispose handler. Sub-class can add their own implementation and call super implementation. This avoids that the class is using the webview instance after it is disposed. Signed-off-by: Bernd Hufmann <[email protected]>
The TSP client implementation notifies clients when the method addConnectionStatusListener() is called. This can lead to incorrect state in on client side. Signed-off-by: Bernd Hufmann <[email protected]>
This method set the status bar and the serverUp context that is used to decide whether the trace explorer or the welcome view should be rendered. This method doesn't check the backend to get status in comparison to checkAndUpdateServerStatus. It's useful when it's known that the server is down or up (e.g. after start notification). In this case, it is not necessary to check the backend again. Don't set the status in the method isTraceServerUp() because it should not have side-effects that are not expected. Initialize and refresh flags traceViewer.serverUp and trace-explorer.noExperiments to ensure the correct view is rendered, i.e. Trace Explorer or Welcome view, when certain event happen (e.g. server started or stopped). Fixes eclipse-cdt-cloud#227 Fixes eclipse-cdt-cloud#228 Signed-off-by: Bernd Hufmann <[email protected]>
For the case that with current URL the welcome view is rendered due to noExperiment or server is down, this will render the trace explorer (or welcome view) after a server URL change. Without calling refresh, the welcome view stays, even if the other server has experiments. Fixes eclipse-cdt-cloud#229 Signed-off-by: Bernd Hufmann <[email protected]>
bhufmann
force-pushed
the
improve_refresh_detailed
branch
from
3655854
to
1f6b33a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR consists of multiple commits that fixes refresh corner cases.
Add default dispose() method to AbstractTraceExplorerProvider
Call it from the webview dispose handler. Sub-class can add their own implementation and call super implementation. This avoids that the class is using the webview instance after it is disposed.
Ignore connection status listener notification in constructor
The TSP client implementation notifies clients when the method
addConnectionStatusListener()is called. This can lead to incorrect state in on client side.Add method to set server status in TraceServerStatusService
This method set the status bar and the
traceViewer.serverUpcontext that is used to decide whether the trace explorer or the welcome view should be rendered. This method doesn't check the backend to get status in comparison tocheckAndUpdateServerStatus. It's useful when it's known that the server is down or up (e.g. after start notification). In this case, it is not necessary to check the backend again. Don't set the status in the methodisTraceServerUp()because it should not have side-effects that are not expected. Initialize and refresh flagstraceViewer.serverUpandtrace-explorer.noExperimentsto ensure the correct view is rendered, i.e. Trace Explorer or Welcome view, when certain events happen (e.g. server started or stopped).Fixes Trace Explorer not refreshed after first trace server started indication #227
Fixes Welcome Page not rendered after trace server stopped indication (intermittent) #228.
Refresh trace viewer after server URL preference changes
For the case that with current URL the welcome view is rendered due to no experiments on server, or the server is down, this will render the trace explorer (or welcome view) after a server URL change. Without calling refresh, the welcome view stays, even if the other server has experiments.
Fixes Trace Explorer/Welcome page not refreshed when updating the server URL (corner case) #229
Signed-off-by: Bernd Hufmann [email protected]