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

return user to their previous position if they exit a subdir #725

Merged
merged 6 commits into from
Oct 31, 2024

Conversation

nnyyxxxx
Copy link
Contributor

@nnyyxxxx nnyyxxxx commented Oct 2, 2024

watch the video

Type of Change

  • New feature

Description

takes the user back to their previous position when they exit a subdir

tested with a subdir inside of a subdir works as expected

tested with a regular subdir works as expected as well

example:

2024-10-02-00-19-31.mp4

before, it would just take u back to the first entry in the list

@nnyyxxxx nnyyxxxx changed the title return user to their previous position if they enter a subdir return user to their previous position if they exit a subdir Oct 2, 2024
Copy link
Contributor

@cartercanedy cartercanedy left a comment

Choose a reason for hiding this comment

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

Put visit_stack and position_stack together:

// ...
    visit_stack: Vec<(Rc<ListNode>, usize)>
// ...

Correction:

// ...
    visit_stack: Vec<(NodeId, usize)>,
// ...

tui/src/state.rs Outdated Show resolved Hide resolved
tui/src/state.rs Outdated Show resolved Hide resolved
@nnyyxxxx
Copy link
Contributor Author

nnyyxxxx commented Oct 2, 2024

Put visit_stack and position_stack together:

// ...
    visit_stack: Vec<(Rc<ListNode>, usize)>
// ...

its best we keep them separated

@cartercanedy
Copy link
Contributor

its best we keep them separated

Besides unnecessarily making a heap allocation, they're both participating in the same functionality; they're best together

@nnyyxxxx
Copy link
Contributor Author

nnyyxxxx commented Oct 2, 2024

its best we keep them separated

Besides unnecessarily making a heap allocation, they're both participating in the same functionality; they're best together

sorry but i see no performance impact with this current implementation, if we want to combine them then we can refactor it in the future. For now this implementation is just a stepping stone.

@cartercanedy
Copy link
Contributor

cartercanedy commented Oct 2, 2024

sorry but i see no performance impact with this current implementation, if we want to combine them then we can refactor it in the future. For now this implementation is just a stepping stone.

My point is less about immediate performance issues and more about not accumulating code that should get refactored down the road...

I figure that there's no better time than now
Just opened a PR for your fork, you're free to merge the changes if you want

@cartercanedy
Copy link
Contributor

I also realize that my initial suggestion was actually incorrect, so that might've been confusing the situation. My 3y/o was a little cranky last night, so that made me a little cranky as well :))

@nnyyxxxx nnyyxxxx requested a review from cartercanedy October 3, 2024 02:17
Copy link
Contributor

@cartercanedy cartercanedy left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for merging that commit

@ChrisTitusTech ChrisTitusTech merged commit 2d14a0a into ChrisTitusTech:main Oct 31, 2024
2 checks passed
@nnyyxxxx nnyyxxxx deleted the savestate branch November 1, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants