Remember meter positions across layout changes#1993
Conversation
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
Interesting approach, but something doesn't quite feel right yet. Haven't looked into it too deeply though, thus mostly first impression rn.
| bool didCopy = false; | ||
| for (size_t i = columns; i < this->header->maxColumns; i++) { | ||
| for (int j = 0; j < Vector_size(this->header->columns[i]); j++) { | ||
| Vector_add(this->header->columns[columns - 1], Vector_get(this->header->columns[i], j)); | ||
| didCopy = true; | ||
| } | ||
| } | ||
| this->header->metersCopied = didCopy; |
There was a problem hiding this comment.
Repeatedly assigning didCopy here feels a bit wasteful, especially if we could get the same value by simply or'ing the Vector_size of each column instead. Also given Vector_add may repeatedly resize the backing array it might be preferable if we could somehow forward the needed amount of memory and reserve a properly sized array beforehand. Not much of an performance issue in practice probably, but at least feels cleaner.
There was a problem hiding this comment.
Thanks, good point. Replaced didCopy with a pre-count of hidden meters and switched from manual Vector_add loop to Vector_splice. I considered dropping the count loop entirely (just checking columns < maxColumns), but kept it for clarity. It makes the intent explicit and avoids unnecessary owner flag toggling when there's nothing to copy
| Header_forEachColumn(this, i) { | ||
| for (size_t i = 0; i < this->maxColumns; i++) { |
There was a problem hiding this comment.
Header_forEachColumn only iterates over visible columns. Since hidden columns now stay allocated in memory, Header_delete needs to free all of them up to maxColumns
|
|
||
| #include "MetersPanel.h" | ||
|
|
||
| #include <stdbool.h> |
There was a problem hiding this comment.
Already included via MetersPanel.h
📝 WalkthroughWalkthroughRemember original meter column assignments during header-layout previews by splicing hidden internal columns into the last visible column, tracking that with ChangesMeter Column Position Memory During Layout Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CategoriesPanel.c`:
- Around line 39-40: The teardown leaves header->metersCopied true after calling
Header_undoMetersCopy(), so the normal draw path still thinks meters are in
copied state and doesn't merge restored columns; update the teardown to clear
the flag by setting this->header->metersCopied = 0 (or false) immediately after
calling Header_undoMetersCopy(), ensuring Header_undoMetersCopy(this->header) is
invoked then the metersCopied field is reset so the draw/layout logic treats
meters as un-copied.
In `@Header.c`:
- Around line 179-183: The loop that accumulates len currently always adds sizes
of hidden columns when iterating past the last visible column (using variables
drawCol, col, numCols, this->maxColumns, and Vector_size), which double-counts
those entries if metersCopied is true; update the loop condition to skip
counting hidden columns when metersCopied is true (e.g., require &&
!metersCopied in the do/while condition) and apply the same fix to the analogous
block that computes colSettings->len later (lines referenced around drawCol use
at 193-210) so colSettings->len matches the actual serialized entries.
- Around line 98-106: The assert in Header_collapseLayout
(assert(this->metersCopied || currentColumns == this->maxColumns)) is too strict
for new call paths (AvailableMetersPanel / MetersPanel clear metersCopied before
invoking collapse); remove or relax that assertion so collapsing proceeds even
when metersCopied is false. Replace the assert with no-op or a runtime guard
that does not abort (e.g., drop the assert entirely) and let the existing
cleanup loop and xReallocArray behavior run normally so reduced layouts with
empty/hidden columns are collapsed without triggering an assertion failure.
In `@MetersPanel.c`:
- Around line 140-143: The code sets modified/result unconditionally after
calling Vector_moveUp/Vector_moveDown, but those functions are no-ops at list
boundaries; change the logic in the handlers that call
Vector_moveUp/Panel_moveSelectedUp and Vector_moveDown/Panel_moveSelectedDown so
you only set modified = true and result = HANDLED when a real move
occurred—detect this by checking whether the item's index changed (compare the
selected index before and after, or use the move function's boolean return if
available) and only mark modified/handled when the index actually differs; apply
the same fix to the symmetric block around the Vector_moveDown calls (also noted
for lines 152-155).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d88b7fb-52d3-4d81-8e57-bf429d48bab0
📒 Files selected for processing (5)
AvailableMetersPanel.cCategoriesPanel.cHeader.cHeader.hMetersPanel.c
There was a problem hiding this comment.
♻️ Duplicate comments (1)
MetersPanel.c (1)
9-9: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUnusual include order.
Vector.his included beforeMetersPanel.h(line 12), which breaks the typical convention of including the corresponding header first afterconfig.h. IfVector.his already transitively included viaMetersPanel.h, this include may be redundant.Suggested reordering
-#include "Vector.h" `#include` "config.h" // IWYU pragma: keep `#include` "MetersPanel.h"If
Vector.his genuinely needed explicitly, move it belowMetersPanel.h:`#include` "config.h" // IWYU pragma: keep `#include` "MetersPanel.h" +#include "Vector.h" `#include` <stdlib.h>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetersPanel.c` at line 9, Move the include of Vector.h so the translation unit's corresponding header MetersPanel.h is included first (after config.h): either reorder the includes so MetersPanel.h appears before Vector.h, or remove the Vector.h include entirely if all Vector symbols are already provided transitively via MetersPanel.h; also verify any use of Vector types/functions in functions like those in MetersPanel.c require an explicit include and add Vector.h only if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@MetersPanel.c`:
- Line 9: Move the include of Vector.h so the translation unit's corresponding
header MetersPanel.h is included first (after config.h): either reorder the
includes so MetersPanel.h appears before Vector.h, or remove the Vector.h
include entirely if all Vector symbols are already provided transitively via
MetersPanel.h; also verify any use of Vector types/functions in functions like
those in MetersPanel.c require an explicit include and add Vector.h only if
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d7b57f7-24c7-4afa-9ee0-4cc70c960058
📒 Files selected for processing (5)
AvailableMetersPanel.cCategoriesPanel.cHeader.cHeader.hMetersPanel.c
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Header.c (1)
245-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the hidden-column traversal in
Header_updateData().These new merged draw paths now surface meters from
columns[currentColumns..maxColumns)when!metersCopied, butHeader_updateData()still only walks the visible columns. After shrinking the header, preserved hidden meters will keep drawing stale values until something else rebuilds them.Suggested fix
void Header_updateData(Header* this) { + const size_t numCols = HeaderLayout_getColumns(this->headerLayout); Header_forEachColumn(this, col) { - Vector* meters = this->columns[col]; - int items = Vector_size(meters); - for (int i = 0; i < items; i++) { - Meter* meter = (Meter*) Vector_get(meters, i); - Meter_updateValues(meter); - } + size_t drawCol = col; + do { + Vector* meters = this->columns[drawCol++]; + for (int i = 0; i < Vector_size(meters); i++) { + Meter* meter = (Meter*) Vector_get(meters, i); + Meter_updateValues(meter); + } + } while ((col == numCols - 1) && (drawCol < this->maxColumns) && !this->metersCopied); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Header.c` around lines 245 - 277, Header_forEachColumn's draw path now iterates meters in columns[currentColumns..maxColumns) when !metersCopied, but Header_updateData still only updates visible columns, so hidden preserved meters keep stale values; update Header_updateData to mirror the same traversal: when this->metersCopied is false, iterate drawCol from the current column index through this->maxColumns (using the same loop condition as Header_forEachColumn) and process/update each Meter in this->columns[drawCol] (honoring Meter_isMultiColumn and columnWidthCount logic if relevant), otherwise keep the existing visible-only behavior; reference Header_forEachColumn, Header_updateData, this->metersCopied, currentColumns, maxColumns, columns, and Meter_isMultiColumn to align the traversal and avoid stale meters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Header.h`:
- Around line 18-21: Header.h uses the bool type in the Header_ struct
(metersCopied) but doesn't include <stdbool.h>, creating fragile transitive
include dependencies; update Header.h to directly include <stdbool.h> so the
bool definition is explicit for the Header_ struct and any code including
Header.h (referencing Header_, metersCopied, and the Header.h interface) no
longer relies on other headers to provide bool.
In `@MetersPanel.c`:
- Around line 200-203: The condition that sets header->metersCopied to false
currently only runs when (modified || sideMove), which misses style-only edits
(the F4/space path) that also mutate a meter; update the logic so style-only
changes are treated as a real modification—either expand the condition to
include the style-change flag used by the F4/space path (e.g., styleChanged or
whatever symbol marks style edits) or call Header_collapseLayout(header);
header->metersCopied = false unconditionally after any code path that mutates
meter style; key symbols to touch: the conditional around
Header_collapseLayout(header) and header->metersCopied assignment, and the
style-change handler invoked by F4/space.
---
Outside diff comments:
In `@Header.c`:
- Around line 245-277: Header_forEachColumn's draw path now iterates meters in
columns[currentColumns..maxColumns) when !metersCopied, but Header_updateData
still only updates visible columns, so hidden preserved meters keep stale
values; update Header_updateData to mirror the same traversal: when
this->metersCopied is false, iterate drawCol from the current column index
through this->maxColumns (using the same loop condition as Header_forEachColumn)
and process/update each Meter in this->columns[drawCol] (honoring
Meter_isMultiColumn and columnWidthCount logic if relevant), otherwise keep the
existing visible-only behavior; reference Header_forEachColumn,
Header_updateData, this->metersCopied, currentColumns, maxColumns, columns, and
Meter_isMultiColumn to align the traversal and avoid stale meters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 74c610ff-b15c-401e-8ce1-e48d89ac25a3
📒 Files selected for processing (5)
AvailableMetersPanel.cCategoriesPanel.cHeader.cHeader.hMetersPanel.c
| if (modified || sideMove) { | ||
| Header_collapseLayout(header); | ||
| header->metersCopied = false; | ||
| } |
There was a problem hiding this comment.
Treat style changes as a real meter modification.
The new modified || sideMove gate excludes the F4/space style path even though it mutates a meter. In a reduced layout, that leaves metersCopied intact after an edit, so restoring the layout later revives the remembered hidden column assignment instead of ending the temporary-preservation window described in the PR.
Suggested fix
case ' ':
case KEY_F(4):
case 't': {
if (!Vector_size(this->meters))
break;
Meter* meter = (Meter*) Vector_get(this->meters, selected);
MeterModeId mode = Meter_nextSupportedMode(meter);
Meter_setMode(meter, mode);
Panel_set(super, selected, (Object*) Meter_toListItem(meter, this->moving));
+ modified = true;
result = HANDLED;
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (modified || sideMove) { | |
| Header_collapseLayout(header); | |
| header->metersCopied = false; | |
| } | |
| case ' ': | |
| case KEY_F(4): | |
| case 't': { | |
| if (!Vector_size(this->meters)) | |
| break; | |
| Meter* meter = (Meter*) Vector_get(this->meters, selected); | |
| MeterModeId mode = Meter_nextSupportedMode(meter); | |
| Meter_setMode(meter, mode); | |
| Panel_set(super, selected, (Object*) Meter_toListItem(meter, this->moving)); | |
| modified = true; | |
| result = HANDLED; | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MetersPanel.c` around lines 200 - 203, The condition that sets
header->metersCopied to false currently only runs when (modified || sideMove),
which misses style-only edits (the F4/space path) that also mutate a meter;
update the logic so style-only changes are treated as a real modification—either
expand the condition to include the style-change flag used by the F4/space path
(e.g., styleChanged or whatever symbol marks style edits) or call
Header_collapseLayout(header); header->metersCopied = false unconditionally
after any code path that mutates meter style; key symbols to touch: the
conditional around Header_collapseLayout(header) and header->metersCopied
assignment, and the style-change handler invoked by F4/space.
When the user reduces the number of header columns, keep the hidden columns allocated in memory instead of collapsing them into the last visible column. This allows restoring meter positions when switching back to the original layout. Fixes htop-dev#1941 Signed-off-by: ugolnikovE <evgeny.ugolnik@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MetersPanel.c (1)
121-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStyle changes (F4/space) should set
modified = true.The style change handler mutates a meter but doesn't set
modified = true. With the current logic, changing a meter's display style won't triggerHeader_collapseLayout, so hidden columns will remain allocated andmetersCopiedwill stay intact. Per the PR objectives, any meter modification in the editor should permanently collapse hidden columns.🐛 Proposed fix
case ' ': case KEY_F(4): case 't': { if (!Vector_size(this->meters)) break; Meter* meter = (Meter*) Vector_get(this->meters, selected); MeterModeId mode = Meter_nextSupportedMode(meter); Meter_setMode(meter, mode); Panel_set(super, selected, (Object*) Meter_toListItem(meter, this->moving)); + modified = true; result = HANDLED; break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MetersPanel.c` around lines 121 - 131, The style-change branch (cases ' ', KEY_F(4), 't') mutates a meter via Meter_nextSupportedMode/Meter_setMode and updates the list item with Panel_set/Meter_toListItem but doesn't mark the editor state as modified; update that branch to set modified = true after applying the mode change (before result = HANDLED) so Header_collapseLayout and subsequent collapse logic will run for this mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@MetersPanel.c`:
- Around line 121-131: The style-change branch (cases ' ', KEY_F(4), 't')
mutates a meter via Meter_nextSupportedMode/Meter_setMode and updates the list
item with Panel_set/Meter_toListItem but doesn't mark the editor state as
modified; update that branch to set modified = true after applying the mode
change (before result = HANDLED) so Header_collapseLayout and subsequent
collapse logic will run for this mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48f8a479-acc4-4e04-a262-170ec4a91de9
📒 Files selected for processing (5)
AvailableMetersPanel.cCategoriesPanel.cHeader.cHeader.hMetersPanel.c
|
@BenBE applied your suggestions (Vector_splice, preallocation, didCopy cleanup). One design note: I kept the current behavior on meter style changes - they change rendering, not layout position, so the collapse path stays bound to position-affecting actions only. Ready for another look when you have time. |
2 participants
Fixes #1941
Based on @Explorer09's suggestion: instead of collapsing meters into the last visible column when the layout is reduced, keep all columns allocated in memory. The rendering layer merges hidden columns visually. Switching back to the original layout restores meter positions.
Hidden columns are permanently collapsed if the user modifies meters in the editor or exits htop.
Summary by CodeRabbit
Bug Fixes
Refactor