Implement line editor for Search, Filter and (re)naming Screens#1933
Implement line editor for Search, Filter and (re)naming Screens#1933fasterit wants to merge 1 commit intohtop-dev:mainfrom
Conversation
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
Mind to split the Line Editor and History changes into two commits?
CommandLine.c
Outdated
| snprintf(historyPath, sizeof(historyPath), "%.*s%s", dirLen, rcPath, "htop_history"); | ||
| } else { | ||
| snprintf(historyPath, sizeof(historyPath), "htop_history"); | ||
| } |
There was a problem hiding this comment.
| } | |
| if (lastSlash) { | |
| int dirLen = (int)(lastSlash - rcPath + 1); | |
| xSnprintf(historyPath, sizeof(historyPath), "%.*s" "htop_history", dirLen, rcPath); | |
| } else { | |
| xSnprintf(historyPath, sizeof(historyPath), "htop_history"); | |
| } |
IncSet.c
Outdated
| } | ||
|
|
||
| void IncSet_drawBar(const IncSet* this, int attr) { | ||
| void IncSet_drawBar(IncSet* this, int attr) { |
There was a problem hiding this comment.
Could this draw call be kept with const IncSet* somehow?
There was a problem hiding this comment.
That'll be a bit involved, need to get rid of fieldStartX. I'll give it a try.
Yes, too much work. Line editor never existed without history. |
History.h
Outdated
| size_t capacity; /* allocated capacity */ | ||
| int position; /* current browse position: count = "at new input" */ | ||
| char saved[LINEEDITOR_MAX + 1]; /* saved current input while browsing */ | ||
| char* filename; /* path to history file (may be NULL) */ |
There was a problem hiding this comment.
How about documenting what would happen if this filename field is NULL then?
There was a problem hiding this comment.
It would gracefully not be read / written (but CommandLine.c will setup the filename so it won't be empty). I'll document it.
|
Added a separate commit for implementing the review feedback, pls. squash on merge |
| snprintf(historyPath, sizeof(historyPath), "htop_history"); | ||
| } | ||
| IncSet_setHistoryFile(panel->inc, historyPath); | ||
| const char* rcPath = settings->filename; |
There was a problem hiding this comment.
Observe these lines in the Settings_new function:
this->filename = xMalloc(PATH_MAX);
if (!realpath(this->initialFilename, this->filename))
free_and_xStrdup(&this->filename, this->initialFilename);So a call like dirname(settings->filename) might not give you the htop directory you wanted, if the htoprc file is a symlink.
I was thinking if it's necessary to add the htopConfigDir field in the Settings object. It might be better in the long run if there's a central place to grab the ~/.config/htop directory path.
There was a problem hiding this comment.
If people link to a central htoprc that does not mean they want a central history, does it?
As of now symlinking htop_history would not work and we'd have all kinds of issues (like the .bash_history being overwritten by terminal sessions and not combined, same thing here).
|
^ typo fix in commit message |
|
Squashed and rebased onto current main |
4 participants
htop_history)The Ctrl-left / Ctrl-right handling is xterm-compatible terminals only for now. Can be extended to other terminals if people care.
The Shift-left / Shift-right are stock ncurses from the extended set, so that should work everywhere™ but nobody used these for movement any more, I guess.
Assisted-by: Microsoft Copilot/Claude Sonnet 4.6