WLR layer surfaces #203

Eskpil · 2021-10-11T21:49:29Z

I added a very tiny abstraction for creating wlr shells. It does nothing special, it just eliminates 50-100 lines of code when creating a wlr shell.

PolyMeilex · 2021-10-11T22:10:38Z

src/shell/wlr.rs

+/// Just a tiny abstraction making anchors easier. Anchor says something about where on the screen the shell is rendered
+
+#[derive(PartialEq, Copy, Clone)]
+pub enum Anchor {


I'm pretty sure that this should be a bitflag, as we can have multiple anchors at the same time.
You can use my Smithay impl as a reference: https://github.com/Smithay/smithay/blob/dd6919dd5fb1ac6571a3e7dff01b12a2102131fe/src/wayland/shell/wlr_layer/types.rs#L113

You can also see that in the protocol it is marked as bitfield: https://wayland.app/protocols/wlr-layer-shell-unstable-v1#zwlr_layer_surface_v1:enum:anchor

Well it is just a an abstraction. It goes down to the correct things i think under the hood. I just wanted to localize Anchor and Layer. When i think about it i probably should just have reexported Anchor and Layer.

I think i will just reexport them and dont bother with it.

Given the wlr layer shell is likely to be stabilized in the future, I would actually declare this bitflag/enum in module and just implement From for the wlr and ext and the sctk type so we don't need to break clients when the ext extension is initiated.

PolyMeilex · 2021-10-11T22:14:30Z

src/shell/wlr.rs

+
+/// A struct representing the wlr shell
+
+pub struct WlrShell {


Kinda nitpicky, but is wlroots shell really a good name?
As of now the only shell created by wlroots team is the layer shell, but in the future they might come up with some new ones, and then the name will be confusing.

Do you have any suggestions, i struggled myself that is why it became WlrShell

i509VCB · 2021-10-11T22:08:09Z

src/shell/wlr.rs

+
+/// A struct representing the wlr shell
+
+pub struct WlrShell {


This seems more like a LayerSurface of sorts?

Yeah i agree. A layer surface makes way more sense.

i509VCB · 2021-10-11T22:09:23Z

src/shell/wlr.rs

+    Closed,
+}
+
+/// Just a tiny abstraction making layers easier.  A layer specifies a z depth for where something is drawn. A normal xdg shell is normally drawn somewhere in between Bottom and Top


I would probably make the docs match the purpose of the enum, this is a bit confusing.

i509VCB · 2021-10-11T22:10:03Z

src/shell/wlr.rs

+}
+
+impl Anchor {
+    pub(crate) fn too_raw(&self) -> zwlr_layer_surface_v1::Anchor {


Might make more sense to implement From for both the type here and the scanner generated type.

Eskpil · 2021-10-11T22:28:39Z

Does it still belong in the shell directory or does it belong somewhere else?

PolyMeilex · 2021-10-11T22:36:25Z

Does it still belong in the shell directory or does it belong somewhere else?

As far as I can tell, it is the best place for shell abstractions

Eskpil · 2021-10-11T22:52:32Z

I totally removed my abstractions and just reexported the ones already provided. I also changed the name too LayerSurface.

i509VCB · 2021-10-12T23:33:14Z

src/shell/layer.rs

+/// Render event
+
+#[derive(PartialEq, Copy, Clone)]
+pub enum RenderEvent {


Implement Debug?

i509VCB · 2021-10-12T23:33:24Z

src/shell/layer.rs

+
+/// A struct representing the layer surface (wlr)
+
+pub struct LayerSurface {


Debug here also

i509VCB · 2021-10-12T23:33:55Z

src/shell/layer.rs

+    Closed,
+}
+
+/// A struct representing the layer surface (wlr)


I think it would be useful to go into detail about what a layer is in the context of Wayland and possible use cases for a layer

i509VCB · 2021-10-12T23:35:22Z

src/shell/layer.rs

+
+pub struct LayerSurface {
+    /// The raw wl_surface
+    pub surface: wl_surface::WlSurface,


I would probably make the surface, layer, anchor and dimensions accessible via functions similar to how the Window abstraction is. The surface() function specifically returning an &wl_surface::WlSurface

i509VCB · 2021-10-12T23:35:59Z

src/shell/layer.rs

+    /// The raw wl_surface
+    pub surface: wl_surface::WlSurface,
+
+    pub(crate) layer_surface: Main<zwlr_layer_surface_v1::ZwlrLayerSurfaceV1>,


This is fine for now, we can always introduce a LayerInner when ext-layer-shell is initiated into wayland-protocols

i509VCB · 2021-10-12T23:36:38Z

src/shell/layer.rs

+            layer_shell.get_layer_surface(&surface, Some(output), layer, "example".to_owned());
+
+        layer_surface.set_size(dimensions.0, dimensions.1);
+        // Anchor to the top left corner of the output


This comment probably doesn't apply

nop, i forgot to remove it after my CTRL C + CTRL V from the example.

i509VCB · 2021-10-12T23:37:19Z

src/shell/layer.rs

+            }
+        });
+
+        // Commit so that the server will send a configure event


Initial commit for configure I guess?

Yeah it as this point i suggest we hand it off to the user and let them do what they want.

src/shell/layer.rs

i509VCB · 2021-10-12T23:39:55Z

src/shell/layer.rs

+/// Render event
+
+#[derive(PartialEq, Copy, Clone)]
+pub enum RenderEvent {


I'd probably go for LayerEvent since Close isn't exactly an event related to rendering.

You are right, changed

i509VCB · 2021-10-12T23:43:48Z

src/shell/layer.rs

+    pub dimensions: (u32, u32),
+
+    /// The next render event
+    pub render_event: Rc<Cell<Option<RenderEvent>>>,


I would mimic what Window does here with callbacks.

Using a callback here makes sense and also provides the DispatchData the client has provided.

This would mean your LayerSurface::new would also now need to take a FnMut(Event, DispatchData) + 'static parameter.

Could you elaborate?

If you look at the methods to create a window you will see one of the parameters is a Impl which has type bounds defined in the where statement.

This FnMut the user passes in is essentially a lambda that is invoked when some event occurs on the window.

You'll see where you call quick_assign on the layer you create that you are given an event, that is where you will invoke the callback.

The DispatchData is a dynamic data container for some arbitrary data a user of the library passes in that is available when events come in.

Eskpil · 2021-10-13T00:04:42Z

Thanks for the feedback, will take a look at it.

elinorbgr · 2021-10-16T11:21:34Z

@Eskpil did you by chance forget to push a commit? There are a few comments you marked as resolved but that are not addressed in the current state of the PR.

Eskpil · 2021-10-19T14:43:03Z

@vberger I got a bit distracted with other things and i needed a tiny break from programming, will take a look at it again soon.

Eskpil added 2 commits October 11, 2021 23:20

added wlr shell abstractions

6ae2af4

added a tiny abstraction for creating wlr_shells

aae0049

PolyMeilex reviewed Oct 11, 2021

View reviewed changes

i509VCB reviewed Oct 11, 2021

View reviewed changes

Eskpil changed the title ~~Wlr shells abstraction~~ WLR layer surfaces Oct 11, 2021

satisfied the requests in the PR

befb718

i509VCB reviewed Oct 12, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WLR layer surfaces #203

WLR layer surfaces #203

Eskpil commented Oct 11, 2021

PolyMeilex Oct 11, 2021 •

edited

Loading

Eskpil Oct 11, 2021

Eskpil Oct 11, 2021

i509VCB Oct 13, 2021

PolyMeilex Oct 11, 2021

Eskpil Oct 11, 2021

i509VCB Oct 11, 2021

Eskpil Oct 11, 2021

i509VCB Oct 11, 2021

i509VCB Oct 11, 2021

Eskpil commented Oct 11, 2021

PolyMeilex commented Oct 11, 2021

Eskpil commented Oct 11, 2021

i509VCB Oct 12, 2021

i509VCB Oct 12, 2021

i509VCB Oct 12, 2021 •

edited

Loading

i509VCB Oct 12, 2021

i509VCB Oct 12, 2021

i509VCB Oct 12, 2021

Eskpil Oct 13, 2021

i509VCB Oct 12, 2021

Eskpil Oct 13, 2021

i509VCB Oct 12, 2021

Eskpil Oct 13, 2021

i509VCB Oct 12, 2021

Eskpil Oct 13, 2021

i509VCB Oct 13, 2021

Eskpil commented Oct 13, 2021

elinorbgr commented Oct 16, 2021

Eskpil commented Oct 19, 2021


		/// A struct representing the wlr shell

		pub struct WlrShell {


		/// A struct representing the layer surface (wlr)

		pub struct LayerSurface {

WLR layer surfaces #203

Are you sure you want to change the base?

WLR layer surfaces #203

Conversation

Eskpil commented Oct 11, 2021

PolyMeilex Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eskpil commented Oct 11, 2021

PolyMeilex commented Oct 11, 2021

Eskpil commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i509VCB Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eskpil commented Oct 13, 2021

elinorbgr commented Oct 16, 2021

Eskpil commented Oct 19, 2021

PolyMeilex Oct 11, 2021 •

edited

Loading

i509VCB Oct 12, 2021 •

edited

Loading