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

feat: add --decay option to highlight newer files #526

Closed
wants to merge 12 commits into from
Closed

feat: add --decay option to highlight newer files #526

wants to merge 12 commits into from

Conversation

aashirbadb
Copy link

This PR adds --decay option that highlights newer files. It has two modes:

  • absolute: Evaluates based on file modification time relative to the past year.
  • relative: Considers file modification time in relation to other files.

Both modes use a luminance formula min + (1.0 - min) * exp(-4.0 * (1.0 - x)), where min can be customized by using the 'EZA_MIN_LUMINANCE' environment variable.

Screenshots:

  • relative mode
    image
  • absolute mode
    image
  • tree
    image

(I'm calling it decay because I couldn't think of a better name 😅. I'd greatly appreciate any suggestions for a more fitting name)

Resolves #486

@PThorpe92
Copy link
Member

I like the term decay.. it's a hell of a lot better than rot lmao

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

This is looking cool.

For conventional commits: doc: add --decay option and EZA_MIN_LUMINANCE env var needs to be docs: add --decay option and EZA_MIN_LUMINANCE env var

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Could we have default behavior where --decay without any arguments default to --decay relative?

@aashirbadb
Copy link
Author

Could we have default behavior where --decay without any arguments default to --decay relative?

I tried adding it but it looks like TakesValue::Optional doesn't work like it should.
Passing --decay something gives (decay, Some(something)) instead of (decay, Some(default_value))(if something is invalid option for the flag) or (decay, Some(something)) (if something is valid option).

eza/src/options/parser.rs

Lines 201 to 207 in 9805dd5

TakesValue::Optional(_) => {
if let Some(next_arg) = inputs.next() {
result_flags.push((flag, Some(next_arg)));
} else {
result_flags.push((flag, None));
}
}

Should I fix this(maybe in a separate PR?) or wait for #197 and implement it then?

@PThorpe92
Copy link
Member

Should I fix this(maybe in a separate PR?) or wait for #197 and implement it then?

I am currently working through another implementation that gives a flag a default value. Once I'm finished I'll take a look and see if we can get it figured out.

Hopefully clap will be very soon. Honestly, this is a really exciting change and part of me does want to wait and release the functionality with v1.0

Feel free to come to the matrix channel too if you want to talk about it

@cohml
Copy link

cohml commented Oct 23, 2023

Feel free to come to the matrix channel too if you want to talk about it

Link to said channel please? I'm not familiar with this "matrix", but would like to be a fly on the wall. Thanks.

@PThorpe92
Copy link
Member

If you go to the main page the "Chat on gitter" button

https://matrix.to/#/#eza-community:gitter.im is the link

@Delapouite
Copy link

Also, should the option --color-scale be renamed to avoid any confusion about where it applies ? size vs date?

@PThorpe92
Copy link
Member

Also, should the option --color-scale be renamed to avoid any confusion about where it applies ? size vs date?

That is a good question.. how would you feel about making this a value for the color-scale flag? e.g. --color-scale=age || --color-scale=size

@PThorpe92
Copy link
Member

also, I was able to get the --icons flag to take a default value and have a TakesValue::Optional(Some(VALUE))

So if you want to have this take an optional argument and set a default value, you can look at how icons does it now.

@Delapouite
Copy link

You're right PThorpe92, the notion of color-scale could even reach further.

Non exhaustive list of possible columns :

  • date: old → new (this PR)
  • size: small → big (kind of current behavior of --color-scale, but limited to magnitude kilo, mega, tera…)
  • links count : few → lot
  • octal permissions : very-restricted → open bar (777)

@PThorpe92
Copy link
Member

I think the way to go would be to have breaking changes, and make sure that --color-scale has a value. It will break people's alias's, but it will also alert them to the new feature.

Have you tested this alongside the current --color-scale flag? How does that work?

@aashirbadb
Copy link
Author

That is a good question.. how would you feel about making this a value for the color-scale flag? e.g. --color-scale=age || --color-scale=size

--decay takes two values so I can just change it so that relative is default value when --color-scale=age is used and it can be changed to absolute mode using --color-scale=age --decay=absolute.

I think the way to go would be to have breaking changes, and make sure that --color-scale has a value. It will break people's alias's, but it will also alert them to the new feature.

We can also allow multiple choices using comma separated list(--color-scale=age,size) and maybe add an all choice which could be optional flag to not introduce a breaking change?

@PThorpe92
Copy link
Member

My thought process was kinda specifically to introduce the feature. Adding a value to a flag isn't nearly as exciting as the new decay option. But I do like that idea. It could just default to all

aashirbadb and others added 3 commits October 26, 2023 14:19
Signed-off-by: Aashirbad Bhandari <89701256+aashirbadb@users.noreply.github.com>
@aashirbadb
Copy link
Author

I think it would be better to remove the --decay flag and use --color-scale and add a new flag(something like --color-scale-mode) to select the mode(fixed,gradient).

Fixed would work like the current --color-scale and gradient would work like relative mode.

This would also make it easier to add more columns like Delapouite suggested.

@aashirbadb
Copy link
Author

I added this to size and it looks pretty cool.

image

@aashirbadb
Copy link
Author

Had to make it a breaking change

I haven't done anything for time's fixed mode. I'm thinking of adding something like --color-scale. It will make the time colors more customizable but it will break custom color for date(da).

Not sure why the CI failing

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Looks like you have a formatting issue, if you're using nix, this is easily fixed with nix fmt.

Also, your breaking change should have a BREAKING footer (or w/e exactly it's called) as specified by conventional commits, you can find more info about it in the conventional commits spec.

Signed-off-by: Christina Sørensen <christina@cafkafk.com>
@@ -90,7 +90,8 @@ eza’s options are almost, but not quite, entirely unlike `ls`’s.
- **-x**, **--across**: sort the grid across, rather than downwards
- **-F**, **--classify**: display type indicator by file names
- **--colo[u]r=(when)**: when to use terminal colours (always, auto, never)
- **--colo[u]r-scale**: highlight levels of file sizes distinctly
- **--colo[u]r-scale=(field)**: highlight levels of `field` distinctly(all, age, size)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like age as Field name could you use the name used on the --header option

Comment on lines -207 to +203
test!(scale_1: ColourScale <- ["--color-scale", "--colour-scale"]; Last => Ok(ColourScale::Gradient));
test!(scale_2: ColourScale <- ["--color-scale", ]; Last => Ok(ColourScale::Gradient));
test!(scale_3: ColourScale <- [ "--colour-scale"]; Last => Ok(ColourScale::Gradient));
test!(scale_4: ColourScale <- [ ]; Last => Ok(ColourScale::Fixed));
// test!(scale_1: ColourScale <- ["--color-scale", "--colour-scale"]; Last => Ok(ColourScale::Gradient));
// test!(scale_2: ColourScale <- ["--color-scale", ]; Last => Ok(ColourScale::Gradient));
// test!(scale_3: ColourScale <- [ "--colour-scale"]; Last => Ok(ColourScale::Gradient));
// test!(scale_4: ColourScale <- [ ]; Last => Ok(ColourScale::Fixed));

test!(scale_5: ColourScale <- ["--color-scale", "--colour-scale"]; Complain => err OptionsError::Duplicate(Flag::Long("color-scale"), Flag::Long("colour-scale")));
test!(scale_6: ColourScale <- ["--color-scale", ]; Complain => Ok(ColourScale::Gradient));
test!(scale_7: ColourScale <- [ "--colour-scale"]; Complain => Ok(ColourScale::Gradient));
test!(scale_8: ColourScale <- [ ]; Complain => Ok(ColourScale::Fixed));
// test!(scale_5: ColourScale <- ["--color-scale", "--colour-scale"]; Complain => err OptionsError::Duplicate(Flag::Long("color-scale"), Flag::Long("colour-scale")));
// test!(scale_6: ColourScale <- ["--color-scale", ]; Complain => Ok(ColourScale::Gradient));
// test!(scale_7: ColourScale <- [ "--colour-scale"]; Complain => Ok(ColourScale::Gradient));
// test!(scale_8: ColourScale <- [ ]; Complain => Ok(ColourScale::Fixed));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove those comments and test the code

Copy link
Contributor

Choose a reason for hiding this comment

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

If not possible to test just remove it

Comment on lines +104 to +112
fn update_information_recursively(
information: &mut ColorScaleInformation,
files: &[File<'_>],
dot_filter: DotFilter,
git: Option<&GitCache>,
git_ignoring: bool,
depth: TreeDepth,
r: Option<RecurseOptions>,
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this function not in the impl above ?

Comment on lines +37 to +38
NumberPrefix::Standalone(_) => None,
NumberPrefix::Prefixed(p, _) => Some(p),
Copy link
Contributor

Choose a reason for hiding this comment

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

please format those lines

cafkafk added a commit that referenced this pull request Nov 8, 2023
This is based on a series of commits done by aashirabadb in #526.

This introduces the `--color-scale` flag both for file age and size, as
well as `--color-scale-mode` which allows "fixed" and gradient".

The reason for reapplying from the diff of the PR instead of the patch
is that the commit history was messy, and introduced "breaking changes"
that were only locally breaking to the PR, that is, not the users.

Further, the PR had seemingly gone stale, and the rather long and
complicated commit history interwoven with merges made it seem more
efficient to just work from scratch.

Again, the work here is done by aashirbadb, my contribution is just
ensuring the quality of the code they've written.

Co-authored-by: aashirbadb <aashirbadbhandari@gmail.com>
Signed-off-by: Christina Sørensen <christina@cafkafk.com>

Refs: #486
@cafkafk cafkafk mentioned this pull request Nov 8, 2023
cafkafk added a commit that referenced this pull request Nov 8, 2023
This is based on a series of commits done by aashirabadb in #526.

This introduces the `--color-scale` flag both for file age and size, as
well as `--color-scale-mode` which allows "fixed" and gradient".

The reason for reapplying from the diff of the PR instead of the patch
is that the commit history was messy, and introduced "breaking changes"
that were only locally breaking to the PR, that is, not the users.

Further, the PR had seemingly gone stale, and the rather long and
complicated commit history interwoven with merges made it seem more
efficient to just work from scratch.

Again, the work here is done by aashirbadb, my contribution is just
ensuring the quality of the code they've written.

Co-authored-by: aashirbadb <aashirbadbhandari@gmail.com>
Signed-off-by: Christina Sørensen <christina@cafkafk.com>

Refs: #486
@cafkafk
Copy link
Member

cafkafk commented Nov 8, 2023

This was fixed in #628

@cafkafk cafkafk closed this Nov 8, 2023
MartinFillon pushed a commit that referenced this pull request Nov 11, 2023
This is based on a series of commits done by aashirabadb in #526.

This introduces the `--color-scale` flag both for file age and size, as
well as `--color-scale-mode` which allows "fixed" and gradient".

The reason for reapplying from the diff of the PR instead of the patch
is that the commit history was messy, and introduced "breaking changes"
that were only locally breaking to the PR, that is, not the users.

Further, the PR had seemingly gone stale, and the rather long and
complicated commit history interwoven with merges made it seem more
efficient to just work from scratch.

Again, the work here is done by aashirbadb, my contribution is just
ensuring the quality of the code they've written.

Co-authored-by: aashirbadb <aashirbadbhandari@gmail.com>
Signed-off-by: Christina Sørensen <christina@cafkafk.com>

Refs: #486
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

feat: Use font dimming to communicate modification/creation time
6 participants