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.

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

Jump to bottom

text_input: Implement input-method popups #7226

Merged
merged 22 commits into from
Feb 20, 2024
Merged

text_input: Implement input-method popups #7226

merged 22 commits into from
Feb 20, 2024
+253 −1

Conversation

Decodetalkers
Copy link
Contributor

@Decodetalkers Decodetalkers commented Oct 17, 2022 •
edited
Loading

May I continue this pr in #5890 ?

@layercak3 layercak3 mentioned this pull request Oct 18, 2022
Closed
@q234rty
Copy link

q234rty commented Oct 27, 2022 •
edited
Loading

I'm not sure but I don't think this currently works at all? i.e. the original PR needs changes after https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3660 .

@Decodetalkers
Copy link
Contributor Author

I'm not sure but I don't think this currently works at all? i.e. the original PR needs changes after https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3660 .

image

Emm, It seems still works with the wlroots master and sway master, and I also make changes with the suggestion of the maintainer..

@q234rty
Copy link

q234rty commented Oct 28, 2022

@Decodetalkers Does the input popup correctly disappear after you commit some text?

@Decodetalkers
Copy link
Contributor Author

@Decodetalkers Does the input popup correctly disappear after you commit some text?

I cannot understand, what kind of "uncorrect", can you show me a video ? I cannot reproduce anything..

@q234rty
Copy link

q234rty commented Oct 28, 2022

mov-2022-10-28--17-05-11.mp4

@Decodetalkers
Copy link
Contributor Author

mov-2022-10-28--17-05-11.mp4

I see, I reproduced it on foot.. but wezterm do not have such problem.. Interesting..

@Decodetalkers
Copy link
Contributor Author

mov-2022-10-28--17-05-11.mp4

Now it should be fixed

@Decodetalkers Decodetalkers force-pushed the input-method-popups branch 4 times, most recently from bf22658 to 4bd7fbc Compare November 1, 2022 10:18
@tadeokondrak tadeokondrak mentioned this pull request Nov 23, 2022
Closed
@Decodetalkers Decodetalkers force-pushed the input-method-popups branch 4 times, most recently from cba433e to e2a9a60 Compare November 25, 2022 07:20
@Riey Riey mentioned this pull request Nov 25, 2022
Open
5 tasks
@wengxt wengxt mentioned this pull request Dec 15, 2022
Closed
@someoneinjd someoneinjd mentioned this pull request Dec 25, 2022
Closed
32 tasks
@q234rty q234rty mentioned this pull request Jan 10, 2023
Closed
@amano-kenji
Copy link

Does this show horizontal pop-ups or vertical pop-ups?

@q234rty
Copy link

q234rty commented Jan 30, 2023

Does this show horizontal pop-ups or vertical pop-ups?

@amano-kenji This depends on your IME. The popup is rendered by the IME.

@amano-kenji
Copy link

Is it going to be merged soon? It sems Hyprland already merged this feature.

emersion
emersion reviewed Feb 17, 2024
View reviewed changes
emersion
emersion reviewed Feb 17, 2024
View reviewed changes
@cherrot

This comment was marked as off-topic.

Sign in to view
@bl4ckb0ne
Copy link
Contributor

@cherrot open a new issue for this

emersion
emersion reviewed Feb 19, 2024
View reviewed changes
@tokyo4j
Copy link

tokyo4j commented Feb 19, 2024 •
edited
Loading

I found the position of popups is shifted by the offset requested with xdg_surface.set_window_geometry in Firefox. So I guess the position should be subtracted by that offset.

20240206_00h26m13s_grim

Maybe my comment here is missed.
Same problem happens when I run GTK_IM_MODULE=wayland gedit (I set GTK_IM_MODULE to ensure GTK doesn't use dbus interface).

I found this problem is not present in sway-im AUR where scene-graph API is not adopted yet.
I'm not familiar with sway codebase, but I guess this is because the position of surface node can be different from that of the view node.
For reference, wlroots sets the offset of surface node here.

@Decodetalkers
Copy link
Contributor Author

I found the position of popups is shifted by the offset requested with xdg_surface.set_window_geometry in Firefox. So I guess the position should be subtracted by that offset.
20240206_00h26m13s_grim

Maybe my comment here is missed. Same problem happens when I run GTK_IM_MODULE=wayland gedit (I set GTK_IM_MODULE to ensure GTK doesn't use dbus interface).

I found this problem is not present in sway-im AUR where scene-graph API is not adopted yet. I'm not familiar with sway codebase, but I guess this is because the position of surface node can be different from that of the view node. For reference, wlroots sets the offset of surface node here.

ok, thanks

emersion
emersion reviewed Feb 20, 2024
View reviewed changes
emersion
emersion reviewed Feb 20, 2024
View reviewed changes
emersion
emersion reviewed Feb 20, 2024
View reviewed changes
sway/input/text_input.c
struct sway_input_popup *popup;
wl_list_for_each(popup, &relay->input_popups, link) {
// send_text_input_rectangle is called in this function
input_popup_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.

input_popup_update() is a pretty big hammer, it means we'll create/destroy scene nodes every time a keypress is made. But let's keep this as-is for now and leave the optimization for later.

emersion
emersion reviewed Feb 20, 2024
View reviewed changes
@emersion
Copy link
Member

Alright, I think this is the last round of comments from me!

@Decodetalkers
chore: tidy up code
34fc0d0 
remove useless line, and code style adjust
@Decodetalkers
Copy link
Contributor Author

Alright, I think this is the last round of comments from me!

Thanks, I have finished it

emersion
emersion approved these changes Feb 20, 2024
View reviewed changes
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Alright, let's go with this then, thanks a lot for your hard work! I think many users will be thankful :P

@emersion emersion merged commit 7c11c46 into swaywm:master Feb 20, 2024
3 checks passed
Guanran928 added a commit to Guanran928/flake that referenced this pull request Feb 24, 2024
@Guanran928
sway: use latest git version
66d7539 
swaywm/sway#7226 is merged
swaywm/sway#6249 looks dead :(

also simplified ./overlays
@tokyo4j tokyo4j mentioned this pull request Mar 10, 2024
Merged
@sam-mccall sam-mccall mentioned this pull request Mar 23, 2024
Closed
@wiwyil2tr wiwyil2tr mentioned this pull request Apr 28, 2024
Closed
@mochaaP mochaaP mentioned this pull request May 7, 2024
Merged
@eternal-sorrow
Copy link

So, will there be a release anytime soon that includes this? Because the patch was reworked on top of the new rendering system and the new rendering system is not included in the 1.9 release, there is no way currently to have popups on sway 1.9. So I'm stuck with sway 1.8.1 and an old version of this patch.

@kennylevinsen
Copy link
Member

Non-bugfix sway releases follow wlroots releases.

If you need features newer than the latest release, you can always build sway and wlroots master. There's even recipes and packages for some distros by community members (AUR, Copr, debian/ubuntu repos, etc.).

@eternal-sorrow
Copy link

So no? Okay then...

@GalaxySnail
Copy link

There is a patch for sway 1.9 on AUR: https://aur.archlinux.org/packages/sway-im, extracted from commit 0789c12a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yutsuten Yutsuten Yutsuten left review comments

@hikyae hikyae hikyae left review comments

@emersion emersion emersion approved these changes

@bl4ckb0ne bl4ckb0ne Awaiting requested review from bl4ckb0ne

@Nefsen402 Nefsen402 Awaiting requested review from Nefsen402

Assignees
No one assigned
Labels
None yet
Milestone
1.10
Development

Successfully merging this pull request may close these issues.