Skip to content

CommandScreen: handle multi-byte UTF-8 code sequences while wrapping #1726

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CommandScreen: handle multi-byte UTF-8 code sequences while wrapping #1726

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 95 additions & 28 deletions CommandScreen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,58 +11,125 @@ in the source distribution for its full text.
#include "CommandScreen.h"

#include <assert.h>
#include <locale.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <wchar.h>
#include <wctype.h>

#include "CRT.h"
#include "Macros.h"
#include "Panel.h"
#include "ProvideCurses.h"


static void CommandScreen_scan(InfoScreen* this) {
Panel* panel = this->display;
int idx = MAXIMUM(Panel_getSelectedIndex(panel), 0);
Panel_prune(panel);
static int CommandScreen_scanAscii(InfoScreen* this, const char* p, size_t total, char* line) {
int line_offset = 0, line_size = 0, last_spc_offset = -1;
for (size_t i = 0; i < total; i++, line_offset++) {
assert(line_offset >= 0 && (size_t)line_offset <= total);
char c = line[line_offset] = p[i];
if (c == ' ') {
last_spc_offset = line_offset;
}

const char* p = Process_getCommand(this->process);
if (line_offset == COLS) {
line_size = (last_spc_offset == -1) ? line_offset : last_spc_offset;
line[line_size] = '\0';
InfoScreen_addLine(this, line);

size_t line_maxlen = COLS < 40 ? 40 : COLS;
size_t line_offset = 0;
size_t last_space = 0;
char* line = xCalloc(line_maxlen + 1, sizeof(char));
line_offset -= line_size;
last_spc_offset = -1;
memcpy(line, p + i - line_offset, line_offset + 1);
}
}

for (; *p != '\0'; p++) {
if (line_offset >= line_maxlen) {
assert(line_offset <= line_maxlen);
assert(last_space <= line_maxlen);
return line_offset;
}

size_t line_len = last_space <= 0 ? line_offset : last_space;
char tmp = line[line_len];
line[line_len] = '\0';
InfoScreen_addLine(this, line);
line[line_len] = tmp;
#ifdef HAVE_LIBNCURSESW

static int CommandScreen_scanWide(InfoScreen* this, const char* p, size_t total, char* line) {
mbstate_t state;
memset(&state, 0, sizeof(state));
int line_cols = 0, line_offset = 0, line_size = 0, width = 1;
int last_spc_cols = -1, last_spc_offset = -1;
for (size_t i = 0, bytes = 1; i < total; bytes = 1, width = 1) {
assert(line_offset >= 0 && (size_t)line_offset <= total);
unsigned char c = (unsigned char)p[i];
if (c < 0x80) { // skip mbrtowc for ASCII characters
line[line_offset] = c;
if (c == ' ') {
last_spc_offset = line_offset;
last_spc_cols = line_cols;
}
} else {
wchar_t wc;
bytes = mbrtowc(&wc, p + i, total - i, &state);
if (bytes != (size_t)-1 && bytes != (size_t)-2) {
width = wcwidth(wc);
width = MAXIMUM(width, 1);
} else {
bytes = 1;
}
memcpy(line + line_offset, p + i, bytes);
}

assert(line_len <= line_offset);
line_offset -= line_len;
memmove(line, line + line_len, line_offset);
i += bytes;
line_offset += bytes;
line_cols += width;
if (line_cols < COLS) {
Copy link
Member

Choose a reason for hiding this comment

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

COLS should ideally be an argument to the string wrap function.

continue;
}

last_space = 0;
if (last_spc_offset >= 0) { // wrap by last space
line_size = last_spc_offset;
line_cols -= last_spc_cols;
last_spc_offset = last_spc_cols = -1;
} else if (line_cols > COLS) { // wrap current wide char to next line
line_size = line_offset - (int) bytes;
line_cols = width;
} else { // wrap by current character
line_size = line_offset;
line_cols = 0;
}

line[line_offset++] = *p;
if (*p == ' ') {
last_space = line_offset;
line[line_size] = '\0';
InfoScreen_addLine(this, line);
line_offset -= line_size;
if (line_offset > 0) {
memcpy(line, p + i - line_offset, line_offset + 1);
}
}

return line_offset;
}

#endif // HAVE_LIBNCURSESW

static void CommandScreen_scan(InfoScreen* this) {
Panel* panel = this->display;
int idx = Panel_getSelectedIndex(panel);
Panel_prune(panel);

const char* p = Process_getCommand(this->process);
assert(p != NULL);
size_t total = strlen(p);
char line[total + 1];
Copy link
Member

Choose a reason for hiding this comment

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

Avoid variable length arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to be line[COL + 1], however due to it's possible for a unicode character to have arbitrary combining marks, like diacritics, therefore COL + 1 is not sufficient and would cause segfault. therefore strlen(p) + 1 is probably the only safe option out there.

With that said, I wonder if you mean to use malloc instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

The strlen(p) is a bad idea as it scans the entire string rather than just stops at the potential line breaking position.

You can reference my code String_mbswidth in #1642 if you need some inspiration.

Copy link
Member

Choose a reason for hiding this comment

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

The comment is about the VLA itself. And yes, a malloc is the preferred way.

That way you avoid having any dynamically sized buffers anywhere near the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I just don't like duplicated efforts. I remembered I have done this "calculate string width and length with respect to whitespace line break" before. It was in #1648. I just need to split that out into a dedicated function so that it can be reused in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an infrastructural change needed for htop to fully support Unicode.

Copy link
Member

Choose a reason for hiding this comment

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

Haha, maybe we could just wait for String_split to magically appear :P

There is an existing function String_split. I meant more like creating a String_wrap function with similar API.

Jokes aside, I honestly don't have enough spare time right now to implement a String_split that would cover all use cases throughout the entire htop codebase.

If a first version only covers the cases used on CommandScreen.c that's totally fine. @Explorer09 can take up the work from there if you like.

It seems like #1105 ran into similar challenges and ended up stalled, which is unfortunate.

Multibyte support is quite tricky to get right, in particular when dealing with input handling.

Copy link
Contributor Author

@ryenus ryenus Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If a first version only covers the cases used on CommandScreen.c that's totally fine. @Explorer09 can take up the work from there if you like.

Yes, that would be great :-)

Copy link
Contributor

@Explorer09 Explorer09 Jun 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I made String_lineBreakWidth() in 2ddc311 that should be able to work with your use case.

Copy link
Member

Choose a reason for hiding this comment

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

The stack buffer is still there …


int line_offset =
#ifdef HAVE_LIBNCURSESW
CRT_utf8 ? CommandScreen_scanWide(this, p, total, line) :
#endif
CommandScreen_scanAscii(this, p, total, line);

assert(line_offset >= 0 && (size_t)line_offset <= total);
if (line_offset > 0) {
line[line_offset] = '\0';
InfoScreen_addLine(this, line);
}

free(line);

Panel_setSelected(panel, idx);
Panel_setSelected(panel, MAXIMUM(idx, 0));
}

static void CommandScreen_draw(InfoScreen* this) {
Expand Down
Loading