-
Notifications
You must be signed in to change notification settings - Fork 21
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
Render new trace server connection status bar item #120
Render new trace server connection status bar item #120
Conversation
I going to review this today. |
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 noticed that when TSP queries in the trace panel fail due to non-connected server, the status in s not updated.
With this current approach by polling in the provider only certain events will trigger that code path and the server status is not updated.
I think you should be able to use the RestClient.addConnectionStatusListener
. Each webview has an instance of the tspclient and also access the static connection listener. When sending a tsp message then it the listener implementation can notify via a message it's vscode extension counterpart and update the status line. I think if you add it to the common class TspClientProvider
then you can share the code. Note that the class
TraceViewerContainer doesn't use the TspClientProvider
class, but you can change that at the same time to be able to use the connection listener implementation.
Signed-off-by: Marco Miller <marco.miller@ericsson.com>
Signed-off-by: Marco Miller <marco.miller@ericsson.com>
Signed-off-by: Marco Miller <marco.miller@ericsson.com>
Add a contextual (secondary, right-side) VS Code status bar item, that renders the current trace server status. Have the extension context pass this status service to each active web-view provider, for them to update the rendered status upon every related message from the view. Have TraceExplorerOpenedTracesViewProvider pass its status service to the TraceViewerPanel upon using it, in turn. Without this, there are instances where the latest server status isn't shown as expected. Keep this undefined in trace-tree.ts where that is not required. This makes the server status listener-based. Add this connection listener through the TspClientProvider constructor, thus consistently use the latter across. This means also using it in the TraceViewerContainer constructor, instead of TspClient's directly prior. Properly 'stringify' (thus parse) the boolean status value, as passing it straight would break the JSON message otherwise. Explicitly defer showing the item to only once Trace Viewer gets selected for the first time. Like open trace-explorer views which remain shown if unselecting Trace Viewer, keep showing the status item from there on. The VS Code API, anyway, doesn't hold an event that could selectively hide anything upon solely exiting Trace Viewer. Prefer a standard VS Code status bar item to the side widget approach of theia-trace-extension. This should be more standard for such a connection status display, from a VS Code UI (and UX) perspective. This also allows not having to implement a custom widget that differs from the Trace Viewer panels. -The latter being meant to render real views. Base this simple design of the status bar item on [1] below. Now, use the warning background for a found server, for such a positive status to be highlighted, thus noticeable. Use the usual red background for a trace server that is not found, highlighting that as an error. No other color can or should be used anyway for status bar items, per [1]. The latter reference also includes [2]'s, used to come up with this change. [1]https://code.visualstudio.com/api/ux-guidelines/status-bar [2]https://github.com/microsoft/vscode-extension-samples/blob/main/statusbar-sample/src/extension.ts Use a short (thus usable) enough statusBarItem.text, along with a tooltip to complement the trace server status narrative. Overall, with this approach, defer the introduction of the inversify module, used in theia-trace-extension. This would be to inject instances such as objects involved in this change and likely other ones; some being potential singletons. Slightly refactor a few surrounding blank lines while editing some of these files. Fixes #33. Signed-off-by: Marco Miller <marco.miller@ericsson.com>
This PR is ready for review again; thanks. |
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.
Thanks for the updates. A few more small things.
...ension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts
Show resolved
Hide resolved
...-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts
Show resolved
Hide resolved
vscode-trace-extension/src/trace-viewer-panel/trace-viewer-webview-panel.ts
Show resolved
Hide resolved
...ension/src/trace-explorer/available-views/trace-explorer-available-views-webview-provider.ts
Show resolved
Hide resolved
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.
LGTM. Thanks for the contribution.
Add a contextual (secondary, right-side) VS Code status bar item, that renders the current trace server status. Have the extension context pass this status service to each active web-view provider, for them to update the rendered status upon every related message from the view.
Have
TraceExplorerOpenedTracesViewProvider
pass its status service to theTraceViewerPanel
upon using it, in turn. Without this, there are instances where the latest server status isn't shown as expected. Keep this undefined intrace-tree.ts
where that is not required.This makes the server status listener-based. Add this connection listener through the
TspClientProvider
constructor, thus consistently use the latter across. This means also using it in theTraceViewerContainer
constructor, instead ofTspClient
's directly prior.Properly
stringify
(thus parse) the boolean status value, as passing it straight would break the JSON message otherwise.Explicitly defer showing the item to only once Trace Viewer gets selected for the first time. Like open trace-explorer views which remain shown if unselecting Trace Viewer, keep showing the status item from there on. The VS Code API, anyway, doesn't hold an event that could selectively hide anything upon solely exiting Trace Viewer.
Prefer a standard VS Code status bar item to the side widget approach of
theia-trace-extension
. This should be more standard for such a connection status display, from a VS Code UI (and UX) perspective. This also allows not having to implement a custom widget that differs from the Trace Viewer panels. -The latter being meant to render real views.Base this simple design of the status bar item on [1] below. Now, use the warning background for a found server, for such a positive status to be highlighted, thus noticeable. Use the usual red background for a trace server that is not found, highlighting that as an error. No other color can or should be used anyway for status bar items, per [1]. The latter reference also includes [2]'s, used to come up with this change.
[1]https://code.visualstudio.com/api/ux-guidelines/status-bar
[2]https://github.com/microsoft/vscode-extension-samples/blob/main/statusbar-sample/src/extension.ts
Use a short (thus usable) enough
statusBarItem.text
, along with atooltip
to complement the trace server status narrative.Overall, with this approach, defer the introduction of the
inversify
module, used intheia-trace-extension
. This would be to inject instances such as objects involved in this change and likely other ones; some being potential singletons.Slightly refactor a few surrounding blank lines while editing some of these files.
Fixes #33.
Signed-off-by: Marco Miller marco.miller@ericsson.com