Awesome wm/awesome and class fixups drawin impl by Timidger · Pull Request #456 · way-cooler/way-cooler

Timidger · 2017-12-10T20:33:44Z

This has some crucial changes needed for AwesomeWM compatibility. There are also some additions for drawin, drawable, and others etc. but they are incomplete. I'm merging them anyways so I don't have to duplicate the effort later but those APIs should still be considered incomplete.

With these changes the rc.lua no longer properly runs (it fails trying to access a layoutbox property). I will attempt to fix that however because I want the master rc.lua always to be usable for testing purposes.

SethBarberee · 2017-12-27T05:19:51Z

                     title = "Oops, there were errors during startup!",
                     text = awesome.startup_errors })
 end
+naughty.notify({ preset = naughty.config.presets.critical,


Did you just duplicate this by accident??

I put this in here to test notifications. I was hoping to get them working when I first started on this PR (notice a lot of the changes relate to adding cairo and xcb stuff to awesome bindings). Alas, no such luck. I'll remove this as this is a debug thing I forgot to remove.

I'm going to review this again properly and merge it in soon (tm). There's some changes here that are fairly critical (fix a lot of issues with Awesome classes that will be needed down the line. Plus it shows how to connect and use xcb)

That would be great! I was going to base the wallpaper work off of this branch anyway!

I'll work on cleaning this up and merging it in today.

Ok "today" was a bit optimistic. Going to try to finish this up ASAP, but you should still base your work off this branch.

We weren't wrapping it in a table, like is expected of in Lua because Awesome does it. HOWEVER, they use lua_setuservalue. We don't have access to that, so we just put it in a table. Need to double check this does the right thing...because this doesn't seem right :(

I really need to implement these properly

By movinging it before hand, xcb can talk to xwayland and get e.g xkb information.

Also adds more safety checks for unsafely getting the group names

Even in the error case we ensure that the connection isn't closed.

Because lua doesn't have the screen, which it gets from the client or from the mouse. So I need to implement it for mouse because that's the easiest way. yay.

The way it's done is crazy, but basically mouse has a reference to the screen table and then if screen global gets a screen instance we just return it. Again, it's really crazy

I was saving it on the instance of the class instead of the class itself, which makes sense in retrospect

You should set it on the class instead

Took way too long because the C code is inscrutable

Had to update dummy-rustwlc -_-

This is to get around issues with testing...also it's technically correct, as we don't have hard dependency on xwayland _quite_ yet

Timidger · 2017-12-29T02:11:44Z

Currently we are doing the objects allocation wrong here. We are allocating the tables weirdly, and we need to be 1-1 on what Awesome does. Once that is setup properly, writing the APIs will be easy enough that it can be done in parallel.

cc @psychon We talked a bit about this on the gitter channel, but I know it's hard to read this Rust when you're not familiar with it. Any quick run-down on how the objects (e.g like a client) should look on the Lua side? Right now this current PR seems to allocate them fine since we are running it against the default rc.lua and it's accessing the objects fine but it's still almost certainly not correct.

psychon · 2017-12-29T07:13:24Z

Any quick run-down on how the objects (e.g like a client) should look on the Lua side?

Well... it's a userdata with some object-specific fields, some of them read-only. The general "stuff" is:

the signal API (add_signal, connect_signal, disconnect_signal, emit_signal),
a valid field that is always true and becomes false after the object's __gc metamethod runs (implemented by setting a different metatable, all other indices cause an error to be thrown),
a data table that the C code saves but does not otherwise care about; this is used to e.g. save the floating state of a client and was needed so that Lua's garbage collector knows that this table is only kept alive by the client and can be garbage collected together with it (previously some magic with weak tables were used and if you did awful.client.property.set(c, "foo", c) this magic would break down and the client object would be un-garbage-collectable).

Each object has a class, e.g. a client belongs to the client global exposed by awesome. This has some fields, too:

the signal API again; one thing that is easy to miss is that all signals on objects are also emitted on its class, e.g. doing c:emit_signal("foo", true) calls all callbacks for "foo" on this client with argument true and also all callbacks on the client global with arguments c, true,
a counter of "how many instances of this class do currently exist?" as .instances (this is a function returning the number, I think); this is not used anywhere, so can be left out, but it still exists and is sometimes useful in debugging leaks ("oh, the number of live client objects is way too high"),
A special "callback registering" API set_index_miss_handler and set_newindex_miss_handler; I think this gets the same arguments as the corresponding metafunctions and is used in Lua to implement some additional magic on individual objects, not the class itself (e.g. makes c.floating return c.data.floating, but is also used to implement read-only properties etc; internally this calls getter and setter functions) (implemented e.g. in lib/awful/client.lua by calling gears.object.properties(client, { getter_class = awful.client.object, setter_class = awful.client.object, getter_fallback = client.property.get, setter_fallback = client.property.set }).

I'll try to give your rust code a close look later.

psychon · 2017-12-29T08:00:11Z

+    pub surface: Option<ImageSurface>,
+    geo: Geometry,
+    refreshed: bool,
+    // TODO Refresh callback


You are using an ImageSurface which allocates its own memory to draw to; awesome uses a XcbSurface drawing to the pixmap. Thus, you don't need a pixmap member here, it's just the "target" for drawing in awesome.

The refresh callback is just an internal implementation detail. For drawins it copies from the drawable's pixmap to the screen and for titlebars it does basically the same. However, for your case it should be enough to just always copy the contents of the surface to the screen on each frame and basically ignore the refresh function that Lua calls (it is always valid to use the contents of the surface, the refresh callback from Lua is just a "I changed something"-signal to force a repaint; this can of course be optimized later).

Edit: Oh, okay, I forgot about the refreshed member. Okay, refresh can just set that boolean and you only draw if it is true...

Thanks, I had a feeling I didn't need a pixmap but I hadn't looked into whether or not I needed it.

Ok so I don't even really need to add it as a field, I can just have a refresh function that checks the bool and only draws if it's true then. That will simplify this.

psychon · 2017-12-29T09:21:23Z

    use super::dummy;
    builder.method("connect_signal".into(), lua.create_function(dummy))?
-           .method("__call".into(), lua.create_function(dummy_create))
+           .method("__call".into(), lua.create_function(|lua, _: Value| Drawin::new(lua)))


Oh and the global (e.g. drawin) when called get a table of properties to be applied, for example visible = true (but the table can also contains keys that the C code does not understand and thus ignores). This code looks like you explicitly ignore these extra arguments.

Nice catch, I'll fix that when I fix the object creation (so I can throw those unknown keys into data)

I don't think awesome stores unknown keys into data, it just really completely ignores them.
This seems to be done in luaA_class_new in common/luaclass.c: All strings for which it finds a lua_class_property_t are handled, all others are just silently ignored.
(This is like this mostly for "historic reasons", I guess; I don't know if it makes sense or not)

psychon · 2017-12-29T09:28:57Z

+        }).unwrap_or_else(|err| {
+            warn!("err: {:#?}", err);
+            Some("pc+us+inet(evdev)".into())
+        }).unwrap())


I liked your previous "just return static fake data" version of this more. :-)
On a more serious note, I would expect a wayland compositor to already have the required data here somewhere. From a quick look at wlroots, this data seems to be in struct roots_keyboard_config (which is part of the example compositor, so perhaps wlroots requires the compositor to handle this itself?).

In wlc (the current compositor framework Way Cooler uses) that information is not exposed (or at least, not anywhere I can see). It, along with xwayland stuff, is all handled internally.

When we switch to wlroots though that will definitely be what we do. Also yes you're correct we have to handle it ourselves--that's the main benefit of wlroots actually is that we have to manage more stuff (compared to wlc). This increases the complexity of the compositor but also gives enough control that we can do stuff like this.

I'm going to keep this really bad version, since it actually gets the required info necessary for the user. When we switch wlroots though it will definitely change :)

psychon · 2017-12-29T09:37:08Z

 /// This actually does nothing, since this is Wayland.
-fn register_xproperty<'lua>(_: &'lua Lua, _: Value<'lua>) -> rlua::Result<()> {
-    warn!("register_xproperty not supported");
+fn register_xproperty<'lua>(lua: &'lua Lua, (name, v_type): (String, String)) -> rlua::Result<()> {


I think I would implement this purely in Lua (somehow). In awesome, awesome can be restarted while the X11 server continues running, so there it makes sense to save things in the X11 server. For way-cooler, a restart surely loses all the state (right?), so emulating xproperties in Lua is okay.
What do you think?
(No, I don't know how to actually do this; it's just a thought.)

We may still need to implement this actually, because we will be relying on xwayland a lot and, AFAIK, the xwayland server lets you register xproperties to the clients still.

If the main use case is to store state in case of a restart though, that means we may need to do some hoops to achieve this...

You're right in that if way-cooler was to restart (e.g the process died) then we would lose all state because Way Cooler is not only the window manager but also the display server. If the use case is primarily recovery from the process dying (e.g from an error) then we would need to achieve this via some other way.

One thing we could do is run Lua in a totally separate process and communicate between Way Cooler and Lua via IPC. Though that would obviously be:

Slower

Way more complicated

That would achieve the same thing Awesome currently does with X11 (since they are separate processes there) but it would mean more work.

Depending on the use case, this could drastically affect the design. But it shouldn't affect implementing the interface, as I'll be moving things around when we move to wlroots anyways. I'll be thinking about this though, thank you.

Well, the "global state" (xproperties on root) is not all that interesting, I think. And xproperties on individual client objects will still be lost since when the display server dies, all clients die with it.

Yes, this xproperty API is from a time when awesome was still restarting whenever the screen configuration changed. Ever since awesome stopped doing that, I haven't seen much use of this API.

psychon · 2017-12-29T09:41:55Z

+                                   // TODO
+                                   Some(lua.create_function(screen_new)),
+                                   Some(lua.create_function(get_visible)),
+                                   Some(lua.create_function(set_visible))))


Huh? Screens don't have a visible property and Lua cannot (that easily) create screens.

Whoops, looks like an oversight. I'll remove those.

Timidger · 2017-12-29T17:22:00Z

Thanks for the detailed writeup! I'm going to work on this PR more and get it to be compliant ASAP. I'm mostly trying to focus on wlroots, since that's going to necessitate mostly starting from scratch on the compositor, but this is really important to enable both me and others to work on whatever part of the Awesome API that seems easiest / most interesting to work on.

a data table that the C code saves but does not otherwise care about; this is used to e.g. save the floating state of a client and was needed so that Lua's garbage collector knows that this table is only kept alive by the client and can be garbage collected together...

Ahhh, ok that was the missing piece in my information. Previously I tried to store these things in the object itself but ran into a lot of issues which prompted the discussion in this rlua issue thread. Here's the relevant takeaway from the thread:

Just so you know though, this is almost NEVER a good idea. This bindings system is not designed to have Lua handles inside UserData, because Lua itself cannot garbage collect through a rust UserData, so you can very easily end up with unbreakable cycles.

It's not 1-1 with Awesome, but from a user's perspective as long as they don't do weird raw setting/getting on the data I give them it should be all good.

Doing this so it can still work on master. If it's uncommented, then you need to fix layoutboxes since it's used in the default rc.lua

Timidger · 2018-01-03T21:58:17Z

Ok I think I fixed all the issues I had with this PR. Still some rough edges around the core object stuff, but nothing blocking working on the other issues. Gonna give this one more look through and then merge it later today.

Timidger added the Awesome label Dec 10, 2017

SethBarberee reviewed Dec 27, 2017

View reviewed changes

Timidger added 28 commits December 27, 2017 12:34

Added class.allocate, proper global name init

2df5fac

Fixed setting state (and fixed tests)

c356fa9

Work on drawable, core to drawin

ef0e1e4

drawin work

a1b77a2

Dummys for the xkb functions

ee4bd80

I really need to implement these properly

Added _really_ bad hack for xkb_get_group_names

2bc305a

Turns out I was just using it wrong (xcb that is)

7684fa8

Implement xkb_get_group_names properly

721f3d7

Move lua init after compositor init for xwayland

49ae7f0

By movinging it before hand, xcb can talk to xwayland and get e.g xkb information.

Set up xcb connection, store in Lua

faa4068

Also adds more safety checks for unsafely getting the group names

Ensure connection not dropped xkb_get_group_names

5376912

Even in the error case we ensure that the connection isn't closed.

Implemented xkb_get_layout_group

2ec54b0

null -> NULL

6ca6536

Implemented register_xproperty

a4055b6

Stubbed out screen functions, not working yet...

5682cc6

Because lua doesn't have the screen, which it gets from the client or from the mouse. So I need to implement it for mouse because that's the easiest way. yay.

Fixed screen creation hackily...I need to fix that

8e1a761

Keygrabber can't fail if it's not loaded yet now

40362dc

Removed bad object setup code in screen

62ee399

Don't create conn connections if unnecessary

f0f1656

Store the xproperties in a global list

3ce6a82

Added custom __index for screen

3d5fcf6

Index mouse and screens correctly when fetching screen

9b879c6

The way it's done is crazy, but basically mouse has a reference to the screen table and then if screen global gets a screen instance we just return it. Again, it's really crazy

Fixed property getting, added screen.workarea prop

238af59

I was saving it on the instance of the class instead of the class itself, which makes sense in retrospect

Removed property setting for objects

2dd680c

You should set it on the class instead

Removed dead code annotation on add_to_meta

4ce5330

Added iterator for screens

751b5e1

Took way too long because the C code is inscrutable

Added "geometry" impl to drawin

9e78a67

Timidger added 3 commits December 27, 2017 12:37

Stubbed out "root.wallpaper", will make it work later

4e4877b

Added __call for Tag

5462545

Stubbed out "root.tags"

bdffbbd

Timidger force-pushed the awesomeWM/awesome_and_class_fixups_drawin_impl branch from 0eaafcb to bdffbbd Compare December 27, 2017 18:45

Timidger added 4 commits December 27, 2017 12:46

Added libxcb to travis build

15ffe42

Removed duplicate test notify from rc.lua

213d950

Clean up of comments and things

c579c55

Fixed tests

dc5a560

Had to update dummy-rustwlc -_-

Timidger force-pushed the awesomeWM/awesome_and_class_fixups_drawin_impl branch from 67659b2 to dc5a560 Compare December 28, 2017 01:11

Don't panic if xcb can't be established.

2993329

This is to get around issues with testing...also it's technically correct, as we don't have hard dependency on xwayland _quite_ yet

psychon reviewed Dec 29, 2017

View reviewed changes

Timidger added 6 commits December 29, 2017 14:10

Fixed object layout for good this time.

7289bfa

It's not 1-1 with Awesome, but from a user's perspective as long as they don't do weird raw setting/getting on the data I give them it should be all good.

Removed incorrect screen functions

5422ddd

Fixed comments and default refresh value

26a26a8

Made note about changing drawin new

f5ae9f0

Removed screen_new

2c99813

Removed screens list to get rc.lua to work

ccfd26c

Doing this so it can still work on master. If it's uncommented, then you need to fix layoutboxes since it's used in the default rc.lua

Timidger merged commit c263393 into master Jan 4, 2018

Timidger deleted the awesomeWM/awesome_and_class_fixups_drawin_impl branch January 4, 2018 17:32

Conversation

Timidger commented Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Timidger commented Dec 29, 2017

Uh oh!

psychon commented Dec 29, 2017

Uh oh!

psychon Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Timidger Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Timidger Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Timidger commented Dec 29, 2017

Uh oh!

Timidger commented Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Timidger commented Dec 10, 2017 •

edited

Loading

psychon Dec 29, 2017 •

edited

Loading

Timidger Dec 29, 2017 •

edited

Loading

Timidger Dec 29, 2017 •

edited

Loading