-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
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.
See some comments.
- Fix copyright issues - Fix CSS (pin, ellipsis) issues - Make configuration for ignored peripheral names
a467ac3
to
1270fe2
Compare
1270fe2
to
c2e0146
Compare
Thanks for the feedback @jreineckearm! The latest changes introduced:
|
Thanks, @haydar-metin !
|
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.
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?
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. |
@martin-fleck-at , that is unfortunately very sensitive now to moving the mouse-pointer to the right of the scrollbar. |
@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? |
Thanks, @martin-fleck-at ! Much better! |
The peripheral ignore list works nicely. Some future enhancements around this:
|
Co-authored-by: Jens Reinecke <[email protected]>
@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 ;-) |
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 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
@jreineckearm 🚀 Great! Happy to merge. I guess we want to squash, right? |
Squashed ;) |
Yes, squash it was :-) |
Closes #27
TreeTable.mp4
Tree
This PR removes the PrimeReact
Tree
andTreeTable
, 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:
PeripheralNode
): Handles data loading and updating.PeripheralNodeDTO
): Contains the serialized data of the peripheral model, which is transferred to the webview.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
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.
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.
Instead of saving the expanded/pinned state directly in the Peripheral Model. We should move it to a custom service.
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.