Skip to content

Use filetreelist crate #1508

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.

Sign up for GitHub

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

Jump to bottom
Draft
abergmeier wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Use filetreelist crate #1508

abergmeier wants to merge 1 commit into from

Conversation

@abergmeier
Copy link

@abergmeier abergmeier commented Jan 15, 2023

No longer safe Status inside of FileTreeItem.
For Status storage, now introduced a new Item, which contains Status alongside FileTreeItem.

Note that FileTreeItemKind is no longer publicy available - thus we cannot store this enum anywhere anymore.

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog
@abergmeier abergmeier mentioned this pull request Jan 15, 2023
Open
@abergmeier
Copy link
Author

abergmeier commented Jan 15, 2023 •
edited
Loading

So far change component shows no diff - not sure why
@abergmeier abergmeier marked this pull request as ready for review January 15, 2023 20:31
@abergmeier
Copy link
Author

abergmeier commented Jan 15, 2023

I think for being able to write unittests I would need to extract a lot of traits for components.
@extrawurst
Copy link
Collaborator

extrawurst commented Jan 15, 2023

All good the tree component itself is pretty well tested
@abergmeier
Use filetreelist crate
6d3faa2 
No longer safe Status inside of FileTreeItem. For Status storage, now introduced a new Item, which contains Status alongside FileTreeItem. Note that FileTreeItemKind is no longer publicy available - thus we cannot store this enum anywhere anymore.
extrawurst
extrawurst reviewed Jan 29, 2023
View reviewed changes
src/components/status_tree.rs
indent: u8,
visible: bool,
item_kind: &'a FileTreeItemKind,
status_item: Option<StatusItem>,
Copy link
Collaborator

@extrawurst extrawurst Jan 29, 2023

Choose a reason for hiding this comment

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

this looks like it will require a lot of allocations where only references where needed before
Copy link
Author

@abergmeier abergmeier Jan 29, 2023

Choose a reason for hiding this comment

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

Will have a look.
@extrawurst
Copy link
Collaborator

extrawurst commented Jan 29, 2023

While reviewing noticed that it still has its own FileTreeItems instead of using the one from filetreelist-crate. We should make it use the crate one
@extrawurst extrawurst marked this pull request as draft January 29, 2023 08:51
@extrawurst
Copy link
Collaborator

extrawurst commented Jan 29, 2023

putting in draft till the next round of reviews
@abergmeier
Copy link
Author

abergmeier commented Jan 29, 2023 •
edited
Loading

While reviewing noticed that it still has its own FileTreeItems instead of using the one from filetreelist-crate. We should make it use the crate one

The reason is that filetreelist crate knows nothing about status. If you want to use it I see 2 basic ways of doing it:

  1. Introduce status to filetreelist crate
  2. Have a separate "list" for status

How do you want handle that?
@stale
Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as stale because it has not had any activity half a year. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
@stale stale bot added the dormant Marked by stale bot on close label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dormant Marked by stale bot on close

2 participants

@abergmeier @extrawurst