dnkl
/
foot
15
251
Fork 66
Code Issues 40 Pull Requests 11 Releases 38 Wiki Activity
Labels Milestones

Send text_input_rectangle requests for text-input #253

Manually merged
dnkl merged 1 commits from tdeo/foot:text-input-rectangle into master 1 year ago
Conversation 12 Commits 1 Files Changed 7
tdeo commented 1 year ago

This enables input method popups to show in the correct location.

Closes #309

This enables input method popups to show in the correct location. Closes #309
dnkl requested changes 1 year ago
dnkl left a comment
Owner

Looks nice and clean!

I haven't actually tested it yet, and I'll need to benchmark it as well, before it can be merged. But looks good so far!

Oh, and another thing: please add an entry to CHANGELOG.md as well

Looks nice and clean! I haven't actually tested it yet, and I'll need to benchmark it as well, before it can be merged. But looks good so far! Oh, and another thing: please add an entry to CHANGELOG.md as well
ime.c Outdated
bool
ime_check_cursor_rect(struct seat *seat, struct terminal *term)
{
dnkl commented 1 year ago
Owner

I'm not too fond of the name; it doesn't say what it is checking, and doesn't imply anything is modified.

But I can't really come up with a better name right now. If you can come up with something better, that's great, otherwise we can keep it as-is for the time being.

I'm not too fond of the name; it doesn't say _what_ it is checking, and doesn't imply anything is modified. But I can't really come up with a better name right now. If you can come up with something better, that's great, otherwise we can keep it as-is for the time being.
tdeo commented 1 year ago
Poster

Yeah, I'm not sure either.

Yeah, I'm not sure either.
dnkl marked this conversation as resolved
ime.c Outdated
int x =
term->margins.left + term->grid->cursor.point.col * term->cell_width;
int y =
term->margins.top + term->grid->cursor.point.row * term->cell_height;
dnkl commented 1 year ago
Owner

y will be incorrect here, if the user has scrolled up in the scrollback history. You will need to adjust it, see b25b201819/render.c (L1987)

`y` will be incorrect here, if the user has scrolled up in the scrollback history. You will need to adjust it, see https://codeberg.org/dnkl/foot/src/commit/b25b201819546c1f4daece057e55e63212bff878/render.c#L1987
tdeo marked this conversation as resolved
render.c Outdated
tll_foreach(term->wl->seats, it)
if (it->item.kbd_focus == term)
ime_update_cursor_rect(&it->item, term);
dnkl commented 1 year ago
Owner

This looping of seats is a bit unfortunate. But there's currently no other way to do it. I think we should change that, but I also think that's better done in a follow up PR. So let's keep this the way it is right now.

This looping of seats is a bit unfortunate. But there's currently no other way to do it. I think we should change that, but I also think that's better done in a follow up PR. So let's keep this the way it is right now.
dnkl commented 1 year ago
Owner

Actually, it might make more sense to keep things the way they are; 99% of all systems will have exactly one seat - tracking which seats are currently focusing a specific terminal instance just adds code complexity (we'd need to sync the seat list in the terminal instances), and increases the memory foot print.

The remaining %1 is likely to have, what, 2-3 seats? That's unlikely to be a performance problem.

Actually, it might make more sense to keep things the way they are; 99% of all systems will have exactly one seat - tracking _which_ seats are currently focusing a specific terminal instance just adds code complexity (we'd need to sync the seat list in the terminal instances), and increases the memory foot print. The remaining %1 is likely to have, what, 2-3 seats? That's unlikely to be a performance problem.
dnkl marked this conversation as resolved
dnkl commented 1 year ago
Owner

I'll need to benchmark it as well

Tested briefly with anthywl. Didn't play around long enough to figure out how to get a popup so haven't been able to test/verify popup positioning.

But benchmarks are looking good; no measurable differences with or without this PR.

> I'll need to benchmark it as well Tested briefly with anthywl. Didn't play around long enough to figure out how to get a popup so haven't been able to test/verify popup positioning. But benchmarks are looking good; no measurable differences with or without this PR.
tdeo force-pushed text-input-rectangle from 93cb34487c to 3c7d4b0a46 1 year ago
tdeo commented 1 year ago
Poster

Tested briefly with anthywl. Didn't play around long enough to figure out how to get a popup so haven't been able to test/verify popup positioning.

It needs a sway patch which you might not have.

> Tested briefly with anthywl. Didn't play around long enough to figure out how to get a popup so haven't been able to test/verify popup positioning. It needs [a sway patch](https://github.com/swaywm/sway/pull/5890) which you might not have.
tdeo force-pushed text-input-rectangle from b32bb7e4ae to 4f26c7e856 1 year ago
tdeo commented 1 year ago
Poster

When scrolling up, the popup shows up outside of the window with my Sway patch, but I believe this is a problem with the Sway patch and not with sending those bounds.

GTK seems to send out-of-bounds coordinates too, and it lets the compositor hide the popup rather than keeping it in an outdated location or the top corner.

When scrolling up, the popup shows up outside of the window with my Sway patch, but I believe this is a problem with the Sway patch and not with sending those bounds. GTK seems to send out-of-bounds coordinates too, and it lets the compositor hide the popup rather than keeping it in an outdated location or the top corner.
dnkl commented 1 year ago
Owner

It needs a sway patch which you might not have.

Thanks, you're right, I did not.

When scrolling up, the popup shows up outside of the window with my Sway patch, but I believe this is a problem with the Sway patch and not with sending those bounds.

Ok, sounds reasonable. I don't use IME that much personally, but I do think it makes sense to "scoll out" the popup along with the cursor, and hiding it when outside the windo, rather than bounding it to the window dimensions.

> It needs a sway patch which you might not have. Thanks, you're right, I did not. > When scrolling up, the popup shows up outside of the window with my Sway patch, but I believe this is a problem with the Sway patch and not with sending those bounds. Ok, sounds reasonable. I don't use IME that much personally, but I do think it makes sense to "scoll out" the popup along with the cursor, and hiding it when outside the windo, rather than bounding it to the window dimensions.
dnkl reviewed 1 year ago
ime.c Outdated
row &= term->grid->num_rows - 1;
if (term->hide_cursor)
col = row = 0;
dnkl commented 1 year ago
Owner

Can you explain the reasoning behind this? Why not use the correct coordinates even if the cursor is hidden?

Can you explain the reasoning behind this? Why not use the correct coordinates even if the cursor is hidden?
tdeo commented 1 year ago
Poster

I've removed this now, it's true that the top left is probably worse than wherever the cursor would be.

I've removed this now, it's true that the top left is probably worse than wherever the cursor would be.
tdeo marked this conversation as resolved
dnkl requested changes 1 year ago
ime.c Outdated
void
ime_update_cursor_rect(struct seat *seat, struct terminal *term)
{
if (!ime_check_cursor_rect(seat, term))
dnkl commented 1 year ago
Owner

We need to check that the text-input interface is available, and just return here if it isn't. See #260, and #259

We need to check that the text-input interface is available, and just return here if it isn't. See https://codeberg.org/dnkl/foot/pulls/260, and https://codeberg.org/dnkl/foot/issues/259
tdeo marked this conversation as resolved
tdeo force-pushed text-input-rectangle from 4f26c7e856 to c435765379 1 year ago
dnkl requested changes 1 year ago
ime.c Outdated
row += term->grid->offset;
row -= term->grid->view;
row &= term->grid->num_rows - 1;
dnkl commented 1 year ago
Owner

I've finally been able to test this with your Sway+wlroots patches, and that made me realize that this doesn't work in scrollback search mode.

I.e. if you do Ctrl+Shift+R to enter search mode, then start typing, the popup is positioned near the terminal cursor, not the search input "box".

Positioning the popup for the search box may be tricky, depending on how fine grained you want it. There is state that can be used to derive the cursor position, but it isn't straight forward. The state you want to use is:

  • term->render.search_glyph_offset: this is the offset into the seach input string where we start rendering (i.e if the search string is wider than the window, we may for example only render the end of it).
  • term->search.cursor: where in the search input string new characters are inserted.

What makes things complicated is that the glyph offset is in cells, not characters, and the cursor is in characters, not cells.

Let's say the search input string is "😊|", with the bar representing the cursor. The cursor would in this case be 1. The corresponding glyph offset is 2.

The name, search_glyph_offset, is a bad name, but was introduced before the search mode handled multi-column characters.

Other options might be to position the popup at the right margin. Or maybe at the beginning of the search input box, although that may prove to be annoying as the left side of the input box moves as the user types (until it reaches the left side of the window).

I've finally been able to test this with your Sway+wlroots patches, and that made me realize that this doesn't work in scrollback search mode. I.e. if you do <kbd>Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>R</kbd> to enter search mode, then start typing, the popup is positioned near the terminal cursor, not the search input "box". Positioning the popup for the search box may be tricky, depending on how fine grained you want it. There is state that can be used to derive the cursor position, but it isn't straight forward. The state you want to use is: * `term->render.search_glyph_offset`: this is the offset into the seach input string where we start rendering (i.e if the search string is wider than the window, we may for example only render the end of it). * `term->search.cursor`: where in the search input string new characters are inserted. What makes things complicated is that the glyph offset is in **cells**, not characters, and the cursor is in **characters**, not cells. Let's say the search input string is "😊|", with the bar representing the cursor. The cursor would in this case be `1`. The corresponding glyph offset is `2`. The name, `search_glyph_offset`, is a bad name, but was introduced before the search mode handled multi-column characters. Other options might be to position the popup at the right margin. Or maybe at the beginning of the search input box, although that may prove to be annoying as the left side of the input box moves as the user types (until it reaches the left side of the window).
tdeo marked this conversation as resolved
tdeo force-pushed text-input-rectangle from c435765379 to 819c8e8736 1 year ago
dnkl requested changes 1 year ago
dnkl left a comment
Owner

Works very well, nicely done! Just two minor nitpicks...

Works very well, nicely done! Just two minor nitpicks...
CHANGELOG.md
* IME popup location support: foot now sends the location of the cursor
so any popup can be displayed near the text that is being typed.
dnkl commented 1 year ago
Owner

Forgot to tell you before: feel free to add yourself to the contributors list a bit further down, if you like.

Forgot to tell you before: feel free to add yourself to the **contributors** list a bit further down, if you like.
tdeo marked this conversation as resolved
ime.c Outdated
size_t post_preedit_cells =
wcswidth(&term->search.buf[term->search.cursor],
term->search.len - term->search.cursor);
dnkl commented 1 year ago
Owner

wcswidth() returns an int, and will return -1 if the string contains a non-printable character. I'm wondering if we should at least convert -1 to 0, since -1 in a size_t is a really large value...

To trigger, press ctrl+shift+v (instead of ctrl+v, to paste)

`wcswidth()` returns an `int`, and will return `-1` if the string contains a non-printable character. I'm wondering if we should at least convert `-1` to `0`, since `-1` in a `size_t` is a **really** large value... To trigger, press <kbd>ctrl</kbd>+<kbd>shift</kbd>+<kbd>v</kbd> (instead of <kbd>ctrl</kbd>+<kbd>v</kbd>, to paste)
dnkl commented 1 year ago
Owner

Related: #314

Related: https://codeberg.org/dnkl/foot/pulls/314
tdeo commented 1 year ago
Poster

I think it's better solved by #314 than adding 4 checks here.

I think it's better solved by #314 than adding 4 checks here.
dnkl commented 1 year ago
Owner

Agreed. That should suffice.

Agreed. That should suffice.
dnkl marked this conversation as resolved
ngkz reviewed 1 year ago
ime.c Outdated
seat->ime.cursor_rect.x = x;
seat->ime.cursor_rect.y = y;
seat->ime.cursor_rect.width = width;
seat->ime.cursor_rect.height = height;
ngkz commented 1 year ago

Convert coordination unit to pixel

    seat->ime.cursor_rect.x = x / term->scale;
    seat->ime.cursor_rect.y = y / term->scale;
    seat->ime.cursor_rect.width = width / term->scale;
    seat->ime.cursor_rect.height = height / term->scale;
Convert coordination unit to pixel ``` seat->ime.cursor_rect.x = x / term->scale; seat->ime.cursor_rect.y = y / term->scale; seat->ime.cursor_rect.width = width / term->scale; seat->ime.cursor_rect.height = height / term->scale; ```
ngkz commented 1 year ago

cursor_rect should be the area of the pre-edit cursor when there is the pre-edit string.

`cursor_rect` should be the area of the pre-edit cursor when there is the pre-edit string.
tdeo marked this conversation as resolved
ngkz reviewed 1 year ago
ime.c Outdated
row &= term->grid->num_rows - 1;
x = term->margins.left + col * term->cell_width;
y = term->margins.top + row * term->cell_height;
width = term->width - term->margins.right - x;
ngkz commented 1 year ago

Arguments of set_cursor_rectangle() are cursor area, so width has to the width of the cursor.

Arguments of set_cursor_rectangle() are cursor area, so `width` has to the width of the cursor.
tdeo marked this conversation as resolved
tdeo commented 1 year ago
Poster

Arguments of set_cursor_rectangle() are cursor area, so width has to the width of the cursor.

@ngkz I've now set the cursor width to 0px when there's no preedit text entered.
This is simpler and it seems consistent with what other applications I tested do.

> Arguments of set_cursor_rectangle() are cursor area, so width has to the width of the cursor. @ngkz I've now set the cursor width to 0px when there's no preedit text entered. This is simpler and it seems consistent with what other applications I tested do.
tdeo force-pushed text-input-rectangle from 819c8e8736 to 6c6aee6e62 1 year ago
ngkz commented 1 year ago

Arguments of set_cursor_rectangle() are cursor area, so width has to the width of the cursor.

@ngkz I've now set the cursor width to 0px when there's no preedit text entered.
This is simpler and it seems consistent with what other applications I tested do.

Oh, all gtk widgets except VTE are calling set_cursor_rectangle with width=0. Somehow I hadn't noticed it even though I read the wayland debug log many times.

> > Arguments of set_cursor_rectangle() are cursor area, so width has to the width of the cursor. > > @ngkz I've now set the cursor width to 0px when there's no preedit text entered. > This is simpler and it seems consistent with what other applications I tested do. Oh, all gtk widgets except VTE are calling set_cursor_rectangle with width=0. Somehow I hadn't noticed it even though I read the wayland debug log many times.
ngkz reviewed 1 year ago
ime.c Outdated
x = term->margins.left + col * term->cell_width;
y = term->margins.top + row * term->cell_height;
width = preedit_cells * term->cell_width;
height = term->cell_height;
ngkz commented 1 year ago

What I meant was that it should pass the area of the cursor(▌ or |) at the insertion point to set_cursor_rectangle(), not the area of the pre-edit text. (As you said, it should be set_cursor_rectangle(insertion point x, insertion point y, 0, line height), through)
Otherwise, the suggestion popup and the candidate selection popup won't show at the insertion point like other apps.
Mutter shows the popup at center-top or center-bottom of the cursor rectangle.
Sway looks like that shows the popup at left-bottom of the cursor rectangle.

What I meant was that it should pass the area of ~~the cursor(`▌` or `|`) at~~ the insertion point to set_cursor_rectangle(), not the area of the pre-edit text. (As you said, it should be `set_cursor_rectangle(insertion point x, insertion point y, 0, line height)`, through) Otherwise, the suggestion popup and the candidate selection popup won't show at the insertion point like other apps. Mutter shows the popup at center-top or center-bottom of the cursor rectangle. Sway looks like that shows the popup at left-bottom of the cursor rectangle.
ngkz commented 1 year ago

Aha, I got why I didn't get my point across.
The cursor in foot is a range tuple and it points to whole pre-edit text when the insert point is the end of the text. I've never seen apps like this before.

Aha, I got why I didn't get my point across. The cursor in foot is a range tuple and it points to whole pre-edit text when the insert point is the end of the text. I've never seen apps like this before.
ngkz commented 1 year ago

I've made !term->is_searching path behave like GTKText with this quick-n-dirty patch.
I think that it needs extracting positioning logic from the renderer or letting renderer positioning the cursor rectangle.

    } else {
        int col = term->grid->cursor.point.col;
        int row = term->grid->cursor.point.row;
        row += term->grid->offset;
        row -= term->grid->view;
        row &= term->grid->num_rows - 1;
        if (term->ime.preedit.cells != NULL && !term->ime.preedit.cursor.hidden) {
            /* FIXME: mostly copy-pasted cursor positioning logic */
            int cells_needed = term->ime.preedit.count;

            int ime_ofs = 0;  /* Offset into pre-edit string to start rendering at */

            int cells_left = term->cols - col;
            int cells_used = min(cells_needed, term->cols);

            /* Adjust start of pre-edit text to the left if string doesn't fit on row */
            if (cells_left < cells_used)
                col -= cells_used - cells_left;

            if (cells_needed > cells_used) {
                int start = term->ime.preedit.cursor.start;
                int end = term->ime.preedit.cursor.end;

                if (start == end) {
                    /* Ensure *end* of pre-edit string is visible */
                    ime_ofs = cells_needed - cells_used;
                } else {
                    /* Ensure the *beginning* of the cursor-area is visible */
                    ime_ofs = start;

                    /* Display as much as possible of the pre-edit string */
                    if (cells_needed - ime_ofs < cells_used)
                        ime_ofs = cells_needed - cells_used;
                }

                /* Make sure we don't start in the middle of a character */
                while (ime_ofs < cells_needed &&
                       term->ime.preedit.cells[ime_ofs].wc == CELL_MULT_COL_SPACER)
                {
                    ime_ofs++;
                }
            }

            int end = term->ime.preedit.cursor.end - ime_ofs;
            col += end;
        }
        x = min(max(term->margins.left + col * term->cell_width, 0), term->width);
        y = term->margins.top + row * term->cell_height;
        width = 0;
        height = term->cell_height;
    }
I've made !term->is_searching path behave like GTKText with this quick-n-dirty patch. I think that it needs extracting positioning logic from the renderer or letting renderer positioning the cursor rectangle. ```c } else { int col = term->grid->cursor.point.col; int row = term->grid->cursor.point.row; row += term->grid->offset; row -= term->grid->view; row &= term->grid->num_rows - 1; if (term->ime.preedit.cells != NULL && !term->ime.preedit.cursor.hidden) { /* FIXME: mostly copy-pasted cursor positioning logic */ int cells_needed = term->ime.preedit.count; int ime_ofs = 0; /* Offset into pre-edit string to start rendering at */ int cells_left = term->cols - col; int cells_used = min(cells_needed, term->cols); /* Adjust start of pre-edit text to the left if string doesn't fit on row */ if (cells_left < cells_used) col -= cells_used - cells_left; if (cells_needed > cells_used) { int start = term->ime.preedit.cursor.start; int end = term->ime.preedit.cursor.end; if (start == end) { /* Ensure *end* of pre-edit string is visible */ ime_ofs = cells_needed - cells_used; } else { /* Ensure the *beginning* of the cursor-area is visible */ ime_ofs = start; /* Display as much as possible of the pre-edit string */ if (cells_needed - ime_ofs < cells_used) ime_ofs = cells_needed - cells_used; } /* Make sure we don't start in the middle of a character */ while (ime_ofs < cells_needed && term->ime.preedit.cells[ime_ofs].wc == CELL_MULT_COL_SPACER) { ime_ofs++; } } int end = term->ime.preedit.cursor.end - ime_ofs; col += end; } x = min(max(term->margins.left + col * term->cell_width, 0), term->width); y = term->margins.top + row * term->cell_height; width = 0; height = term->cell_height; } ```
dnkl commented 1 year ago
Owner

The cursor in foot is a range tuple and it points to whole pre-edit text when the insert point is the end of the text

It is a range while pre-editing, and in this case the range is the cursor_begin and cursor_end parameters from text_input_v3::preedit_string(). I.e. it is ultimately controlled by the IME engine.

If either cursor_begin or cursor_end is -1, the cursor is hidden. If they are equal, we render a bar, |, at the indicated pre-edit position. Otherwise we render a rectangle covering the whole pre-edit cursor range. Note that this isn't necessarily the entire pre-edit text - it can be a sub-component of it.

When not pre-editing, the cursor is not a range, just a single point, rendered as either a block, underline or bar, depending on the current cursor style setting.

I think that it needs extracting positioning logic from the renderer or letting renderer positioning the cursor rectangle.

We definitely do not want to duplicate that much logic. Updating the cursor rectangle in the corresponding render functions is fine:

  • render_ime_preedit() could handle the !searching && is_preediting branch
  • render_search_box() could handle the searching (both is_preediting and !is_preediting) branches.
  • The remaining branch, i.e. no pre-edit, no searching, is probably best handled in a separate function, like it is now. It could technically be done in render_cell(), but I rather not add all the if-statements needed (!searching && !is_preediting) there, since that is a performance critical code path.

The first two, render_ime_preedit() and render_search_box() are mutually exclusive; we explicitly reset the IME state when entering/leaving search mode.

> The cursor in foot is a range tuple and it points to whole pre-edit text when the insert point is the end of the text It is a range **while pre-editing**, and in this case the range is the `cursor_begin` and `cursor_end` parameters from `text_input_v3::preedit_string()`. I.e. it is ultimately controlled by the IME engine. If either `cursor_begin` or `cursor_end` is `-1`, the cursor is hidden. If they are equal, we render a bar, `|`, at the indicated pre-edit position. Otherwise we render a rectangle covering the whole pre-edit cursor range. Note that this isn't necessarily the entire pre-edit text - it can be a sub-component of it. When **not** pre-editing, the cursor is **not** a range, just a single point, rendered as either a block, underline or bar, depending on the current cursor style setting. > I think that it needs extracting positioning logic from the renderer or letting renderer positioning the cursor rectangle. We definitely do not want to duplicate that much logic. Updating the cursor rectangle in the corresponding render functions is fine: * `render_ime_preedit()` could handle the `!searching && is_preediting` branch * `render_search_box() `could handle the `searching` (both `is_preediting` and `!is_preediting`) branches. * The remaining branch, i.e. no pre-edit, no searching, is probably best handled in a separate function, like it is now. It could technically be done in `render_cell()`, but I rather not add all the `if`-statements needed (`!searching && !is_preediting`) there, since that is a performance critical code path. The first two, `render_ime_preedit()` and `render_search_box()` are mutually exclusive; we explicitly reset the IME state when entering/leaving search mode.
tdeo commented 1 year ago
Poster

The cursor in foot is a range tuple and it points to whole pre-edit text when the insert point is the end of the text. I've never seen apps like this before.

This is a bug in foot: it doesn't handle the case where the cursor is after the preedit text.

         * Note that this has only been tested with
         *
         *   cursor_begin == cursor_end == 0
         *
         * I haven't found an IME that requests anything else
         */

(anthywl does set both cursor_begin and cursor_end to the end of the pre-edit, and it is handled incorrectly by foot. I didn't realize this was a foot bug until now.)

Oh, all gtk widgets except VTE are calling set_cursor_rectangle with width=0. Somehow I hadn't noticed it even though I read the wayland debug log many times.

Hmm, now that I see that VTE sets the cursor to the cursor block, I think it makes sense for foot to do that too.

> The cursor in foot is a range tuple and it points to whole pre-edit text when the insert point is the end of the text. I've never seen apps like this before. This is a bug in foot: it doesn't handle the case where the cursor is after the preedit text. ``` * Note that this has only been tested with * * cursor_begin == cursor_end == 0 * * I haven't found an IME that requests anything else */ ``` (anthywl does set both cursor_begin and cursor_end to the end of the pre-edit, and it is handled incorrectly by foot. I didn't realize this was a foot bug until now.) > Oh, all gtk widgets except VTE are calling set_cursor_rectangle with width=0. Somehow I hadn't noticed it even though I read the wayland debug log many times. Hmm, now that I see that VTE sets the cursor to the cursor block, I think it makes sense for foot to do that too.
dnkl commented 1 year ago
Owner

It's actually not true (anymore) that foot hasn't been tested with anything but cursor_begin == cursor_end == 0.

But cursor_begin == cursor_end == end-of-preedit hasn't been tested specifically.

It's actually not true (anymore) that foot hasn't been tested with anything but `cursor_begin == cursor_end == 0`. But `cursor_begin == cursor_end == end-of-preedit` hasn't been tested specifically.
dnkl commented 1 year ago
Owner

Hmm, now that I see that VTE sets the cursor to the cursor block, I think it makes sense for foot to do that too.

Which is why it would make sense to update the rectangle in the render_* functions :)

> Hmm, now that I see that VTE sets the cursor to the cursor block, I think it makes sense for foot to do that too. Which is why it would make sense to update the rectangle in the `render_*` functions :)
dnkl commented 1 year ago
Owner

(anthywl does set both cursor_begin and cursor_end to the end of the pre-edit, and it is handled incorrectly by foot. I didn't realize this was a foot bug until now.)

When does it do this? Enabling debug logging in foot:

 dbg: ime.c:273: pre-edit cursor: begin=0, end=1
 dbg: ime.c:49: preedit-string: text=ひ, begin=3, end=3
 dbg: ime.c:105: done: serial=3
 dbg: ime.c:273: pre-edit cursor: begin=0, end=2
 dbg: ime.c:49: preedit-string: text=ひr, begin=4, end=4
 dbg: ime.c:105: done: serial=3
 dbg: ime.c:273: pre-edit cursor: begin=0, end=3
 dbg: ime.c:49: preedit-string: text=ひら, begin=6, end=6
 dbg: ime.c:105: done: serial=3
 dbg: ime.c:273: pre-edit cursor: begin=0, end=4
 dbg: ime.c:49: preedit-string: text=ひらg, begin=7, end=7
 dbg: ime.c:105: done: serial=3
 dbg: ime.c:273: pre-edit cursor: begin=0, end=5
 dbg: ime.c:49: preedit-string: text=ひらが, begin=9, end=9
 dbg: ime.c:105: done: serial=3
 dbg: ime.c:273: pre-edit cursor: begin=0, end=6
 dbg: ime.c:49: preedit-string: text=ひらがn, begin=10, end=10
 dbg: ime.c:105: done: serial=3
 dbg: ime.c:273: pre-edit cursor: begin=0, end=7
 dbg: ime.c:49: preedit-string: text=ひらがな, begin=12, end=12
 dbg: ime.c:105: done: serial=3
 dbg: ime.c:273: pre-edit cursor: begin=0, end=8
 dbg: ime.c:66: commit: text=ひらがな
> (anthywl does set both cursor_begin and cursor_end to the end of the pre-edit, and it is handled incorrectly by foot. I didn't realize this was a foot bug until now.) When does it do this? Enabling debug logging in foot: ``` dbg: ime.c:273: pre-edit cursor: begin=0, end=1 dbg: ime.c:49: preedit-string: text=ひ, begin=3, end=3 dbg: ime.c:105: done: serial=3 dbg: ime.c:273: pre-edit cursor: begin=0, end=2 dbg: ime.c:49: preedit-string: text=ひr, begin=4, end=4 dbg: ime.c:105: done: serial=3 dbg: ime.c:273: pre-edit cursor: begin=0, end=3 dbg: ime.c:49: preedit-string: text=ひら, begin=6, end=6 dbg: ime.c:105: done: serial=3 dbg: ime.c:273: pre-edit cursor: begin=0, end=4 dbg: ime.c:49: preedit-string: text=ひらg, begin=7, end=7 dbg: ime.c:105: done: serial=3 dbg: ime.c:273: pre-edit cursor: begin=0, end=5 dbg: ime.c:49: preedit-string: text=ひらが, begin=9, end=9 dbg: ime.c:105: done: serial=3 dbg: ime.c:273: pre-edit cursor: begin=0, end=6 dbg: ime.c:49: preedit-string: text=ひらがn, begin=10, end=10 dbg: ime.c:105: done: serial=3 dbg: ime.c:273: pre-edit cursor: begin=0, end=7 dbg: ime.c:49: preedit-string: text=ひらがな, begin=12, end=12 dbg: ime.c:105: done: serial=3 dbg: ime.c:273: pre-edit cursor: begin=0, end=8 dbg: ime.c:66: commit: text=ひらがな ```
dnkl commented 1 year ago
Owner

Oh wait... I see now :/

Oh wait... I see now :/
dnkl commented 1 year ago
Owner

#318 should fix the pre-edit cursor issue, when positioned after the pre-edit string.

https://codeberg.org/dnkl/foot/pulls/318 should fix the pre-edit cursor issue, when positioned after the pre-edit string.
ngkz commented 1 year ago

It is a range while pre-editing, and in this case the range is the cursor_begin and cursor_end parameters from text_input_v3::preedit_string(). I.e. it is ultimately controlled by the IME engine.

Aha, The IME uses it for highlighting a word while selecting conversion candidates, right? I misunderstood what text-input-v3 protocol does. In GNOME on Wayland, It looks like Mutter or Fcitx doesn't send ranged preedit_string() due to a bug.

> It is a range while pre-editing, and in this case the range is the cursor_begin and cursor_end parameters from text_input_v3::preedit_string(). I.e. it is ultimately controlled by the IME engine. Aha, The IME uses it for highlighting a word while selecting conversion candidates, right? I misunderstood what text-input-v3 protocol does. In GNOME on Wayland, It looks like Mutter or Fcitx doesn't send ranged `preedit_string()` due to a bug.
tdeo marked this conversation as resolved
tdeo force-pushed text-input-rectangle from 6c6aee6e62 to dbba35227e 1 year ago
tdeo commented 1 year ago
Poster

I think I got everything mentioned so far.
@ngkz, can you test again, with #318 also applied?

I think I got everything mentioned so far. @ngkz, can you test again, with https://codeberg.org/dnkl/foot/pulls/318 also applied?
ngkz commented 1 year ago

I think I got everything mentioned so far.
@ngkz, can you test again, with #318 also applied?

It works 👍👍

EDIT: I couldn't test hollow cursor path due to a bug of Mutter or Fcitx5
EDIT 2: Sorry, I forgot to test a search input box, wait a moment.

> I think I got everything mentioned so far. > @ngkz, can you test again, with https://codeberg.org/dnkl/foot/pulls/318 also applied? ~~It works 👍👍~~ EDIT: I couldn't test hollow cursor path due to a bug of Mutter or Fcitx5 EDIT 2: Sorry, I forgot to test a search input box, wait a moment.
ngkz requested changes 1 year ago
ngkz left a comment

You forgot to update the cursor position at render.c:2453.

You forgot to update the cursor position at render.c:2453.
ngkz requested changes 1 year ago
ime.c Outdated
seat->ime.serial++;
memcpy(&seat->ime.cursor_rect.sent, &seat->ime.cursor_rect.pending,
sizeof(seat->ime.cursor_rect.sent));
ngkz commented 1 year ago

I suggest using structure assignment here. It is clearer and safer than memcpy.

I suggest using structure assignment here. It is clearer and safer than memcpy.
tdeo marked this conversation as resolved
ime.c Outdated
seat->ime.cursor_rect.pending.height);
memcpy(&seat->ime.cursor_rect.sent, &seat->ime.cursor_rect.pending,
sizeof(seat->ime.cursor_rect.sent));
ngkz commented 1 year ago

I suggest using structure assignment.

I suggest using structure assignment.
tdeo marked this conversation as resolved
wayland.h Outdated
int height;
} sent;
} cursor_rect;
ngkz commented 1 year ago

I suggest that define the named rectangle struct and declare pending and sent with it (to use structure assignment)

I suggest that define the named rectangle struct and declare `pending` and `sent` with it (to use structure assignment)
dnkl commented 1 year ago
Owner

+1

+1
tdeo marked this conversation as resolved
dnkl requested changes 1 year ago
dnkl left a comment
Owner

Just a couple of comments below.

Note that I haven't yet tested the code, but will as soon as possible. I'll add more comments if I encounter any issues.

Just a couple of comments below. Note that I haven't yet tested the code, but will as soon as possible. I'll add more comments if I encounter any issues.
ime.c Outdated
if (term->is_searching)
goto update;
dnkl commented 1 year ago
Owner

Can we add comments before the two gotos and describe the rectangle is set by render_xyz() in these two cases?

Can we add comments before the two `goto`s and describe the rectangle is set by `render_xyz()` in these two cases?
tdeo marked this conversation as resolved
ime.c Outdated
seat->ime.cursor_rect.pending.y = y / term->scale;
seat->ime.cursor_rect.pending.width = width / term->scale;
seat->ime.cursor_rect.pending.height = height / term->scale;
dnkl commented 1 year ago
Owner

Can we move the conversion to surface coordinates to the call to zwp_text_input_v3_set_cursor_rectangle(), and keep pending and sent in pixels?

Can we move the conversion to surface coordinates to the call to `zwp_text_input_v3_set_cursor_rectangle()`, and keep `pending` and `sent` in pixels?
tdeo marked this conversation as resolved
render.c Outdated
it->item.ime.cursor_rect.pending.width = 1 / term->scale;
it->item.ime.cursor_rect.pending.height = term->cell_height / term->scale;
}
}
dnkl commented 1 year ago
Owner

I think we should add a function to set the pending rectangle, taking the term pointer, and the x, y, width and height values.

I think we should add a function to set the `pending` rectangle, taking the `term` pointer, and the `x`, `y`, `width` and `height` values.
tdeo marked this conversation as resolved
dnkl commented 1 year ago
Owner

@tdeo I've merged a ton of PRs, several related to scrollback search. I strongly suggest you rebase this PR on latest master.

@tdeo I've merged a ton of PRs, several related to scrollback search. I strongly suggest you rebase this PR on latest master.
ngkz commented 1 year ago

You forgot to update the cursor position at render.c:2453.

I mean, it needs to update seat->ime.cursor_rect.pending at:

render.c:
...
                }
            } else if (!have_preedit)
#endif
            {
                /* Cursor *should* be in the visible area */
                xassert(cell_idx >= glyph_offset);
                xassert(cell_idx <= glyph_offset + visible_cells);
                draw_bar(term, buf->pix[0], font, &fg, x, y);
                ** HERE **
            }
        }

        if (next_cell_idx >= glyph_offset && next_cell_idx - glyph_offset > visible_cells) {
...
> You forgot to update the cursor position at render.c:2453. I mean, it needs to update `seat->ime.cursor_rect.pending` at: ```c render.c: ... } } else if (!have_preedit) #endif { /* Cursor *should* be in the visible area */ xassert(cell_idx >= glyph_offset); xassert(cell_idx <= glyph_offset + visible_cells); draw_bar(term, buf->pix[0], font, &fg, x, y); ** HERE ** } } if (next_cell_idx >= glyph_offset && next_cell_idx - glyph_offset > visible_cells) { ... ```
tdeo force-pushed text-input-rectangle from dbba35227e to ae1a931d37 1 year ago
ngkz reviewed 1 year ago
render.c Outdated
}
term_ime_set_cursor_rect(
term, x, y, (end * start) * term->cell_width, term->cell_height);
ngkz commented 1 year ago

end * start

typo

> end * start typo
tdeo marked this conversation as resolved
tdeo force-pushed text-input-rectangle from ae1a931d37 to 9b8ed57eb6 1 year ago
dnkl requested changes 1 year ago
dnkl left a comment
Owner

Getting close now I think :)

I've tested it with Sway+anthywl and couldn't find any functional issues - good job!

Getting close now I think :) I've tested it with Sway+anthywl and couldn't find any functional issues - good job!
ime.c Outdated
}
void ime_update_cursor_rect(struct seat *seat, struct terminal *term) {
// Set in render_ime_preedit()
dnkl commented 1 year ago
Owner

style:

-void ime_update_cursor_rect(struct seat *seat, struct terminal *term) {
+void
+ime_update_cursor_rect(struct seat *seat, struct terminal *term)
+{
style: ```diff -void ime_update_cursor_rect(struct seat *seat, struct terminal *term) { +void +ime_update_cursor_rect(struct seat *seat, struct terminal *term) +{ ```
tdeo marked this conversation as resolved
ime.c Outdated
seat->ime.cursor_rect.pending.x = x / term->scale;
seat->ime.cursor_rect.pending.y = y / term->scale;
seat->ime.cursor_rect.pending.width = width / term->scale;
seat->ime.cursor_rect.pending.height = height / term->scale;
dnkl commented 1 year ago
Owner

You shouldn't divide with the scaling factor here.

You shouldn't divide with the scaling factor here.
👍 1
tdeo marked this conversation as resolved
render.c Outdated
xassert(cell_idx >= glyph_offset);
xassert(cell_idx <= glyph_offset + visible_cells);
draw_bar(term, buf->pix[0], font, &fg, x, y);
term_ime_set_cursor_rect(term, margin + x, y, 1, term->cell_height);
dnkl commented 1 year ago
Owner

Shouldn't y here by term->height - margin - y - term->cell_height?

Speaking of which, I had trouble understanding why x and y differs in the draw_bar() and term_ime_set_cursor_rect() calls. Even though I wrote the original code, and knows the coordinates are sub-surface local and that the cursor rectangle is in main surface coordinates.

Perhaps we could wrap term_ime_set_cursor_rect() in a local macro in render_search_box() that translates from sub-surface to main-surface coordinates? Then the x/y parameters to draw_bar() and the wrapped term_ime_set_cursor_rect() would match.

Shouldn't `y` here by `term->height - margin - y - term->cell_height`? Speaking of which, I had trouble understanding why `x` and `y` differs in the `draw_bar()` and `term_ime_set_cursor_rect()` calls. Even though **I** wrote the original code, and **knows** the coordinates are sub-surface local and that the cursor rectangle is in main surface coordinates. Perhaps we could wrap `term_ime_set_cursor_rect()` in a local macro in `render_search_box()` that translates from sub-surface to main-surface coordinates? Then the x/y parameters to `draw_bar()` and the wrapped `term_ime_set_cursor_rect()` would match.
👍 1
tdeo commented 1 year ago
Poster

Added two local macros that convert from subsurfce coordinates to window coordinates instead, let me know if you prefer a single wrapper macro instead.

Added two local macros that convert from subsurfce coordinates to window coordinates instead, let me know if you prefer a single wrapper macro instead.
dnkl commented 1 year ago
Owner

Nah, that's fine. Better, actually :)

Nah, that's fine. Better, actually :)
dnkl marked this conversation as resolved
ngkz commented 1 year ago

It works perfectly for me with GNOME on Wayland + Fcitx 5 + fcitx5-mozc-ut + gnome-shell-extension-kimpanel if issues dnkl pointed out are fixed.

It works perfectly for me with GNOME on Wayland + Fcitx 5 + fcitx5-mozc-ut + gnome-shell-extension-kimpanel if issues dnkl pointed out are fixed.
tdeo force-pushed text-input-rectangle from 9b8ed57eb6 to f00493e8c4 1 year ago
dnkl reviewed 1 year ago
render.c Outdated
term_ime_set_cursor_rect(term,
margin + x, term->height - margin - y - term->cell_height,
1, term->cell_height);
}
dnkl commented 1 year ago
Owner

I think you missed this one when adding the WINDOW_{X,Y} macros?

I think you missed this one when adding the `WINDOW_{X,Y}` macros?
tdeo marked this conversation as resolved
ngkz reviewed 1 year ago
ime.c Outdated
seat->ime.cursor_rect.pending.x / term->scale,
seat->ime.cursor_rect.pending.y / term->scale,
seat->ime.cursor_rect.pending.width / term->scale,
seat->ime.cursor_rect.pending.height / term->scale);
ngkz commented 1 year ago

I noticed that x of the cursor rectangle becomes negative and the im popup is shown outside of the window when there is long preedit text and the caret is at the hidden part of the text.
I think x and y of the rectangle should be limited in the window.

I noticed that x of the cursor rectangle becomes negative and the im popup is shown outside of the window when there is long preedit text and the caret is at the hidden part of the text. I think x and y of the rectangle should be limited in the window.
tdeo commented 1 year ago
Poster

Yeah, I left it like that because GTK does the same thing.
Does mutter not hide it when it's out of bounds? I'd expect it to.

Yeah, I left it like that because GTK does the same thing. Does mutter not hide it when it's out of bounds? I'd expect it to.
ngkz commented 1 year ago

Yeah, I left it like that because GTK does the same thing.
Oh, VTE also set the cursor rectangle outside when the preedit is long.
I'm going to fix mutter instead.

Does mutter not hide it when it's out of bounds? I'd expect it to.
No, it does not.

> Yeah, I left it like that because GTK does the same thing. Oh, VTE also set the cursor rectangle outside when the preedit is long. I'm going to fix mutter instead. > Does mutter not hide it when it's out of bounds? I'd expect it to. No, it does not.
tdeo force-pushed text-input-rectangle from f00493e8c4 to 80601c993a 1 year ago
tdeo force-pushed text-input-rectangle from 80601c993a to 95c0c89cac 1 year ago
dnkl approved these changes 1 year ago
dnkl left a comment
Owner

LGTM!

@ngkz @tdeo I'll merge as soon as the discussion around bounding the coordinates to the window has been resolved.

LGTM! @ngkz @tdeo I'll merge as soon as the discussion around bounding the coordinates to the window has been resolved.
dnkl added the
bug
 label 1 year ago
ngkz approved these changes 1 year ago
dnkl merged commit 87d6739c03 into master manually 1 year ago
dnkl commented 1 year ago
Owner

@tdeo @ngkz thank you both, for all the time and effort!

Note: I've also decided to backport this to 1.6.3, to be released shortly.

@tdeo @ngkz thank you both, for all the time and effort! Note: I've also decided to backport this to 1.6.3, to be released shortly.
👍 1 🎉 2

Reviewers

dnkl approved these changes 1 year ago
ngkz approved these changes 1 year ago
The pull request has been manually merged as 87d6739c03.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
dnkl
ngkz
Labels
Apply labels
Clear labels
bug
Something is not working doc
Documentation duplicate
This issue or pull request already exists easy enhancement
New feature help wanted
Need some help invalid
Something is wrong not-a-bug performance question
More information is needed refactor regression upstream
Issue needs to be fixed upstream what do you think?
Looking for opinions wiki
Issue or patch for the wiki wontfix
This won't be fixed
No Label bug doc duplicate easy enhancement help wanted invalid not-a-bug performance question refactor regression upstream what do you think? wiki wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'tdeo/foot:text-input-rectangle'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes