Finish tinywl. #5
Conversation
* Get back pure ocaml type from Xdg_shell.Surface.role.
* Corresponds to wlr_xdg_surface_from_surface in xdg_shell.
Keycodes is not in Wlroots, but CTypes does not support abstract arrays. So we're using this as a wrapper with a void*.
This is to finish focus_view in tinywl.
Needed to do keyboard_notify_enter.
We need to mutate this value after it's held, so we need to make it into a ref.
Needed for move and resize events in tinywl.
Don't need to make the entire thing a ref. Just make the specific field we are concerned with mutable.
Need to handle the interactive events (moving or resizing a winodw). Need a new data type for the what the cursor is trying to do.
Might never be set to true. Do we need to handle this?
Edges are used in 32bit fashion. This could be changed since I do not know if they can be coerced safely to 64 bits.
* Moves option elimination outside of process_cursor* functions. * Changes grab to have an optional resize field, since resizing and moving are two disjoint states. * Changes Resize cursor_mode paramter to Edges.t
|
Wow, thanks a lot for this work, that looks great! I appreciate it especially given that it's useful work that is also quite tedious to do by oneself :). I'll certainly need to refresh my memory a bit as it's been a while since I haven't touched this project, but I'll definitely try to have a look soon. Did you hit any particular issues that you'd like to report, when building the project or extending the existing bindings? |
|
Hey, thanks! It was a really great learning experience. I got from 0 to 1 with the ctypes library. The only real problem came up about 2 weeks ago with the release of wlroots 0.13. A lot of the unstable features changed. |
|
Yeah, I noticed today that the repo doesn't build anymore with the latest git version of wlroots. While I figure how to do that, could you tell me which git commit of wlroots you used when working on the bindings? |
|
We were using the I personally try to avoid vendoring whenever possible. I am not sure how fast 0.13 is going to be adopted in distros, though. |
|
Thanks. I was thinking of vendoring only for developping the library, not for what would be a released version of the bindings. |
|
Fair enough. I am using nix/guix to manage the wlroots installation so it's possible to have multiple versions installed. Totally understand if that does not help your process, though. |
|
Ah, that's a very good point. Relying on nix or guix for development does sound simpler than figuring out vendoring. Would you mind sharing which setup you use for that? (if you have a nix/guix profile file it could be part of the repo) |
|
I used opam pretty extensively but I don't think it would be entirely necessary. I could setup a nix shell or guix environment if you wish. Right now my setup is a bit of a patchwork. |
|
Sorry for the delay, I just came back from holidays. I think if you could cook up a nix or guix environment that would be super useful (even more if it somehow works with opam, as that's what I know most). I tried to do that my self (with guix), but couldn't manage to make it work. |
|
I've been suggested another approach on #sway-devel: to build wlroots and install it locally (e.g. in |
|
I can certainly setup a nix/guix environment. It might take a week or so,
I'm quite busy right now
…On Thu, Jul 8, 2021 at 3:58 AM Armaël Guéneau ***@***.***> wrote:
I've been suggested another approach on #sway-devel: to build wlroots and
install it locally (e.g. in ~/.local), then tweak environment variables
to point to the local installation. I guess that's a form of poor-man's nix
environment, but if it works it would be convenient enough for me. I'll try
that tonight and report back.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/swaywm/wlroots-ocaml/pull/5*issuecomment-876341610__;Iw!!IKRxdwAv5BmarQ!M_CZEjO-9gbj1Pv9FiJ4jZ1mfNt4_EjVLU9Ot3BM65kXOSlUPe1ijsX7L3VNVQ$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ACMTD2N7ASVUCJD4IUKLRXTTWWAEVANCNFSM46T7KZLQ__;!!IKRxdwAv5BmarQ!M_CZEjO-9gbj1Pv9FiJ4jZ1mfNt4_EjVLU9Ot3BM65kXOSlUPe1ijsV-QEgmMw$>
.
|
|
Alright, don't worry too much then, it turns out the solution of installing wlroots in ~/.local works well for me; I could compile your branch and I'm currently doing the review :-). |
|
Ok thanks! There are still a few bugs with it but thank you!
… On Friday, Jul 09, 2021 at 1:23 PM, Armaël Guéneau ***@***.*** ***@***.***)> wrote:
Alright, don't worry too much then, it turns out the solution of installing wlroots in ~/.local works well for me; I could compile your branch and I'm currently doing the review :-).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (https://urldefense.com/v3/__https://github.com/swaywm/wlroots-ocaml/pull/5*issuecomment-877436533__;Iw!!IKRxdwAv5BmarQ!P2bGYfsMd8BqVtRDi7swmpCjCjJ2SqSGVs58PmwvndJGPy-ZaAtL-QFcMvX5yg$), or unsubscribe (https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ACMTD2KYOKA2F2X664WP56LTW5LDDANCNFSM46T7KZLQ__;!!IKRxdwAv5BmarQ!P2bGYfsMd8BqVtRDi7swmpCjCjJ2SqSGVs58PmwvndJGPy-ZaAtL-QE2Pdo2nQ$).
|
ffi/ffi.ml
Outdated
| let wlr_output_render_software_cursors = foreign "wlr_output_render_software_cursors" | ||
| (* FIXME: The void pointer is a pixman_region32_t for which no bindings exist (yet). | ||
| This is only ok because so far, no one uses it. *) | ||
| (wlr_output_p @-> ptr void @-> returning void) |
There was a problem hiding this comment.
I would instead add a let pixman_region32_p = ptr void somewhere earlier in the file and use that here (maybe with a comment saying that it represents a pointer to a pixman_region32_t).
ffi/ffi.ml
Outdated
| @@ -104,6 +104,10 @@ struct | |||
|
|
|||
| let wl_resource_p = ptr Wl_resource.t | |||
|
|
|||
| (* wl_output_transform *) | |||
|
|
|||
| let wl_output_transform = Wl_output_transform.t | |||
There was a problem hiding this comment.
I would drop the alias here (it's not adding anything) and directly use Wl_output_transform.t below.
|
|
||
| let signal_motion (cursor: t) : Pointer.Event_motion.t Wl.Signal.t = { | ||
| let signal_motion (cursor: t) : Event_pointer_motion.t Wl.Signal.t = { |
There was a problem hiding this comment.
Why move these to external modules? The naming convention I was trying to follow (and that also appears with Keyboard.Event_key for example) is that if there's a wlr_foo.h, then wlr_event_foo_bar gets mapped to Foo.Event_bar.
Did that naming scheme not work here? Otherwise I'd prefer to stick to it.
There was a problem hiding this comment.
I think we did this because we were struggling to learn the module system. We can definitely use a different convention.
I was kind of hoping to move events to submodules as a convention, like Pointer.Event.motion for instance.
There was a problem hiding this comment.
I think in any case each event type needs to have its own module (so it can include its compare/hash function for example), so it would have to be Pointer.Event.Motion.t.
I think having an Event submodule would be useful if we had functions that worked on all events, so that we could put them there (Pointer.Event.foo). However I IIRC don't think we have such functions currently, so for now I would just stick with the Pointer.Event_foo naming scheme.
lib/edges.ml
Outdated
| module Bindings = Wlroots_ffi_f.Ffi.Make (Generated_ffi) | ||
| module Types = Wlroots_ffi_f.Ffi.Types | ||
|
|
||
| type edges = None | Top | Bottom | Left | Right |
There was a problem hiding this comment.
I agree, but we wanted to stick to the names from wlroots. I think it is defined as wlr_edges in https://github.com/swaywm/wlroots/blob/475d9701e21e5047ee9d7e56909762c1eb961fcd/include/wlr/util/edges.h#L19:
enum wlr_edges {
WLR_EDGE_NONE = 0,
WLR_EDGE_TOP = 1 << 0,
WLR_EDGE_BOTTOM = 1 << 1,
WLR_EDGE_LEFT = 1 << 2,
WLR_EDGE_RIGHT = 1 << 3,
};There was a problem hiding this comment.
Ah, I see. I still think that in ocaml it would be more natural to have the type named edge. In C it makes sense because an element of wlr_edges is going to be used as a bit-field and thus indeed corresponds to a set of edges ; but this is not the case for an inhabitant of the ocaml sum type.
common/utils.ml
Outdated
| ) zero items | ||
| in | ||
| view ~read ~write uint32_t | ||
|
|
There was a problem hiding this comment.
When should we use bitwise_enum32 instead of bitwise_enum?
After a quick google search it looks like it's not terribly well specified which size enums should have; that they fit in a uint32_t is probably a good conservative assumption. However enum from ctypes assumes int64 values so this is probably fine as well?
In any case I would make a choice and either get rid of bitwise_enum or of bitwise_enum32.
There was a problem hiding this comment.
Ah yeah, this was because the edges field of the wlr_xdg_toplevel_resize_event is a uint32_t here https://github.com/swaywm/wlroots/blob/84dea55b20b60c229a5e31ccd37b58c96cba611a/include/wlr/types/wlr_xdg_shell.h#L224:
struct wlr_xdg_toplevel_resize_event {
struct wlr_xdg_surface *surface;
struct wlr_seat_client *seat;
uint32_t serial;
uint32_t edges;
};Which is used in begin_interactive in tinywl https://github.com/swaywm/wlroots/blob/475d9701e21e5047ee9d7e56909762c1eb961fcd/tinywl/tinywl.c#L723:
static void begin_interactive(struct tinywl_view *view,
enum tinywl_cursor_mode mode, uint32_t edges) {
...
double border_x = (view->x + geo_box.x) + ((edges & WLR_EDGE_RIGHT) ? geo_box.width : 0);
double border_y = (view->y + geo_box.y) + ((edges & WLR_EDGE_BOTTOM) ? geo_box.height : 0);
...
}
static void xdg_toplevel_request_move(
struct wl_listener *listener, void *data) {
/* This event is raised when a client would like to begin an interactive
* move, typically because the user clicked on their client-side
* decorations. Note that a more sophisticated compositor should check the
* provied serial against a list of button press serials sent to this
* client, to prevent the client from requesting this whenever they want. */
struct tinywl_view *view = wl_container_of(listener, view, request_move);
begin_interactive(view, TINYWL_CURSOR_MOVE, 0);
}
static void xdg_toplevel_request_resize(
struct wl_listener *listener, void *data) {
/* This event is raised when a client would like to begin an interactive
* resize, typically because the user clicked on their client-side
* decorations. Note that a more sophisticated compositor should check the
* provied serial against a list of button press serials sent to this
* client, to prevent the client from requesting this whenever they want. */
struct wlr_xdg_toplevel_resize_event *event = data;
struct tinywl_view *view = wl_container_of(listener, view, request_resize);
begin_interactive(view, TINYWL_CURSOR_RESIZE, event->edges);
}I wonder if this is an upstream issue.
There was a problem hiding this comment.
Another related question: Should bitwise_enum be defined in terms of int64 instead of uint64_t? (due to the underspecified nature of enum sizes)
There was a problem hiding this comment.
Thanks for the details. Then I agree that sticking to the C api and avoiding any integer casts is best. In that case it makes sense to have both bitwise_enum and bitwise_enum32.
There was a problem hiding this comment.
Would it be possible to use a functor to parameterize the type of uint size of the enum members?
There was a problem hiding this comment.
I just pushed a WIP commit with a functor for bitwise enums. It does change uint64 to int64 where the unsigned version was used.
| (* Worth it? *) | ||
| if Bindings.wlr_surface_is_xdg_surface surface | ||
| then | ||
| (* assert is called so this might blow up *) |
There was a problem hiding this comment.
I'm not sure I understand the comment there?
| let surface = getfield Types.Xdg_surface.surface | ||
| let toplevel = getfield Types.Xdg_surface.toplevel | ||
|
|
||
| let from_surface (surface : Surface.t) : t option = |
There was a problem hiding this comment.
Should this function be called from_wlr_surface for consistency?
| let seat = getfield Types.Xdg_toplevel_resize_event.seat | ||
| let serial = getfield Types.Xdg_toplevel_resize_event.serial | ||
| let edges ev = coerce uint32_t Edges.t | ||
| (getfield Types.Xdg_toplevel_resize_event.edges ev) |
There was a problem hiding this comment.
instead of coerce, shouldn't this be using bitwise_enum?
types/types.ml
Outdated
| @@ -144,12 +190,47 @@ module Make (S : Cstubs_structs.TYPE) = struct | |||
| let state = field t "state" Key_state.t | |||
| end | |||
|
|
|||
| module Keyboard_modifier = struct | |||
| type modifier = | |||
types/types.ml
Outdated
| let _WLR_MODIFIER_LOGO = constant "WLR_MODIFIER_LOGO" int64_t | ||
| let _WLR_MODIFIER_MOD5 = constant "WLR_MODIFIER_MOD5" int64_t | ||
|
|
||
| let modifier : modifier typ = |
Note that pixman_region32_t is stubbed as a void pointer since no bindings to pixman exist yet.
Just use Wl_output_transform.t, as wl_output_transform was just an alias.
|
Thanks for the detailed review! I'll be going over it this week. |
Hello,
@LambdaCalrissian and me have been working on finishing the tinywl implementation. We have something close to a working version here with a few bugs to fix.
We completed the rest of the
failwiths in the file and completed the ffi required to accomplish it.There are certainly things here that might need discussion and, as mentioned it still has bugs.
Thanks for the great start!