Skip to content

refactor: misc restructuring#1015

Merged
ChrisTitusTech merged 1 commit intomainfrom
unknown repository
Feb 11, 2025
Merged

refactor: misc restructuring#1015
ChrisTitusTech merged 1 commit intomainfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Feb 2, 2025

Changes:

Separate cli flags and put them in
their own module.

Add a rustfmt configuration file for
grouping imports into crates (so we
dont have to do this manually, and it
seems we were already doing it manually
so might as well)

Use comments to describe cli flags
instead of using ugly syntax (they both
work the same but one is less ugly and
more readable)

Add a --mouse flag to enable mouse
interaction (mouse interaction is now
off by default)

Add a --bypass-root flag to disable
the annoying popup when you run the
utility as root

Fix an issue where you can't reach the
bottom of the list in a subdir (i also
reduced the nesting in those functions
as well for readability)

Add help feature to clap dependency to
enable --help / -h

Add usage feature to clap dependency
to enable usage example when running
with --help / -h

Remove CLI arg examples from readme,
and add instructions on how to view
them on CLI to prevent it from bloating
up the readme

Explanation:

Some changes in this pull request
duplicate the fixes from #961
while the changes are not
inherently duplicated, I
had refactored the 2 func's
to improve readability and
ended up fixing the same bug.
I had commented under that PR
asking if I should drop the changes
but I have not received a response yet
(at the time of writing)

@ghost
Copy link
Author

ghost commented Feb 2, 2025

I also think we should go through and reduce nesting across the codebase as well, the code isn't very readable right now

@ghost
Copy link
Author

ghost commented Feb 2, 2025

@lj3954 @JustLinuxUser @cartercanedy @adamperkowski feel free to review

@ghost ghost mentioned this pull request Feb 2, 2025
5 tasks
@JustLinuxUser
Copy link
Contributor

I still haven't reviewed the code, but the --help problem is because someone disabled default-features in clap in Cargo.toml, so a regression introduced in #922 . Just add help in the feature list, you can learn more here

@ghost
Copy link
Author

ghost commented Feb 2, 2025

I still haven't reviewed the code, but the --help problem is because someone disabled default-features in clap in Cargo.toml, so a regression introduced in #922 . Just add help in the feature list, you can learn more here

thanks

Copy link
Contributor

@JustLinuxUser JustLinuxUser left a comment

Choose a reason for hiding this comment

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

I like the changes, but some small tweaks would improve them even more, especially the scroll up, and down thing, I would prefer something like:

    fn scroll_down(&mut self) {
        let list_len = self.filter.item_list().len() + !self.at_root() as usize;
        if let Some(selected) = self.selection.selected() {
            if selected == list_len - 1 {
                self.selection.select_first();
            } else {
                self.selection.select_next();
            }
        }
    }

    fn scroll_up(&mut self) {
        if let Some(selected) = self.selection.selected() {
            if selected == 0 {
                self.selection.select_last();
            } else {
                self.selection.select_previous();
            }
        }
    }

@JustLinuxUser
Copy link
Contributor

Btw, the --help thing, you might want to coordinate that with @zdiff, the creator of #935 pr, and the creator of the PR that created the regression in the first place

@ghost
Copy link
Author

ghost commented Feb 2, 2025

I like the changes, but some small tweaks would improve them even more, especially the scroll up, and down thing, I would prefer something like:

    fn scroll_down(&mut self) {
        let list_len = self.filter.item_list().len() + !self.at_root() as usize;
        if let Some(selected) = self.selection.selected() {
            if selected == list_len - 1 {
                self.selection.select_first();
            } else {
                self.selection.select_next();
            }
        }
    }

    fn scroll_up(&mut self) {
        if let Some(selected) = self.selection.selected() {
            if selected == 0 {
                self.selection.select_last();
            } else {
                self.selection.select_previous();
            }
        }
    }

to be honest, i like my version better. Maybe its based on style preferences, but i prefer flat, easy to follow, code that has clean separation.

I would also like to mention that mine avoids nested conditionals and separates the length calc from the selection calc.

@ghost
Copy link
Author

ghost commented Feb 2, 2025

Btw, the --help thing, you might want to coordinate that with @zdiff, the creator of #935 pr, and the creator of the PR that created the regression in the first place

I fixed the issue in d1444bd 5164f14

Regardless he should see this pull request since you mentioned him

@lj3954
Copy link
Contributor

lj3954 commented Feb 2, 2025

I like the changes, but some small tweaks would improve them even more, especially the scroll up, and down thing, I would prefer something like:

    fn scroll_down(&mut self) {
        let list_len = self.filter.item_list().len() + !self.at_root() as usize;
        if let Some(selected) = self.selection.selected() {
            if selected == list_len - 1 {
                self.selection.select_first();
            } else {
                self.selection.select_next();
            }
        }
    }

    fn scroll_up(&mut self) {
        if let Some(selected) = self.selection.selected() {
            if selected == 0 {
                self.selection.select_last();
            } else {
                self.selection.select_previous();
            }
        }
    }

I dislike the boolean to integer cast. It makes it more difficult to reason about the code.

@JustLinuxUser
Copy link
Contributor

JustLinuxUser commented Feb 2, 2025

I dislike the boolean to integer cast. It makes it more difficult to reason about the code.

It's ok, if you want, you can still use the if, so it would become:

        let list_len = self.filter.item_list().len() + if self.at_root() { 0 } else { 1 };

or even

let list_len = if self.at_root {self.filter.item_list().len()} else {self.filter.item_list().len() + 1}

like you had before, choose whatever you like best

@koibtw koibtw changed the title refactor(tui): large changes... refact: misc restructuring Feb 7, 2025
@ghost ghost changed the title refact: misc restructuring refactor(tui): misc restructuring Feb 7, 2025
@ghost ghost requested a review from koibtw February 7, 2025 21:09
@ghost
Copy link
Author

ghost commented Feb 7, 2025

@JustLinuxUser take a look at f28e085 and see if you like it better

@koibtw koibtw changed the title refactor(tui): misc restructuring refactor: misc restructuring Feb 7, 2025
Copy link
Collaborator

@koibtw koibtw left a comment

Choose a reason for hiding this comment

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

the scope shouldn't be explicitly tui

@JustLinuxUser
Copy link
Contributor

@JustLinuxUser take a look at f28e085 and see if you like it better

That does look good, ty

@ghost ghost closed this Feb 10, 2025
@ghost
Copy link
Author

ghost commented Feb 10, 2025

oops

@ghost
Copy link
Author

ghost commented Feb 10, 2025

wrong repo 😭

@ghost ghost reopened this Feb 10, 2025
Changes:

Separate cli flags and put them in
their own module.

Add a rustfmt configuration file for
grouping imports into crates (so we
dont have to do this manually, and it
seems we were already doing it manually
so might as well)

Use comments to describe cli flags
instead of using ugly syntax (they both
work the same but one is less ugly and
more readable)

Add a --mouse flag to enable mouse
interaction (mouse interaction is now
off by default)

Add a --bypass-root flag to disable
the annoying popup when you run the
utility as root

Fix an issue where you can't reach the
bottom of the list in a subdir (i also
reduced the nesting in those functions
as well for readability)

Add help feature to clap dependency to
enable --help / -h

Add usage feature to clap dependency
to enable usage example when running
with --help / -h

Remove CLI arg examples from readme,
and add instructions on how to view
them on CLI to prevent it from bloating
up the readme
@ChrisTitusTech ChrisTitusTech merged commit 6b572b1 into ChrisTitusTech:main Feb 11, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants