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

Grid view is broken #66

Closed
cafkafk opened this issue Jul 29, 2023 · 30 comments · Fixed by #79
Closed

Grid view is broken #66

cafkafk opened this issue Jul 29, 2023 · 30 comments · Fixed by #79

Comments

@cafkafk
Copy link
Member

cafkafk commented Jul 29, 2023

Running --grid with the latest git version of zetta is broken on my machine.

E.g. by doing cargo run --release -- --grid, or using an installed version (see screenshot below).

2023-07-28_13-43

This is a fairly severe regression as the the default expected behavior of exa/zetta is to run --grid if nothing else is specified.

ogham/exa#1165 (comment)

syphar/zetta#4

@cafkafk
Copy link
Member Author

cafkafk commented Jul 29, 2023

This existed before any of the merges thou, as discussed in zetta, but tracking and bisecting this issue is important, and I'll get on it after I'm done with the PRs

@cafkafk cafkafk pinned this issue Jul 29, 2023
@cafkafk
Copy link
Member Author

cafkafk commented Jul 29, 2023

After a bisect, I've arrive here:

af208285e8a0fb383f59372c966bdb77f94006e9 is the first bad commit
commit af208285e8a0fb383f59372c966bdb77f94006e9
Author: Bastien Orivel <eijebong@bananium.fr>
Date:   Fri Dec 10 13:21:13 2021 +0100

    Update term_grid to 0.2

 Cargo.lock                 | 4 ++--
 Cargo.toml                 | 2 +-
 src/output/grid.rs         | 1 +
 src/output/grid_details.rs | 2 ++
 4 files changed, 6 insertions(+), 3 deletions(-)

af20828

Gonna see if I can solve it.

@cafkafk
Copy link
Member Author

cafkafk commented Jul 29, 2023

The plot thickens ogham/rust-term-grid#11

@cafkafk
Copy link
Member Author

cafkafk commented Jul 29, 2023

I'm pretty sure we should move to https://github.com/uutils/uutils-term-grid (see ogham/rust-term-grid#15), but for now I'll try to go back to rust-term-grid 0.1.

@sbatial
Copy link
Collaborator

sbatial commented Jul 29, 2023

I'm pretty sure we should move to https://github.com/uutils/uutils-term-grid (see ogham/rust-term-grid#15)

Agreed

@ariasuni
Copy link
Contributor

ariasuni commented Aug 4, 2023

Regarding uutils/uutils-term-grid#6, do you think the behavior should change in uutils-term-grid? I don’t really care myself but I figured it could matter to some people here.

@cafkafk
Copy link
Member Author

cafkafk commented Aug 5, 2023

Regarding uutils/uutils-term-grid#6, do you think the behavior should change in uutils-term-grid? I don’t really care myself but I figured it could matter to some people here.

I'm don't really care that much, at least when just looking at it initially. Of course if it's not that big of an issue, having it configurable would perhaps be preferred, but personally, I don't see it being an issue for my use.

I'd be interested in how other people feel about it however.

@Vistaus
Copy link

Vistaus commented Sep 12, 2023

It's still broken for me, even in eza.

Eza version: 0.11
OS: openSUSE Tumbleweed
Terminal emulator: Yakuake

@ariasuni
Copy link
Contributor

Do you think you could post a screenshot of the output of eza vs ls?

@LeoniePhiline
Copy link
Contributor

eza --grid works for me on OpenSUSE Tumbleweed.
Terminal emulator: Konsole
Maybe Yakuake is the problem?

grafik

@Vistaus
Copy link

Vistaus commented Sep 13, 2023

I figured it out: it had to do with the width. For some reason, if I set Yakuake to 60%, which I prefer, eza does not do grid. However, if I set it to 80%, it does work. Why it does not work with 60% is beyond me, as my screen is 2560x1440, so I have plenty of space to show a proper grid with 60%, but oh well. Thanks for all of your input!

@Vistaus
Copy link

Vistaus commented Sep 13, 2023

UPDATE: actually, it doesn't always seem to work with 80% either. Only when I have a handful of files. In a folder with about 15 files, it doesn't work.

@ariasuni
Copy link
Contributor

I asked for a screenshot (but a copy-paste of the output would work as well) because otherwise it’s impossible to diagnose, since it triggers in only some cases. If you’re concerned about privacy, it should be possible to recreate the issue with different file names of the same length (if they sort in the same order).

@ariasuni ariasuni reopened this Sep 14, 2023
@cafkafk
Copy link
Member Author

cafkafk commented Sep 14, 2023

Also I should mention, echo $COLUMNS has historically been super useful to diagnose problems like this

@github-actions
Copy link

This issue is stale because it has been open for 30 days with no activity.

@userrand
Copy link

UPDATE: actually, it doesn't always seem to work with 80% either. Only when I have a handful of files. In a folder with about 15 files, it doesn't work.

I figured it out: it had to do with the width. For some reason, if I set Yakuake to 60%, which I prefer, eza does not do grid. However, if I set it to 80%, it does work. Why it does not work with 60% is beyond me, as my screen is 2560x1440, so I have plenty of space to show a proper grid with 60%, but oh well. Thanks for all of your input!

Maybe it is just my font size but I noticed that, at least on my computer, yakuake does not work with fzf ** completion whereas konsole does. I am also unable to see the processes in htop with yakuake. So it seems to me that yakuake has some trouble showing some tui processes (but maybe it is just my font size).

@cafkafk
Copy link
Member Author

cafkafk commented May 5, 2024

I'm pretty sure this issue has been resolved by now, if not, let me know and we can always reopen.

@cafkafk cafkafk closed this as completed May 5, 2024
@tribals
Copy link

tribals commented Jun 30, 2024

ysh ysh-0.22.0$ eza --version
eza - A modern, maintained replacement for ls
v0.18.17 [+git]
https://github.com/eza-community/eza
ysh ysh-0.22.0$ eza --long --grid
drwxr-xr-x   - anthony 14 Jun 04:28  debian-nosh        drwxr-xr-x   - anthony 22 Jun 17:20  projects    drwxr-xr-x   - anthony 22 Jun 18:58  space
.rw-r--r-- 28k anthony 14 Jun 20:52  guix-install.sh    drwxr-xr-x   - anthony 23 Jun 19:05  soccoop     drwxr-xr-x   - anthony 25 Jun 23:17  vpnn
ysh ysh-0.22.0$ env EZA_GRID_ROWS=10 eza --long --grid
drwxr-xr-x   - anthony 14 Jun 04:28  debian-nosh        drwxr-xr-x   - anthony 22 Jun 17:20  projects    drwxr-xr-x   - anthony 22 Jun 18:58  space
.rw-r--r-- 28k anthony 14 Jun 20:52  guix-install.sh    drwxr-xr-x   - anthony 23 Jun 19:05  soccoop     drwxr-xr-x   - anthony 25 Jun 23:17  vpnn
ysh ysh-0.22.0$ env EZA_GRID_ROWS=666 eza --long --grid
drwxr-xr-x   - anthony 14 Jun 04:28  debian-nosh        drwxr-xr-x   - anthony 22 Jun 17:20  projects    drwxr-xr-x   - anthony 22 Jun 18:58  space
.rw-r--r-- 28k anthony 14 Jun 20:52  guix-install.sh    drwxr-xr-x   - anthony 23 Jun 19:05  soccoop     drwxr-xr-x   - anthony 25 Jun 23:17  vpnn

@tribals

This comment was marked as abuse.

@sbatial
Copy link
Collaborator

sbatial commented Jun 30, 2024

What a problem with that rust programmers?.. Are they really capable of producing at least working as expected software?

Yes, yes they are.

Look, you were even so nice and provided an example of a working rust project, as far as I can tell: #66 (comment)

@tribals

This comment was marked as abuse.

@sbatial
Copy link
Collaborator

sbatial commented Jun 30, 2024

The only thing I can imagine from your example is that your terminal is wide enough that more rows could fit but only a maximum of 3 rows is rendered.

But that is only a suspicion.
Can you elaborate what exactly is wrong (e.g. your terminal is wide enough, your terminal is smaller so too many rows are rendered, etc.)?
Or do you have a screenshot of the output, which is kinda necessary to debug issues with the grid display.

@LeoniePhiline
Copy link
Contributor

LeoniePhiline commented Jun 30, 2024

In which case does terminal_size::terminal_size_using_fd return None? Could the terminal be incompatible?

row_threshold (the parsed value of EZA_GRID_ROWS) is only taken into account when a terminal column width could be determined or is set by the user.

eza/src/main.rs

Line 462 in c58ddcb

row_threshold,

If the terminal width is None, then EZA_GRID_ROWS has no effect. https://github.com/eza-community/eza/blob/main/src/main.rs#L471

Are any other eza or exa environment variables set in the terminal session? Which terminal is in use on which OS?

@LeoniePhiline
Copy link
Contributor

Can you elaborate what exactly is wrong (e.g. your terminal is wide enough, your terminal is smaller so too many rows are rendered, etc.)?

The issue I can see in #66 (comment) is that EZA_GRID_ROWS=N should make eza use grid view only if at least N rows would be displayed, and use non grid row view for shorter result sets.

However, even with EZA_GRID_ROWS=10 or even =666, the result is rendered as grid despite being only 6 items long.

@sbatial
Copy link
Collaborator

sbatial commented Jun 30, 2024

oohhh, you're right.
my apologies, I had remembered the effect the wrong way around

@cafkafk
Copy link
Member Author

cafkafk commented Jul 1, 2024

@tribals has been blocked from all @eza-community projects for the time being, we do not tolerate that kind of behavior

@LeoniePhiline
Copy link
Contributor

I believe EZA_GRID_ROWS is not working correctly:

image

As I understand it, EZA_GRID_ROWS should make the items after the first prompt get printed in list, rather than grid view.

@LeoniePhiline
Copy link
Contributor

row_threshold is passed to grid_details::Render, but it does nothing with it.

image

I believe the match on match (mode, self.console_width) in fn print_files should have an added if guard, comparing row_threshold to the length of the files vector. If the row threshold is not reached, then it should use details::Render rather than grid_details::Render.

@cafkafk
Copy link
Member Author

cafkafk commented Jul 1, 2024

row_threshold is passed to grid_details::Render, but it does nothing with it.

image

I believe the match on match (mode, self.console_width) in fn print_files should have an added if guard, comparing row_threshold to the length of the files vector. If the row threshold is not reached, then it should use details::Render rather than grid_details::Render.

Maybe a separate issue should be opened for this, sounds like you're halfway to a PR as well :)

LeoniePhiline added a commit to LeoniePhiline/eza that referenced this issue Jul 1, 2024
Grid details view had been prevented only by
console width being unavailable.

This changeset implements `EZA_GRID_ROWS`
as secondary grid details inhibitor, preventing
grid details view if the minimum rows threshold
is not reached by the number of files matched.

Fixes complaint at eza-community#66 (comment)
@LeoniePhiline
Copy link
Contributor

LeoniePhiline commented Jul 1, 2024

sounds like you're halfway to a PR as well :)

Nerd-sniped.

Pull request: #1043 (The CI failure of Flake Checker appears unrelated. ➡️ DeterminateSystems/flake-checker-action#32)

I noticed that EZA_GRID_ROWS apparently isn't meant to apply to non-details views? Is that correct? I found the code quite clear about that, due to grid_details::RowThreshold's placement within its grid_details module.

However, from a functionality perspective, as an end user I would expect this to apply to plain EZA_GRID_ROWS=1234 eza as well, without involvement of details views.

image

The second prompt should, to my intuition, result in a list view like eza -1.

LeoniePhiline added a commit to LeoniePhiline/eza that referenced this issue Jul 4, 2024
Grid details view had been prevented only by
console width being unavailable.

This changeset implements `EZA_GRID_ROWS`
as secondary grid details inhibitor, preventing
grid details view if the minimum rows threshold
is not reached by grid which would be rendered.

Fixes complaint at eza-community#66 (comment)
LeoniePhiline added a commit to LeoniePhiline/eza that referenced this issue Jul 4, 2024
Grid details view had been prevented only by
console width being unavailable.

This changeset implements `EZA_GRID_ROWS`
as secondary grid details inhibitor, preventing
grid details view if the minimum rows threshold
is not reached by grid which would be rendered.

Fixes complaint at eza-community#66 (comment)
cafkafk pushed a commit to LeoniePhiline/eza that referenced this issue Aug 7, 2024
Grid details view had been prevented only by
console width being unavailable.

This changeset implements `EZA_GRID_ROWS`
as secondary grid details inhibitor, preventing
grid details view if the minimum rows threshold
is not reached by grid which would be rendered.

Fixes complaint at eza-community#66 (comment)
cafkafk pushed a commit to LeoniePhiline/eza that referenced this issue Aug 7, 2024
Grid details view had been prevented only by console width being
unavailable.

This changeset implements `EZA_GRID_ROWS` as secondary grid details
inhibitor, preventing grid details view if the minimum rows threshold is
not reached by grid which would be rendered.

Fix:
eza-community#66 (comment)
Fix: eza-community#1044

BREAKING CHANGE: Before this change, the `EZA_GRID_ROWS` variable was
ignored, despite documentation existing. Users relying on `EZA_GRID_ROW`
not doing anything will find their output changed. For more info, see
cafkafk pushed a commit to LeoniePhiline/eza that referenced this issue Aug 7, 2024
Grid details view had been prevented only by console width being
unavailable.

This changeset implements `EZA_GRID_ROWS` as secondary grid details
inhibitor, preventing grid details view if the minimum rows threshold is
not reached by grid which would be rendered.

Fix:
eza-community#66 (comment)
Fix: eza-community#1044

BREAKING CHANGE: Before this change, the `EZA_GRID_ROWS` variable was
ignored, despite documentation existing. Users relying on `EZA_GRID_ROW`
not doing anything will find their output changed. For more info, see
cafkafk pushed a commit that referenced this issue Aug 7, 2024
Grid details view had been prevented only by console width being
unavailable.

This changeset implements `EZA_GRID_ROWS` as secondary grid details
inhibitor, preventing grid details view if the minimum rows threshold is
not reached by grid which would be rendered.

Fix:
#66 (comment)
Fix: #1044

BREAKING CHANGE: Before this change, the `EZA_GRID_ROWS` variable was
ignored, despite documentation existing. Users relying on `EZA_GRID_ROW`
not doing anything will find their output changed. For more info, see
erwinvaneijk pushed a commit to erwinvaneijk/eza that referenced this issue Oct 1, 2024
Grid details view had been prevented only by console width being
unavailable.

This changeset implements `EZA_GRID_ROWS` as secondary grid details
inhibitor, preventing grid details view if the minimum rows threshold is
not reached by grid which would be rendered.

Fix:
eza-community#66 (comment)
Fix: eza-community#1044

BREAKING CHANGE: Before this change, the `EZA_GRID_ROWS` variable was
ignored, despite documentation existing. Users relying on `EZA_GRID_ROW`
not doing anything will find their output changed. For more info, see
tertsdiepraam pushed a commit to tertsdiepraam/eza that referenced this issue Oct 10, 2024
Grid details view had been prevented only by console width being
unavailable.

This changeset implements `EZA_GRID_ROWS` as secondary grid details
inhibitor, preventing grid details view if the minimum rows threshold is
not reached by grid which would be rendered.

Fix:
eza-community#66 (comment)
Fix: eza-community#1044

BREAKING CHANGE: Before this change, the `EZA_GRID_ROWS` variable was
ignored, despite documentation existing. Users relying on `EZA_GRID_ROW`
not doing anything will find their output changed. For more info, see
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants