Split viewer-prototype in packages #172

tahini · 2020-10-26T20:28:01Z

In prevision of the development of a vscode extension, the code in the
viewer-prototype has been mostly moved to 2 new packages:

@trace-viewer/base: Queries the trace server for traces, experiments
and possible outputs
@trace-viewer/react-components: Contains the various types of
visualization for trace data, queries the trace server for output data
to display

Those 2 packages can eventually be published to npm.

In the viewer-prototype directory only remains the theia specific module
contributions. So the application still works as before using theia,
it's just that most of the classes used by the trace-viewer have been
moved to new npm packages.

Signed-off-by: Geneviève Bastien gbastien@versatic.net

tahini · 2020-10-26T20:31:36Z

This PR is replacing PR #124. This one does not change our way of working right now. The theia browser and electron apps still work, but a good part of the code is moved to 2 packages that will be published on npm and consumed by the vscode and theia extension.

Thus this is a smaller bite to swallow on our way to a vscode extension.

marcdumais-work · 2020-10-27T00:23:48Z

I have not opened this repo in Gitpod for a while, so I do not know if this is new, but I notice the readability of the text is poor, in the example Electron application, accessed through vnc:

I found a VNC setting that fixes it: scaling mode: remote resizing instead of local scaling

marcdumais-work · 2020-10-27T00:52:53Z

When I briefly tested, I could get almost no graphs to display - maybe a single line for CPU usage view. Could be related to this backend trace, or not. Starting fresh in another container, that time I seem to crash it playing with the trace views. Not sure what happened exactly but the backend became unreachable. Maybe just bad luck. But I see kind of crazy amount of CPU being used, with one trace open and only the non-working CPU usage view. Like half to three quarters of the CPU power of the 16 cores of that host. Likely my container was killed because of too much resource usage.

Like before, I do not know if this is new or the same would happen on master.

root WARN Provided WebGL context does not have a stencil buffer, masks may not render correctly
root INFO 
    PixiJS 5.3.3 - ✰ WebGL 2 ✰      http://www.pixijs.com/    ♥♥♥ 


root WARN Provided WebGL context

React is apparently not happy about something. Not sure if important:

root WARN Warning: componentWillReceiveProps has been renamed, and is not recommended for use. See https://fb.me/react-unsafe-component-lifecycles for details.

* Move data fetching code or side effects to componentDidUpdate.
* If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://fb.me/react-derived-state
* Rename componentWillReceiveProps to UNSAFE_componentWillReceiveProps to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

Please update the following components: ReactGridLayout, ResponsiveReactGridLayout

tahini · 2020-10-27T18:12:20Z

@marcdumais-work Thanks for trying it. You're talking about gitpod only? Or did you try it somewhere else more local to you? I'll investigate, try to see if this is new from this PR or if it was there now in master.

marcdumais-work

LGTM, thanks @tahini . I will let others review the TypeScript code changes, but the split and configs look good. I briefly tried on Gitpod and there is high CPU usage and other issues, but apparently already in master.

marcdumais-work · 2020-10-27T19:07:02Z

It will be very nice when we can use the browser version of your app, in Gitpod. All these GPU problems go away then.

marcdumais-work · 2020-10-27T20:08:10Z

You're talking about gitpod only?

Correct. Most likely it will be better locally, but I'll let others confirm for now

bhufmann

All the features work fine. Only comments on the tooltips. Please let me know what you think for that.

bhufmann · 2020-10-29T19:47:37Z

README.md

@@ -98,7 +98,3 @@ Now, you will see 3 sections: **Opened experiments**, **Available analysis** and

 ### Navigation and zooming
 There is only a limited number of such operations and they are only implemented in the Time Graph views (the ones looking like Gantt charts). For Zoom-in/out use CTRL+mouse wheel. Or use left mouse drag on time axis on top. Navigating the trace you can use the scrollbar at the bottom of the experiment container.
-
-### Time Graph Tooltip


I still see the Tooltip window in the trace viewer, but it's not populated anymore. I wonder if the removal of that functionality should be in this patch or separate. Maybe, it should be done in a separate PR so that this PR is only about the restructuring.

I didn't figure out yet about how to send tooltip signals from the react-component. Or actually, I did think of something, but wondered if it was a good idea to keep it or not, or if it should be another kind of signal (like in the Properties, so that whichever element is selected, we can display stuff in the window we now call Tooltip. Should I bring back the tooltip functionality now?

This should be put back, right?

bhufmann · 2020-10-29T19:49:54Z

viewer-prototype/src/browser/trace-explorer/trace-explorer-widget.tsx

@@ -63,9 +63,8 @@ export class TraceExplorerWidget extends ReactWidget {
        this.title.label = TRACE_EXPLORER_LABEL;
        this.title.caption = TRACE_EXPLORER_LABEL;
        this.title.iconClass = 'trace-explorer-tab-icon';
-        this.toDispose.push(experimentManager.experimentOpenedSignal(experiment => this.onExperimentOpened(experiment)));
-        this.toDispose.push(experimentManager.experimentClosedSignal(experiment => this.onExperimentClosed(experiment)));
-        this.toDispose.push(SignalManager.getInstance().tooltipSignal(tooltip => this.onTooltip(tooltip)));


related to comment about tooltips

bhufmann · 2020-10-29T19:53:17Z

viewer-prototype/src/browser/trace-explorer/trace-explorer-widget.tsx

-        this.tooltip = tooltip;
-        this.update();
-    }
-


viewer-prototype/src/browser/trace-viewer/trace-viewer.tsx

tahini · 2020-11-03T22:35:01Z

@bhufmann I'm waiting for your feedback on this patch.

Would you rather
1- I do something custom to keep the tooltip for now, or
2- wait for a later patch where we can make the react-components dispatch , including tooltip, to ?

bhufmann · 2020-11-06T19:52:21Z

@bhufmann I'm waiting for your feedback on this patch.

Would you rather
1- I do something custom to keep the tooltip for now, or
2- wait for a later patch where we can make the react-components dispatch , including tooltip, to ?

@tahini, #170 implements a tooltip pop-up window and it doesn't need the signal (I think). So, I don't think we need a 1). Later I will see that we need for other purposes a way to communicate between the VSCode extension and the react-components.

bhufmann

Looks good to me. Before merging I'd like to coordinate with the team for this.

tahini · 2020-11-09T21:48:22Z

Let me know when I can merge this, I just rebased on latest master, including the experiment sync.

bhufmann · 2020-11-13T14:48:21Z

Let me know when I can merge this, I just rebased on latest master, including the experiment sync.

@tahini #170 won't be ready for the MVP. So, this split patch would remove the tooltip window that shows the state tooltip. I think we should have the original tooltip behaviour for the MVP. So, we have 2 options:

don't merge this patch before the MVP
update this patch to bring back the original tooltip functionality.

Option 1) is the safe way for the MVP. However, option 2) would ok too, if the change doesn't take too long to implement. For option 2) we would have a minor update on our internal integration build.

What do you think?

tahini · 2020-11-13T19:18:37Z

I'll update this patch and bring back tooltip. I'd rather have this in sooner, it's a pain to rebase, especially with features that involve interactions between trace explorer and trace viewer.

It shouldn't impact your internal MVP too much, especially that the packages are still in the same repo, no?

bhufmann · 2020-11-13T19:24:06Z

I'll update this patch and bring back tooltip. I'd rather have this in sooner, it's a pain to rebase, especially with features that involve interactions between trace explorer and trace viewer.

It shouldn't impact your internal MVP too much, especially that the packages are still in the same repo, no?

The only impact I think is to have both the base and react-component packages published in our registry and reference them in our package.json. Should be straight forward.

Thanks for bringing the tooltip back in this PR.

tahini · 2020-11-16T21:55:27Z

The tooltips are back

bhufmann

It works fine. Please keep the tooltip in the README.md as well

bhufmann · 2020-11-17T20:48:57Z

README.md

@@ -98,7 +98,3 @@ Now, you will see 3 sections: **Opened experiments**, **Available analysis** and

 ### Navigation and zooming
 There is only a limited number of such operations and they are only implemented in the Time Graph views (the ones looking like Gantt charts). For Zoom-in/out use CTRL+mouse wheel. Or use left mouse drag on time axis on top. Navigating the trace you can use the scrollbar at the bottom of the experiment container.
-
-### Time Graph Tooltip


This should be put back, right?

In prevision of the development of a vscode extension, the code in the viewer-prototype has been mostly moved to 2 new packages: * @trace-viewer/base: Queries the trace server for traces, experiments and possible outputs * @trace-viewer/react-components: Contains the various types of visualization for trace data, queries the trace server for output data to display Those 2 packages can eventually be published to npm. In the viewer-prototype directory only remains the theia specific module contributions. So the application still works as before using theia, it's just that most of the classes used by the trace-viewer have been moved to new npm packages. Signed-off-by: Geneviève Bastien <gbastien@versatic.net>

bhufmann

Looks good to me. Thanks for this restructuring patch.

bhufmann · 2020-11-19T16:04:19Z

@tahini, patch is good to be merged

tahini · 2020-11-19T17:45:51Z

Thanks!

tahini requested review from bhufmann, MatthewKhouzam and marcdumais-work October 26, 2020 20:28

tahini mentioned this pull request Oct 26, 2020

Vscode migration #124

Closed

marcdumais-work approved these changes Oct 27, 2020

View reviewed changes

tahini force-pushed the split_packages branch from d2b2c4e to 879b1a1 Compare October 27, 2020 19:09

bhufmann requested changes Oct 29, 2020

View reviewed changes

tahini force-pushed the split_packages branch from 879b1a1 to 37a972b Compare October 30, 2020 17:36

tahini requested a review from bhufmann November 5, 2020 20:11

bhufmann approved these changes Nov 6, 2020

View reviewed changes

tahini force-pushed the split_packages branch from 37a972b to 61aeb5f Compare November 9, 2020 21:44

tahini force-pushed the split_packages branch from 61aeb5f to 0849f00 Compare November 16, 2020 21:54

tahini requested a review from bhufmann November 16, 2020 21:55

bhufmann requested changes Nov 17, 2020

View reviewed changes

tahini force-pushed the split_packages branch from 0849f00 to e3668cf Compare November 19, 2020 13:36

tahini requested a review from bhufmann November 19, 2020 13:38

bhufmann approved these changes Nov 19, 2020

View reviewed changes

tahini merged commit 6fdad14 into eclipse-cdt-cloud:master Nov 19, 2020

tahini deleted the split_packages branch November 19, 2020 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split viewer-prototype in packages #172

Split viewer-prototype in packages #172

tahini commented Oct 26, 2020

tahini commented Oct 26, 2020

marcdumais-work commented Oct 27, 2020

marcdumais-work commented Oct 27, 2020 •

edited

Loading

tahini commented Oct 27, 2020

marcdumais-work left a comment

marcdumais-work commented Oct 27, 2020

marcdumais-work commented Oct 27, 2020

bhufmann left a comment •

edited

Loading

bhufmann Oct 29, 2020

tahini Oct 30, 2020

bhufmann Nov 17, 2020

bhufmann Oct 29, 2020

bhufmann Oct 29, 2020

tahini commented Nov 3, 2020

bhufmann commented Nov 6, 2020

bhufmann left a comment

tahini commented Nov 9, 2020

bhufmann commented Nov 13, 2020

tahini commented Nov 13, 2020

bhufmann commented Nov 13, 2020

tahini commented Nov 16, 2020

bhufmann left a comment

bhufmann Nov 17, 2020

bhufmann left a comment

bhufmann commented Nov 19, 2020

tahini commented Nov 19, 2020

Split viewer-prototype in packages #172

Split viewer-prototype in packages #172

Conversation

tahini commented Oct 26, 2020

tahini commented Oct 26, 2020

marcdumais-work commented Oct 27, 2020

marcdumais-work commented Oct 27, 2020 • edited Loading

tahini commented Oct 27, 2020

marcdumais-work left a comment

Choose a reason for hiding this comment

marcdumais-work commented Oct 27, 2020

marcdumais-work commented Oct 27, 2020

bhufmann left a comment • edited Loading

Choose a reason for hiding this comment

bhufmann Oct 29, 2020

Choose a reason for hiding this comment

tahini Oct 30, 2020

Choose a reason for hiding this comment

bhufmann Nov 17, 2020

Choose a reason for hiding this comment

bhufmann Oct 29, 2020

Choose a reason for hiding this comment

bhufmann Oct 29, 2020

Choose a reason for hiding this comment

tahini commented Nov 3, 2020

bhufmann commented Nov 6, 2020

bhufmann left a comment

Choose a reason for hiding this comment

tahini commented Nov 9, 2020

bhufmann commented Nov 13, 2020

tahini commented Nov 13, 2020

bhufmann commented Nov 13, 2020

tahini commented Nov 16, 2020

bhufmann left a comment

Choose a reason for hiding this comment

bhufmann Nov 17, 2020

Choose a reason for hiding this comment

bhufmann left a comment

Choose a reason for hiding this comment

bhufmann commented Nov 19, 2020

tahini commented Nov 19, 2020

marcdumais-work commented Oct 27, 2020 •

edited

Loading

bhufmann left a comment •

edited

Loading