Conversation
stsymbaliuk
left a comment
stsymbaliuk
left a comment
There was a problem hiding this comment.
Hi @giantatwork,
I've verified current commit on my PC and it partly fixes the issue #180 .
With the commit:
- in cell selection mode the current column displayed as expected in the issue;
- in column selection mode behavior stays the same as in the ticket and the current column displays the first visible column on the left and not the real selected column.
And about the commit header "fix issue #180". Please update it to show that this commit fixes the current selected column in the status bar. So it will be easier to see the commit purpose in the git log.
Please check out my latest changes to see if this problem is fixed now. |
|
Thank you @giantatwork, But I've checked behavior when rows filter ( "Col" in status bar does not work as expectedAfter column filter ( Steps:
For reference "Row" in status bar has expected behaviorWhen applied rows filter ( |
Please check my latest changes to see if the column numbering issue in filter mode is now fixed. |
|
Hi @giantatwork, Steps:
|
|
Thank you @giantatwork,
|
YS-L
left a comment
YS-L
left a comment
There was a problem hiding this comment.
Thanks @giantatwork for the PR!
I think the logic added to ui.rs is getting a bit too complex. Ideally, that module should focus purely on rendering. If more complex data massaging is needed before rendering, it would be better to prepare that information externally and pass it in.
| // - Line wrap is off | ||
| // - We're in cell or column selection mode | ||
| // Otherwise fall back to the scroll offset. | ||
| let use_selection_col = !state.enable_line_wrap |
There was a problem hiding this comment.
I'm not sure why enabling / disabling line wrap should change the behavior here. Could you clarify?
There was a problem hiding this comment.
After checking "Col" behavior with -S (Toggle line wrapping) or with -W (Toggle line wrapping by words) I can see that "Col" does not show the currently selected column in Cell or Column selection mode. The next quick change fixed the issue for me.
diff --git a/src/ui.rs b/src/ui.rs
index 02e576a..c0e4f8a 100644
--- a/src/ui.rs
+++ b/src/ui.rs
@@ -804,7 +804,8 @@ impl<'a> CsvTable<'a> {
// - Line wrap is off
// - We're in cell or column selection mode
// Otherwise fall back to the scroll offset.
- let use_selection_col = !state.enable_line_wrap
+ let use_selection_col = true
&& matches!(
state.selection.as_ref().map(|s| s.selection_type()),
Some(view::SelectionType::Cell | view::SelectionType::Column)There was a problem hiding this comment.
Upon further review, I believe this condition is redundant and can be removed.
| if self.disabled_because_no_match { | ||
| line += "no match, showing all columns]"; | ||
| } else { | ||
| line += format!("{}/{} cols]", self.shown, self.total).as_str(); |
There was a problem hiding this comment.
This was actually intended to be simple static information about the number of matches, independent of the selected column.
There was a problem hiding this comment.
This was actually intended to be simple static information about the number of matches, independent of the selected column.
In my opinion original implementation for Filter Column status was not consistent with Filter Rows status.
For example when Rows Filter &New and Columns Filter *city_ascii|state|county applied at the same time we can see status for both of them. And Filter Rows status dynamically shows the current selected row and number of row matches. So to be consistent Filter Column status should show selected column and number of column matches as currently implemented in 3181ec0
[Row 575/31254, Col 2/17] [Filter "New" in city_ascii: 11/416] [Filter "city_ascii|state|county": 1/5 cols]
There was a problem hiding this comment.
Ah that makes sense to me now. Thanks for clarifying!
I agree that this logic should be moved to a file other than |
3 participants
Hello @YS-L,
This PR tries to fix the problem described in #180.
Please let me know if this is the right fix for this problem.
Kind regards,
Previous behavior: the status bar “Col” value always showed the scroll offset (first visible non-frozen column as 1). In column mode this often lagged or restarted on viewport scrolls, and didn’t reflect the selected column. In wrapped/filtered views it stayed offset-based.
New behavior: the status bar shows the actual selected column number when in cell mode (row + column), with line wrap off and no active filter; otherwise it keeps the offset-based display for consistency in row/column-only modes, wrapped views, and filters. This makes column numbers update correctly during navigation while preserving expected wrapped/filter behavior.