Skip to content
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.

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

Implement input_method_v2 popups #5890

Closed
wants to merge 1 commit into from

Conversation

tadeokondrak
Copy link
Contributor

@tadeokondrak tadeokondrak commented Dec 16, 2020

@tadeokondrak tadeokondrak force-pushed the input-method-popups branch 3 times, most recently from 95af575 to e399a4c Compare December 16, 2020 20:09
@tadeokondrak tadeokondrak force-pushed the input-method-popups branch 2 times, most recently from b94a50b to 7478170 Compare January 2, 2021 19:24
sway/desktop/render.c Outdated Show resolved Hide resolved
@Riey Riey mentioned this pull request Jan 22, 2021
2 tasks
@tadeokondrak tadeokondrak force-pushed the input-method-popups branch 4 times, most recently from 36357ee to ce391ee Compare January 23, 2021 22:35
@12101111
Copy link

12101111 commented Jan 25, 2021

sway crash on exit

  * frame #0: 0x00007fcc2418f6a7 libwayland-server.so.0`wl_list_remove(elm=0x00007fcc1bd2dd98) at wayland-util.c:55:18
    frame #1: 0x00007fcc2413a06a libwlroots.so.7`xdg_popup_grab_handle_seat_destroy(listener=<unavailable>, data=<unavailable>) at wlr_xdg_popup.c:158:2
    frame #2: 0x00007fcc2416634c libwlroots.so.7`wlr_signal_emit_safe(signal=<unavailable>, data=0x00007fcc1c04e050) at signal.c:29:3
    frame #3: 0x00007fcc24134e88 libwlroots.so.7`wlr_seat_destroy(seat=0x00007fcc1c04e050) at wlr_seat.c:170:2
    frame #4: 0x00007fcc2418975f libwayland-server.so.0`wl_display_destroy [inlined] wl_priv_signal_final_emit(signal=0x00007fcc245fce88, data=0x00007fcc245fce20) at wayland-server.c:2157:3
    frame #5: 0x00007fcc24189728 libwayland-server.so.0`wl_display_destroy(display=0x00007fcc245fce20) at wayland-server.c:1134
    frame #6: 0x00005589ae2246a3 sway`server_fini(server=0x00005589ae2825d0) at server.c:203:2
    frame #7: 0x00005589ae223dee sway`main(argc=1, argv=0x00007ffd55a5e338) at main.c:436:2
    frame #8: 0x00007fcc247c0129 ld-musl-x86_64.so.1`libc_start_main_stage2(main=(sway`main at main.c:245), argc=<unavailable>, argv=0x00007ffd55a5e338) at __libc_start_main.c:94:7
    frame #9: 0x00005589ae216eb6 sway`_start + 22

another crash

* thread #1, name = 'sway', stop reason = signal SIGSEGV
  * frame #0: 0x000055bfceadf096 sway`desktop_damage_view at desktop.c:0:2
    frame #1: 0x000055bfceadf05e sway`desktop_damage_view(view=0x00007f1b143dd2d0) at desktop.c:38
    frame #2: 0x000055bfceafd400 sway`input_popup_update [inlined] input_popup_damage(popup=<unavailable>) at text_input.c:36:2
    frame #3: 0x000055bfceafd3e8 sway`input_popup_update(popup=0x00007f1b13c4fac0) at text_input.c:40
    frame #4: 0x00007f1b1fec5f8c libwlroots.so.7`wlr_signal_emit_safe(signal=<unavailable>, data=0x00007f1b1355a790) at signal.c:29:3
    frame #5: 0x000055bfceafcb29 sway`handle_text_input_enable(listener=<unavailable>, data=<unavailable>) at text_input.c:260:2
    frame #6: 0x00007f1b1fec5f8c libwlroots.so.7`wlr_signal_emit_safe(signal=<unavailable>, data=0x00007f1b1439db10) at signal.c:29:3
    frame #7: 0x00007f1b1f9016b5 libffi.so.7`___lldb_unnamed_symbol3$$libffi.so.7 + 85
    frame #8: 0x00007f1b1f905c6e libffi.so.7`___lldb_unnamed_symbol19$$libffi.so.7 + 750
    frame #9: 0x00007f1b1feee664 libwayland-server.so.0`wl_closure_invoke(closure=0x00007f1b144155c0, flags=<unavailable>, target=<unavailable>, opcode=7, data=<unavailable>) at connection.c:1018:2
    frame #10: 0x00007f1b1fee8958 libwayland-server.so.0`wl_client_connection_data(fd=<unavailable>, mask=<unavailable>, data=<unavailable>) at wayland-server.c:432:4
    frame #11: 0x00007f1b1feecd3c libwayland-server.so.0`wl_event_loop_dispatch(loop=<unavailable>, timeout=<unavailable>) at event-loop.c:1027:4
    frame #12: 0x00007f1b1fee9bad libwayland-server.so.0`wl_display_run(display=0x00007f1b20324c20) at wayland-server.c:1351:3
    frame #13: 0x000055bfceaddcf1 sway`main [inlined] server_run(server=<unavailable>) at server.c:247:2
    frame #14: 0x000055bfceaddcbf sway`main(argc=1, argv=0x00007ffe7aa881c8) at main.c:431
    frame #15: 0x00007f1b20520129 ld-musl-x86_64.so.1`libc_start_main_stage2(main=(sway`main at main.c:245), argc=<unavailable>, argv=0x00007ffe7aa881c8) at __libc_start_main.c:94:7
    frame #16: 0x000055bfcead0e16 sway`_start + 22

@emersion
Copy link
Member

#4932 has been merged now, so this needs a rebase.

chuangzhu added a commit to chuangzhu/nixpkgs that referenced this pull request Feb 25, 2022
Foot and alacritty no longer crashes.
@JingMatrix
Copy link
Contributor

I apply this PR with the current HEAD.
The api of wlr_output_layout_get_box has changed, so you should change your code a bit in sway/sway/input/text_input.c.
After this, I compiled your code, and tested it with alacritty and kitty, both gave me the same result:
the popup showed up at the very beginning, and then never showed up again.

@dragove
Copy link

dragove commented Apr 5, 2022

I've tested it with AUR package sway-im with fcitx5 as input method in kitty. When the character meets the end of a line, the popup will be hidden. I don't know if it is a issue with kitty. Alacritty won't meet this issue since it won't put characters into the line before I select my needed word in IM.
Edit: This problem only shows up when I set default_border none 0 in sway configuration file.

@JingMatrix
Copy link
Contributor

Now I test it ( after fixing the output_box api problem) with the HEAD of fcitx5 and HEAD of sway, everything works!

@Legogris
Copy link

@emersion Looks like this is ready for another review :)

@@ -395,6 +395,8 @@ static bool surface_is_popup(struct wlr_surface *surface) {
wlr_subsurface_from_wlr_surface(surface);
surface = subsurface->parent;
}
if (wlr_surface_is_input_popup_surface_v2(surface))
Copy link
Member

Choose a reason for hiding this comment

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

Style: braces are mandatory

@@ -139,6 +161,11 @@ struct sway_node *node_at_coords(
return NULL;
}

if ((*surface = input_popup_surface_at(output,
Copy link
Member

Choose a reason for hiding this comment

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

We should probably move this up right before "check for unmanaged views" to allow fullscreen surfaces to work with IMEs.

continue;
}
double _sx = ox - lx;
double _sy = oy - ly;
Copy link
Member

Choose a reason for hiding this comment

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

This mixes up layout-local coords and output-local coords. I think input_popup_surface_at should take layout-local coords instead.

wl_signal_add(&view->events.unmap, &popup->focused_surface_unmap);

// Since the focus has changed, the popup may have to adjust
update_popup:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a goto, else if/else would be more readable.


struct sway_view *view = view_from_wlr_surface(
text_input->input->focused_surface);
if (view->container == NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

This can return NULL here.

This logic doesn't work on subsurfaces nor xdg_popups.

@evanslify
Copy link

@tadeokondrak @emersion This PR seems to have some amendments pending. Can I take over by starting another PR?

sevz17 added a commit to sevz17/dwl that referenced this pull request Sep 30, 2022
Based on swaywm/sway#5890

Co-authored-by: Leonardo Hernández Hernández <leohdz172@protonmail.com>
@emersion emersion removed this from the 1.8 milestone Nov 11, 2022
@flibitijibibo
Copy link

Someone has been testing this with SDL and it seems there's an issue with fcitx, but only when integer scaling is used...?

libsdl-org/SDL#5892

We have some ideas on our end, but figured we'd include the issue here in case it's something obvious on the compositor side of things.

@tadeokondrak
Copy link
Contributor Author

Sorry about this PR... I've left it open because I thought I could get to it eventually - but it looks like someone else is continuing the effort at #7226, which is great.

@Decodetalkers
Copy link
Contributor

Sorry about this PR... I've left it open because I thought I could get to it eventually - but it looks like someone else is continuing the effort at #7226, which is great.

Thanks that you have return back..Your help is still needed..Do you still have time now?

@Decodetalkers
Copy link
Contributor

Sorry about this PR... I've left it open because I thought I could get to it eventually - but it looks like someone else is continuing the effort at #7226, which is great.

This mixes up layout-local coords and output-local coords. I think input_popup_surface_at should take layout-local coords instead.

I do not know how to make change about this request, can you help me?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or incremental improvement
Development

Successfully merging this pull request may close these issues.