[WIP] Basic WM #14

charafau · 2022-05-31T00:20:37Z

Done:

Remove circular dependency between compositor and surface
Add equals methods to Surface class
Change single surface view to multiple views in demo project
Commented out focusing code in surface for now
Refocusing view from dart compositor
Add isPopUp variable to surface object
Actual floating
View resizing in demo wm

Todo:
~~Get actual surface size from wlroots~~ view is actually resized when first appearing (SizeBox defines size) - we should probably check if that size or alright.

Fix pop up positions

Add equals methods to Surface class Change single surface view to multiple views in demo project Commented out focusing code in surface for now

hansihe · 2022-05-31T11:08:33Z

compositor_dart/lib/compositor_dart.dart

-      surfaces[surface.handle] = surface;
+      surfaces.putIfAbsent(surface.handle, () => surface);
+
      surfaceMapped.add(surface);
    });

    platform.addHandler("surface_unmap", (call) async {
      int handle = call.arguments["handle"];
-      Surface surface = surfaces[handle]!;
-      surfaces.remove(handle);
-      surfaceUnmapped.add(surface);
+      if (surfaces.containsKey(handle)) {
+        Surface surface = surfaces[handle]!;
+        surfaces.remove(handle);
+        surfaceUnmapped.add(surface);
+      }


For any given surface, I believe map and unmap should be only called strictly interspersed in a sequence:

map, unmap, map, unmap, ...

This should mean that:

When map is called, there should never be a surface of that handle in the surfaces map, we add it

When unmap is called, there should always be a surface of that handle in the surfaces, we remove it

We should likely add assertions for this, if this assumption is broken it would be a bug in the native code

I wonder if the fact that things fail here on multiple surfaces means there is an error somewhere in my code where multiple surfaces get the same handle id? I think there are some debug prints for when a surface is mapped with its handle id, are those different?

From what I've checked, handles were different but I think it was the way we managed compositor - now it's created in and managed in main.

hansihe · 2022-05-31T11:10:48Z

compositor_dart/lib/compositor_dart.dart

  Surface({
    required this.handle,
    required this.pid,
    required this.gid,
    required this.uid,
-    required this.compositor,


Should we keep a reference to Compositor from Surfaces or not?

I think it's not necassry. Compositor will be created in main, and probably main should keep track of it. Is there actually any use case where we might have more than 1 compositor ?

There will never be more than one compositor at a time in a given isolate, but the Compositor it belongs to is still part of the state conceptually in my mind.

At the very least it prevents us from having to pass it around everywhere along with the surface. Most times you have a Surface you need access to the compositor as well.

In my mind we have a few options:

Pass the compositor object around alongside every surface. Might result in quite a bit of boilerplate as the dart side library expands in size.

Just rely on the fact that there will only ever be one compositor object. Access it through a global everywhere. Not sure I like this, I like to keep state local even with singletons.

Store the Compositor object within each surface. Only downside I can think of here is memory, but that's only a pointer.

what about providing the compositor using an InheritedWidget?
the app in order to use the compositor will need to use as root a Compositor widget of some sort which will then be used to create, maintain and dispose the compositor instance and also expose it to descendants using Compositor.of(context).
This way a Surface won't need to hold a compositor instance but instead only the SurfaceView will need to get it, using the of method indeed.
It would also be possible to detect if the app is running with the compositor or not because of Compositor.maybeOf(context) returning null if no compositor is available for example

I would prefer not using the widget tree to pass around the compositor instance.

The main reason for this is that the compositor instance inherently has another lifetime compared to something in the widget tree.

It creates a platform channel and uses this to communicate with native code on the other side for the lifetime of the isolate. It also accumulates and maintains state for its whole lifetime, something conceptually incompatible with being maintained as part of the widget tree. It is analogous to the various Bindings in the flutter framework. (WidgetBinding for instance)

It's just inherently a singleton with a global lifetime, and I think we would save ourselves some pain and complexity in the long run by just treating it as such from the beginning.

That being said, whether we should pass it around as a part of the surface objects or access it as a global every time is still an open question for me. Having written this out and thought through it a bit more I'm leaning towards the global route.

Fair point then, i'm more leaning towards the global approach myself too

hansihe · 2022-05-31T11:15:05Z

src/surface.c

+	// 				seat->keyboard_state.focused_surface);
+	// 	assert(previous->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL);
+	// 	wlr_xdg_toplevel_set_activated(previous->toplevel, false);
+	// }


Focus should definitely be managed by the dart side, but does setting more than one surface as focused violate a wlroots invariant?

In which case it seems like a good idea to keep guard code that makes sure the previous surface is unfocused when we attempt to set a new focus.

I get exception from wlroots when I try to show second view and this if statement is here. Needs some fixes when we have proper focus management

Do you remember what sort of exception you get?

I will check today and let you know

sorry for late response. when I leave code as it was I get:

00:00:11.553 [../src/surface.c:74] viewhandle 2 simple: types/xdg_shell/wlr_xdg_toplevel.c:516: wlr_xdg_toplevel_set_activated: Assertion `surface->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL' failed. 00:00:11.553 [../src/flutter_wlroots.c:196] DART [flutter] handled call surface_map

Add changing focus for demo WM

HrX03 · 2022-06-02T13:58:47Z

I wonder if it would be appropriate to introduce a proper event api in this pr or it would be better to have it as a different pr. If a different pr would be optimal then i could work on it once this pr gets eventually merged

hansihe · 2022-06-02T14:39:02Z

How do you think the best way would be to make a proper event API?

HrX03 · 2022-06-02T19:07:17Z

I already did an event api for utopia and i think we could lean towards that kind of architecture. Basically, you add a listener of sorts to the event source which is nothing more than a callback method that gets called each time any event happens. It has one single params which is a subclass of an abstract Event class that represents the kind of event that was generated. You just have to do some type checking to see which event was generated, eventually cast the param (tho usually it's automatically done as long as it's locally scoped) and then access the instance params and do your thing.

Basically it would be something like Compositor.addEventHandler(callback) where callback is of type void Function(Event)

hansihe · 2022-06-18T14:44:05Z

compositor_dart/lib/compositor_dart.dart

+  @override
+  bool operator ==(Object other) {
+    if (identical(this, other)) return true;
+
+    return other is Surface &&
+        other.handle == handle &&
+        other.pid == pid &&
+        other.gid == gid &&
+        other.uid == uid &&
+        other.isPopup == isPopup &&
+        other.parentHandle == parentHandle;
+  }
+
+  @override
+  int get hashCode {
+    return handle.hashCode ^
+        pid.hashCode ^
+        gid.hashCode ^
+        uid.hashCode ^
+        isPopup.hashCode ^
+        parentHandle.hashCode;
+  }


If everything else is functioning as it should, there should be one and only one Surface class instance for every handle on the native side.

It should be safe to compare Surface classes by identity, so we probably don't need these implementations.

Add function to find handle in map

Add pointer to fwr_view in xdg_surface

fix qt pop ups

pass offsets for surface pass contante widht and height

charafau · 2022-11-15T18:30:51Z

There's some breakage after merge. Need to fix that

Remove circular dependency between compositor and surface

772920e

Add equals methods to Surface class Change single surface view to multiple views in demo project Commented out focusing code in surface for now

hansihe reviewed May 31, 2022

View reviewed changes

Add support for focused view

7171f26

Add changing focus for demo WM

charafau added 2 commits June 4, 2022 03:26

Add popup surface support

edcb00f

Add basic floating functionality

8045de6

hansihe reviewed Jun 18, 2022

View reviewed changes

charafau added 3 commits June 20, 2022 03:04

remove hash and equal from surface

d5f59fc

Add passing preffered size from surface to compositor

5fcf6f9

Remove reverse map

fd9fdee

Add function to find handle in map

charafau mentioned this pull request Jun 21, 2022

Expose additional surface properties to the dart side #17

Open

3 tasks

charafau added 14 commits June 22, 2022 19:38

Remove map find function

7d23a40

Add pointer to fwr_view in xdg_surface

Add window maximize event

cd07c96

add minimize event handling

c348f7c

pass fullscreen request to compositor

f053820

pass window move event

a5805dd

pass resize event

b3d8cd6

fix qt pop ups

set preffered with from toplevel state for popups

2d16c6f

refactor window decoration

d5ac87d

add window resizing

478aa92

add window control

8f5c450

pass surface width and height

45eab9e

pass offsets for surface pass contante widht and height

fix interceptor widget bindings

a2c7e1b

Merge branch 'fix-interceptor-widget' into basic-floating-wm

01565e0

Rebased with master

0358c32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Basic WM #14

[WIP] Basic WM #14

charafau commented May 31, 2022 •

edited

Loading

hansihe May 31, 2022

hansihe Jun 1, 2022

charafau Jun 1, 2022

hansihe May 31, 2022

charafau May 31, 2022

hansihe Jun 1, 2022

HrX03 Jun 2, 2022

hansihe Jun 2, 2022

HrX03 Jun 2, 2022

hansihe May 31, 2022

charafau May 31, 2022

hansihe Jun 1, 2022

charafau Jun 1, 2022

charafau Jun 2, 2022

HrX03 commented Jun 2, 2022

hansihe commented Jun 2, 2022

HrX03 commented Jun 2, 2022

hansihe Jun 18, 2022

charafau commented Nov 15, 2022

[WIP] Basic WM #14

Are you sure you want to change the base?

[WIP] Basic WM #14

Conversation

charafau commented May 31, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HrX03 commented Jun 2, 2022

hansihe commented Jun 2, 2022

HrX03 commented Jun 2, 2022

Choose a reason for hiding this comment

charafau commented Nov 15, 2022

charafau commented May 31, 2022 •

edited

Loading