-
Notifications
You must be signed in to change notification settings - Fork 343
Conversation
Questions about the protocol (@dcz-purism?):
|
Thanks for the contribution! Is there a client to test this with?
What would be the benefit? Also, there's no defined concept of "roles", so one would have to be introduced. EDIT: now I think I see what you mean: preedit surface is wrapping a wl_surface. That should probably be an error indeed.
Thanks for noticing this. I think my assumption was "if it doesn't report surrounding text, it doesn't support preedit", but those are somewhat independent, especially when the input method can spawn a popup. Perhaps that could be a bit field on the
It was easier to design the protocol this way. It's up to the compositor.
This might be a good idea. I didn't try to tackle this problem because I have little experience with using popups. |
I've been implementing this for https://github.com/tadeokondrak/anthywl.
Also, having to put a buffer on the screen to get the text input rectangle at all seems like it might lead to flickering where the first frame is drawn without any knowledge of it, then the rest are correctly drawn. |
Those are all good points. When discussing the new version of input-method (currently not progressing due to lack of reviewers and me being focused on other things), the conclusion was not to implement the popup due to unforeseen bugs. Turns out you're finding them as you implement it. As far as I know, no one else implements popups, so if you want to fix the protocol by introducing a new version, there would be no one to hold you up. |
Side note: I wonder if we should add input-method-v2 to wlr-protocols, because it's not hosted anywhere ATM. |
c957ca4
to
8960e09
Compare
4f78d2d
to
49583eb
Compare
While implementing text-input's However, input-method describes it differently: <event name="text_input_rectangle">
<description summary="set text input area position">
Notify about the position of the area of the text input expressed as a
rectangle in surface local coordinates.
This is a hint to the input method telling it the relative position of
the text being entered.
</description>
<arg name="x" type="int"/>
<arg name="y" type="int"/>
<arg name="width" type="int"/>
<arg name="height" type="int"/>
</event> Is this intentional? I think multiple popups add considerable implementation complexity while having no guaranteed behaviour and no use case as far as I can tell. I think it's worth disallowing them. I think the I tried to come up with a protocol update making it possible: <!-- in zwp_input_popup_surface_v2 -->
<enum name="anchor" bitfield="true" since="2">
<entry name="top" value="1"/>
<entry name="bottom" value="2"/>
<entry name="left" value="4"/>
<entry name="right" value="8"/>
</enum>
<event name="configure" since="2">
<arg name="cursor_width" type="uint" summary="width of the cursor"/>
<arg name="cursor_height" type="uint" summary="height of the cursor"/>
<arg name="up_available" type="uint"
summary="space available above of the cursor"/>
<arg name="down_available" type="uint"
summary="space available below of the cursor"/>
<arg name="left_available" type="uint"
summary="space available to the left of the cursor"/>
<arg name="right_available" type="uint"
summary="space available to the right of the cursor"/>
<arg name="serial" type="uint" summary="serial of the configure event"/>
</event>
<event name="ack_configure" since="2">
<arg name="anchor" type="uint" enum="anchor"/>
<arg name="margin_x" type="int"
summary="horizontal distance from the anchor"/>
<arg name="margin_y" type="int"
summary="vertical distance from the anchor"/>
<arg name="serial" type="uint"
summary="the serial from the configure event"/>
</event> This also makes it possible for IMEs to receive the cursor rectangle before actually rendering anything, preventing flicker if the IME decides to do anything with it.
A typical case without any shortage of space where the IME wants 50px of margin before the text: If the window is approaching the right edge of the monitor: If the window is approaching the bottom edge of the monitor: Feedback on this would be appreciated. |
At least to some extent, as in I think I made it vague because I didn't have an opinion other than "allows the compositor to choose a placement of the pop up such as what's being edited is not obscured". I think it's reasonable at least to cover the entire preedit text within the rectangle. I actually never tested what GTK returns here, but I know the rectangle is being set.
If you decide to add such a reuirement to a newer revision of the protocol, I will not have any arguments to oppose it ;) How much positioning of the pop up is needed? The Wayland assumption is that surfaces don't know how they are positioned in terms of absolute coordinates, and I tend to agree with that. Also, I think while pop ups are less useful without control over positioning, they don't become useless at all. That'd be good as a first step. I do like the idea with the anchor, although I would rather have the client provide the preferences together with the surface, and let the compositor figure out everything. With this proposal, the application is allowed to set constrains which are impossible to meet. Then the request turns into a suggestion, so the compositor must have positioning logic too. If the compositor must have positioning logic, we might as well feed it the general rule and let it handle the entirety of positioning. |
Thanks for the reply!
From what I can tell the correct behaviour is:
Knowing what the rectangle actually means is important if the IME wants to do anything with it, so I think the protocol should clarify this.
Didn't realize it when I was writing my last comment, but they do make sense if positioning is added. I'll add back support to the PRs.
Yeah, I agree with the Wayland avoidance of absolute coordinates, but I don't think my anchor protocol idea is in conflict with that. More on it below. My protocol idea allows the surface to have an arrow pointing to the cursor area, like the last screenshot in my last comment, without the arrow ever becoming out of place. I don't see any reason you'd need more positioning, but of course less positioning is enough at the expense of aesthetics.
Agreed, I think positioning can be part of another PR.
I don't think there are any constraints that are impossible to meet. The popup trailing off the screen is completely okay, it's just something that should be avoided. And if the compositor doesn't have the concept of output borders, it can just provide UINT_MAX to all the "available" parameters. The client providing the preferences itself seems difficult because for the compositor to know how to position it, it needs to know its width, height, and "hotspot" (where the arrow would be pointing to, for example). But if the compositor decides to position it above instead of below for example, the arrow would switch to the other side, requiring back-and-forth with the client. Giving the client available space lets it know where the "hotspot" would be before actually needing to draw anything.
I realized that popups shouldn't actually be repositioned to fit on the screen, normal xdg_popups don't do this either. The popup would be repositioned the next time it would normally be updated. |
Do you mean "observed" behaviour? Or is that what you think would be optimal?
What could an IME want to do with it? I can see the fancy arrow-thing pointing to the cursor, but it doesn't need to be so accurate. To me in the beginning it was about giving the compositor enough info to make sure the text is not obscured. What are the other potential use cases?
Ah, that's where we had different plans. The popup trailing off the screen was something I intended to never happen as long as the compositor can help it. This design gives the application the ability to push the pop up off the visible area. To support my view of things, the xdg pop up which appears on right-click never appears off the screen, because (I presume, didn't actually check) the compositor takes control of its exact position.
This is a good point. There is a way for Gtk to display such pop ups, I think it uses subsurfaces. They also display a fancy arrow pointing approximately to the click site. Might be wirth it to work out how exactly this happens. That would make this interface more aligned with what's already established, and help choose between our competing views. |
A bit of both, I needed to find out what behaviour is best when actually implementing it, there's some discussion here of me narrowing it down. It's important for text-input clients to send it correctly becuase GNOME actually uses the arrow pointing to the cursor on its overlay.
Well, why is the text_input_rectangle event in input-method if the IME doesn't have any use for it? A basic use I can think of is knowing whether or not the text is above or below the popup, to position extra information farther away from the cursor rather than moving the main content.
Sure, I agree. By "completely okay" I mean that if the IME willingly positions the popup so part of it is off screen, what's the problem with it? There's no reason it would want to do that.
Well, if there is no space on the screen for the popup to fit in any direction, it goes off screen. [I hit submit before I finished my post. Last part in a separate comment to follow] |
I ask this question to arrive at the level of specificity we actually care about. So far the most specific we have are examples of caring about the general area, and that's easy to satisfy while giving the implementors freedom to do what's easiest to them.
Okay, but no one wants the right click menu off the screen either. Yet the protocol designers decided to take this ability away anyway. What was their reasoning? Does it apply here? If we can't answer that, then do we have good reasons to believe that we aren't missing anything important when we make a different decision? |
Ah, I see what you mean. During normal Japanese IME input, you have two phases.
Yeah, looking at the xdg_positioner protocol does give me more ideas for how to design the input-method part. |
Maybe xdg-popup can be re-used for IMEs? The layer-shell is an example of a protocol that re-uses xdg-popups. |
I thought of that, but I saw some issues with it. Looking closer, they can all probably be defined:
Is changing the behaviour of a protocol when used through another like that okay? |
49583eb
to
28ddb20
Compare
An issue in the protocol that has nothing to do with positioning: If you're on one window typing with a popup open, then switch to another window, then the popup may stay on screen for however long it takes for the IME to realize it's been deactivated and reactivated. |
The compositor should probably close the popup when switching away? |
There isn't a way for the compositor to close an input-method popup, it's always visible when the input method is active:
|
Thsi doesn't look like a good design decision to me. There are many reasons why the compositor would want to close a popup: switching focus, opening a lockscreen, switching input methods... Why not use the |
I'm referring to the existing input-method-v2 popups rather than xdg-popup right now, it doesn't have a
These are done by deactivating the input method. |
Ah, I see. The compositor shouldn't rely on the client to withdraw the popup. |
It doesn't, but the situation is switching focus from one text-input client to another text-input client, directly from input box to input box. The input method has a single popup shared across all windows. |
28ddb20
to
8cc1465
Compare
8cc1465
to
f8ee901
Compare
f8ee901
to
1983cb0
Compare
Are all of the popup-related protocol issues documented in https://gitlab.freedesktop.org/wayland/wayland-protocols/-/issues/40? Do we need protocol changes before this can be reviewed? |
1983cb0
to
d096d1e
Compare
I think so.
No, this should still be useful as-is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Here's a first round of comments.
I see the popup interface is pretty bare-bones right now, it sounds like there can be a number of glitches because a de-synchronized state. We probably want to improve that with the next protocol version.
wl_signal_init(&popup_surface->events.unmap); | ||
wl_signal_init(&popup_surface->events.destroy); | ||
|
||
popup_surface_set_mapped(popup_surface, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow already-mapped surfaces? I guess the protocol allows them, and disallowing them would be a breaking change. But before getting the text input box, the client can't really know how to draw the popup.
struct wlr_input_popup_surface_v2 *popup_surface, | ||
struct wlr_box *sbox) { | ||
zwp_input_popup_surface_v2_send_text_input_rectangle( | ||
popup_surface->resource, sbox->x, sbox->y, sbox->width, sbox->height); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protocol issue: the compositor has no way to know when the client has taken this new information into account. Thus the compositor might draw an old buffer at a new position, resulting in a glitch.
These are issues with the protocol, but even with a flawed protocol, this feature is useful. |
Yes, these are more of drive-by comments, I don't expect these to be fixed in this PR. :) |
Code looks pretty good to me. |
Pushed to the wrong branch maybe? |
872314b
to
1dd2f95
Compare
Whoops, was testing the commits together and accidentally pushed. Didn't notice |
1dd2f95
to
a5e7811
Compare
How do we feel about this PR? Do we need a patched |
a5e7811
to
1a8c3a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said before, code LGTM! Let's merge this even if the Sway PR isn't ready yet.
Thanks for your contribution!
I'm looking forward to documenting this whole setup. |
No description provided.