Migrate vscode-webview-ui-toolkit to primereact

[haydar-metin]

What it does

Migrates the vscode-webkit-toolkit to primereact.

DataTable

• It is possible to select a single cell (currently, it only highlights it - will be reset in specific cases)
• It is possible to resize the column width

Options

• The input fields submit on Enter
• The framework formik has been used for better handling of input fields. (Side note: This framework is also recommended by primereact)
• Input is validated
• The form fields wrap now if there is not enough width (it is responsive)

Misc

• Loading spinner is shown if data is fetched in the background
• The theme is custom.
  • The approach is described in the documentations
  • The colors will update in accordance with the CSS values provided by the vscode host

Bug Fixes

• Columns are correctly aligned after scrolling. Closes #38
• Memory inspector would loose an updated memory reference after the webview changed state
  • Open the memory inspector -> change the address -> click somewhere outside of the webview

Follow up

• 56

• 63

How to test

Open the memory-inspector with the known ways.

• Scroll in the datatable
• Change the options
• Submit with enter or the button

Review checklist

• [x] as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

Closes #54

[thegecko]

Failed due to CI requiring Node 18, see https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/pull/58

Please rebase this on main to fix.

[haydar-metin]

@thegecko Thanks, I rebased and now the action requires an approval.

[colin-grant-work]

A few thoughts:

• This PR does more than the headline indicates. In addition to migrating to a new UI framework, it implements new infinite scrolling features. I'd prefer if those things were in separate PR's for clarity.
• I'm not sure how enthusiastic I am about bringing in a UI framework that introduces hundreds of lines of boilerplate. I'm also not sure that all of the boilerplate included makes sense. I see imports of

@import "~primereact-sass-theme/theme-base/components/input/_calendar";
...
@import "~primereact-sass-theme/theme-base/components/input/_rating";
...
@import "~primereact-sass-theme/theme-base/components/file/_fileupload";

etc. etc.

Are all of those imports in fact used in the Memory Inspector UI? If they aren't, why are they being imported?

[haydar-metin]

@colin-grant-work

I'm not sure how enthusiastic I am about bringing in a UI framework that introduces hundreds of lines of boilerplate. I'm also not sure that all of the boilerplate included makes sense. I see imports of

You are absolutely correct. I left them because that was the default way, and we should remove them. However, concerning the variables folder, we can solve it in three ways, as the values will be only used while compiling:

1. We remove the variables we do not use - however, the variables are general-purpose variables, and they are used in different SCSS files. Removing and adding them again would require some work to identify what we now need and what not (also in the future).
2. We leave the variables there; however, then we would have unused variables in the repository, which would be removed while compiling.
3. We import the default values also from the base repository and override only the new values - ~this should work but it may be a little fragile depending on when the variables are used / how it is implemented~

My tendency is to make 3) work. What do you think?

[colin-grant-work]

What do you think?

I'd say the description makes me even more nervous about the framework as a whole: it sounds like we don't really know what we need to do to make it work for us, and that makes it sound like it's either not very well documented or it's not very well architected. Can you elaborate a little bit on why you chose this framework for reimplementing the UI?

[thegecko]

Thanks for your feedback @colin-grant-work

My team at Arm and EclipseSource have investigated different frameworks we could use in this project which give us the flexibility, customisability and theming required to deliver a tool mainly focussed around a rich table which feels native to VS Code.

We have had much success with primereact in our other extensions at Arm and found it to be a very good option with a large user base and good support. Please feel free to recommend a different table component which we can contrast/compare.

Are all of those imports in fact used in the Memory Inspector UI?

Although I believe tree-shaking is removing the components we don't use, I agree stripping out the style sheets for unused components will make the codebase easier to maintain. This approach would mean we need to add them back in as leverage more components, however.

[haydar-metin]

@colin-grant-work: Please pardon me if my explanation made you unsure. The theming is used on top of primereact which means, we have full control and can add or remove the styling according to our requirements. I just used a base theme examplary to demonstrate how it can be done if we use all components (which is not the case of course for us) and all features, now we can of course modify and strip / clear the style sheets to improve the code base.

~Update: I will change the CSS to be more maintainable.~ DONE

[haydar-metin]

@colin-grant-work @thegecko the changes should be now minimal without the overhead. Can you please provide feedback again? Thanks!

[thegecko]

@colin-grant-work @thegecko the changes should be now minimal without the overhead. Can you please provide feedback again? Thanks!

Looks good, thanks for the update

[colin-grant-work]

Thanks, I appreciate the reduction in the boilerplate. I'll take a closer look at the specifics later today.

[colin-grant-work]

For the record, I'm also not opposed to the infinite scroll feature (as long as it's possible to opt out). I just think it should be a separate PR. Feel free to open one with this as the base, so that it can be addressed when this one gets in.

[haydar-metin]

@thegecko @colin-grant-work as we now have a scrollable table changing the groups-per-row option can have an influence to the scroll position as it changes the height of the items. In the original version, there was no issue as we had to scroll to the top to trigger the change. I did an approximation to get to the last visible area / items, but i think resetting the scroll always to the starting position (top) would be better. What do you think?

@colin-grant-work should I squash the changes?

[colin-grant-work]

@colin-grant-work should I squash the changes?

As you like. If you don't squash, I'll use the Squash and Merge merge mode from GitHub.

[colin-grant-work]

A very minor comment, but while we're changing around a lot of the CSS, would it be possible to get the address column to be sized to max-content so that it doesn't take up extra horizontal space?

[colin-grant-work]

Actually, my practical test raises bigger issues with the styling, here's what I'm seeing:

It appears that each 'group' is being placed on its own line, even though there are oodles of horizontal space for them. That should be avoided.

[haydar-metin]

I've been looking into the virtual scroller with dynamic row heights and wanted to suggest that we consider tackling this as a follow-up task. Implementing this feature is quite complex due to the need for sophisticated calculations for dynamic content sizes, which can significantly impact development time. Would it be also fine for you @colin-grant-work? Apart from that, I think the current solution now provides a similar experience as the original version.

[colin-grant-work]

Previously, some (in fact all, I believe) columns used a monospace font, but it appears that in the current iteration, all columns use a non-monospace font by default. Monospace is certainly preferable for the data and address columns; very likely for ASCII, and it wouldn't do any harm in the variable column, so I think it's a better default.

[haydar-metin]

New Version:

Original Version:

[colin-grant-work]

Hm... in the current state of the code, I'm seeing cases where the address column is overflowing onto a second line, again despite having plenty of horizontal space for everything:

[haydar-metin]

Different widths

Still possible to resize

Hint: It will not automatically break like the other columns! We are defining, that the column width should always be the content width. Allowing also to break would result in a contradiction CSS-wise. That means, we can not say, that it should also break after reaching some max percentage like the other columns - we can provide a max-width with a px value, however, that would be a hardline.

We can solve this in a follow-up task with JS if required.

[colin-grant-work]

The new default sizing looks good. I hadn't noticed the resizability until you mentioned it in one of your commit messages and your screenshots. I won't hold up this PR, but I think to make that feature more discoverable, the drag handle would need to be visible.