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

Switch to AntD TreeTable and remove deprecated code #32

Merged
merged 12 commits into from
Dec 17, 2024

Conversation

haydar-metin
Copy link
Contributor

@haydar-metin haydar-metin commented Nov 28, 2024

Closes #27

TreeTable.mp4

Tree

This PR removes the PrimeReact Tree and TreeTable, as well as the VSCode Tree. Instead, it introduces the Ant Design (AntD) Tree Table, which is allows virtualization and provides a simpler API. Alongside this, all deprecated code has been removed, ensuring the project now exclusively uses the new component.

Model

One major issue with the slow rendering was the back-and-forth communication between the VSCode extension and the webview. To address this, the peripheral model structure has been separated into three layers:

  • Peripheral Model (e.g., PeripheralNode): Handles data loading and updating.
  • Serialization Model (e.g., PeripheralNodeDTO): Contains the serialized data of the peripheral model, which is transferred to the webview.
  • Display Model (e.g., CDTTreeItem): Renders the tree table in the webview.

UI-specific aspects have been moved to a custom converter (e.g., PeripheralNodeConverter). This converter uses the serialized data to create the tree for rendering (Serialization Model -> Display Model).

The webview only sees the Serialization Model / Display Model.

Communication

UI-specific actions like expansion and pinning are handled directly in the webview, which updates the display model immediately. However, these updates are also propagated back to the extension to update the domain model when needed.

The webview can request a resync if necessary. In such cases, after an action is triggered, the entire serialized domain model is sent back to the webview to recreate the display model.

This means expansion happens instantly, and if any data changes, the extension can sync the local model accordingly.

Follow-up

  1. I’ve tried to be as cautious as possible and avoided changing the peripheral model too much. It still includes "pinned" and "expansion" properties for now, but we should consider removing these in a follow-up PR.

  2. While the tree feels smoother now, there’s still room for improvement. For instance, caching the peripheral models and serializing only the changes could make it even faster.

  3. Instead of saving the expanded/pinned state directly in the Peripheral Model. We should move it to a custom service.

  4. Most of the commands are not necessary anymore (e.g., Pin, Set Format). We should remove them and handle those cases directly in the webview. For example, we could show a context menu with the different formats instead of using the VSCode quick select.

PS: Most of the changes are moving functionality around.

Copy link
Contributor

@jreineckearm jreineckearm left a comment

Choose a reason for hiding this comment

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

See some comments.

src/common/format.ts Show resolved Hide resolved
src/common/peripheral.ts Outdated Show resolved Hide resolved
src/common/vscode.ts Show resolved Hide resolved
src/manifest.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/plugin/peripheral/tree/peripheral-session-tree.ts Outdated Show resolved Hide resolved
src/svd-parser.ts Outdated Show resolved Hide resolved
@jreineckearm
Copy link
Contributor

One finding from testing:

  • I see this behavior if I reduce the width of the panel so that there is not enough space to show all text of the name column:
    image

@jreineckearm
Copy link
Contributor

Some more findings:

  • With the original TreeView implementation it was possible to navigate via the keyboard. And hence to select a node. We should try to restore that. This may also help with navigating the tree in combination with the search functionality.
  • We have now two additional context menu entries Undo and Redo (compared to TreeView implementation). What history would these use? Only user input? Or also values from updates due to executing code? We might want to consider removing those for the time being. If they haven't been added by purpose.
  • The display of pinned values looks slightly different (compared to the TreeView and the PrimeReact implementation). Is it possible to remove the extra node by adding the pin icon inline?
    Before
    image
    After
    image

- Fix copyright issues
- Fix CSS (pin, ellipsis) issues
- Make configuration for ignored peripheral names
@haydar-metin
Copy link
Contributor Author

Thanks for the feedback @jreineckearm!

The latest changes introduced:

  • ellipsis for the columns and other css fixes
  • settings for ignored peripheral names
  • keyboard navigation (same way as in vscode)

@jreineckearm
Copy link
Contributor

Thanks, @haydar-metin !

  • The rendering artifacts caused by the panel resizing are gone. I noticed however that narrowing the panel size comes with a delay after releasing the mouse button before the view is re-rendered. Whereas widening the panel size takes immediate effect. Is this something under our control?

  • The keyboard navigation works nicely. However, I believe the associated node selection causes some trouble. To reproduce, click into the tree, then click on the vertical scrollbar and drag up and down. At some point the scrollbar keeps moving if I release the left mouse button. It requires another click into the tree to stop that.
    I noticed this rendering effect which makes me wonder if there is an issue with the mouse-hit-areas of vertical scrollbar and node elements due to some miscalculated boundaries....this one unfortunately needs attention before merge.
    image

  • The new undo /redo context menu entries are still there. But I could live with that.

  • The pinning works like before the refurbishing. Thanks for that!

@martin-fleck-at
Copy link
Contributor

  • The rendering artifacts caused by the panel resizing are gone. I noticed however that narrowing the panel size comes with a delay after releasing the mouse button before the view is re-rendered. Whereas widening the panel size takes immediate effect. Is this something under our control?

Very interesting, not sure what could cause this. However, I also experience this with other views in VS Code (e.g., Copilot Chat) so I'm not sure what we can do about this.

  • The keyboard navigation works nicely. However, I believe the associated node selection causes some trouble. To reproduce, click into the tree, then click on the vertical scrollbar and drag up and down. At some point the scrollbar keeps moving if I release the left mouse button. It requires another click into the tree to stop that.
    I noticed this rendering effect which makes me wonder if there is an issue with the mouse-hit-areas of vertical scrollbar and node elements due to some miscalculated boundaries....this one unfortunately needs attention before merge.
    image

I pushed a commit that should fix that. From what I can tell, the problem is that when you release the mouse outside the iframe (our view), we do not abort the scrolling as we do not receive the event. I now abort the scrolling on purpose once we leave the iframe, could you please check whether that also fixes your problem?

  • The new undo /redo context menu entries are still there. But I could live with that.

Haydar and I had a look at this together but we couldn't find any way to suppress this. There is some code in that allows you to suppress cut/copy/paste but not undo/redo. I can also see the Undo/Redo in other extensions webview, e.g., GitLens, so we probably need to add a hook in VS Code for that.

@jreineckearm
Copy link
Contributor

I pushed a commit that should fix that. From what I can tell, the problem is that when you release the mouse outside the iframe (our view), we do not abort the scrolling as we do not receive the event. I now abort the scrolling on purpose once we leave the iframe, could you please check whether that also fixes your problem?

@martin-fleck-at , that is unfortunately very sensitive now to moving the mouse-pointer to the right of the scrollbar.
Is it the mouse up event that you do not receive?

@martin-fleck-at
Copy link
Contributor

@jreineckearm I think that sensitivity might have always been there. Basically we do not receive any events from outside the iframe. I pushed a commit that aborts the scroll on re-enter which gives a much better user experience in my opinion - the scrolling still stops outside the view but if you keep the mouse pressed and go back it will continue. Could you have a look whether that works for you as well?

@jreineckearm
Copy link
Contributor

Thanks, @martin-fleck-at ! Much better!
I compared also with the behavior of other views in VS Code and can see that those even jump back to the start of the scrolling action. But I am good with not replicating that behavior right now. Could be a future enhancement (unless you say is super-easy to do this :-) ).

package.json Outdated Show resolved Hide resolved
@jreineckearm
Copy link
Contributor

The peripheral ignore list works nicely. Some future enhancements around this:

  • Allow modification of the list at runtime.
  • Allow adding peripherals to the list through Peripheral Inspector context menu (maybe there for workspace not user settings).
    • Clearing the list might be worth a consideration to keep symmetry in the feature.

Co-authored-by: Jens Reinecke <[email protected]>
@martin-fleck-at
Copy link
Contributor

@jreineckearm Sounds good to me, should we create a separate issue for the enhancements? I pushed your typo fix as well. If you agree with the PR now, please feel free to approve and merge it ;-)

Copy link
Contributor

@jreineckearm jreineckearm left a comment

Choose a reason for hiding this comment

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

Thanks again for this! Changes look good to me now. Nice work!

@martin-fleck-at , yes, please open the remaining enhancement ideas as new issues.

@planger , @thegecko , could you please help with merging this PR?

Thanks,
Jens

@planger
Copy link

planger commented Dec 17, 2024

@jreineckearm 🚀 Great! Happy to merge. I guess we want to squash, right?

@thegecko thegecko merged commit 5787e16 into eclipse-cdt-cloud:main Dec 17, 2024
5 checks passed
@thegecko
Copy link
Contributor

Squashed ;)

@jreineckearm
Copy link
Contributor

Yes, squash it was :-)

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.

Consider Ant Design Table as UI Framework
5 participants