Skip to content

[WIP] Basic WM #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

[WIP] Basic WM #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
772920e
Remove circular dependency between compositor and surface
charafau May 31, 2022
7171f26
Add support for focused view
charafau Jun 1, 2022
edcb00f
Add popup surface support
charafau Jun 4, 2022
8045de6
Add basic floating functionality
charafau Jun 7, 2022
d5f59fc
remove hash and equal from surface
charafau Jun 20, 2022
5fcf6f9
Add passing preffered size from surface to compositor
charafau Jun 20, 2022
fd9fdee
Remove reverse map
charafau Jun 21, 2022
7d23a40
Remove map find function
charafau Jun 22, 2022
cd07c96
Add window maximize event
charafau Jul 19, 2022
c348f7c
add minimize event handling
charafau Jul 19, 2022
f053820
pass fullscreen request to compositor
charafau Jul 20, 2022
a5805dd
pass window move event
charafau Jul 20, 2022
b3d8cd6
pass resize event
charafau Jul 20, 2022
2d16c6f
set preffered with from toplevel state for popups
charafau Jul 20, 2022
d5ac87d
refactor window decoration
charafau Jul 22, 2022
478aa92
add window resizing
charafau Jul 22, 2022
8f5c450
add window control
charafau Jul 22, 2022
45eab9e
pass surface width and height
charafau Aug 7, 2022
a2c7e1b
fix interceptor widget bindings
charafau Nov 15, 2022
01565e0
Merge branch 'fix-interceptor-widget' into basic-floating-wm
charafau Nov 15, 2022
0358c32
Rebased with master
charafau Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions compositor_dart/lib/compositor_dart.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// ignore_for_file: public_member_api_docs, sort_constructors_first
library compositor_dart;

import 'dart:async';
Expand All @@ -21,15 +22,33 @@ class Surface {
final int gid;
final int uid;

final Compositor compositor;

Surface({
required this.handle,
required this.pid,
required this.gid,
required this.uid,
required this.compositor,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Pass the compositor object around alongside every surface. Might result in quite a bit of boilerplate as the dart side library expands in size.
  2. 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.
  3. Store the Compositor object within each surface. Only downside I can think of here is memory, but that's only a pointer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

@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;
}

@override
int get hashCode {
return handle.hashCode ^ pid.hashCode ^ gid.hashCode ^ uid.hashCode;
}

@override
String toString() {
return 'Surface(handle: $handle, pid: $pid, gid: $gid, uid: $uid)';
}
}

class _CompositorPlatform {
Expand Down Expand Up @@ -81,8 +100,6 @@ class _CompositorPlatform {
}
}

final Compositor compositor = Compositor();

class Compositor {
static void initLogger() {
FlutterError.onError = (FlutterErrorDetails details) {
Expand Down Expand Up @@ -111,17 +128,19 @@ class Compositor {
pid: call.arguments["client_pid"],
gid: call.arguments["client_gid"],
uid: call.arguments["client_uid"],
compositor: this,
);
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);
}
Comment on lines -128 to +197
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

platform.addHandler("flutter/keyevent", (call) async {});
Expand Down
29 changes: 22 additions & 7 deletions compositor_dart/lib/surface.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class _MeasureSizeRenderObject extends RenderProxyBox {
if (oldSize == newSize) return;

oldSize = newSize;
WidgetsBinding.instance?.addPostFrameCallback((_) {
WidgetsBinding.instance.addPostFrameCallback((_) {
onChange(newSize);
});
}
Expand All @@ -46,8 +46,13 @@ class _MeasureSize extends SingleChildRenderObjectWidget {

class SurfaceView extends StatefulWidget {
final Surface surface;
final Compositor compositor;

const SurfaceView({Key? key, required this.surface}) : super(key: key);
const SurfaceView({
Key? key,
required this.surface,
required this.compositor,
}) : super(key: key);

@override
State<StatefulWidget> createState() {
Expand All @@ -61,15 +66,21 @@ class _SurfaceViewState extends State<SurfaceView> {
@override
void initState() {
super.initState();
controller = _CompositorPlatformViewController(surface: widget.surface);
controller = _CompositorPlatformViewController(
surface: widget.surface,
compositor: widget.compositor,
);
}

@override
void didUpdateWidget(SurfaceView oldWidget) {
super.didUpdateWidget(oldWidget);
if (oldWidget.surface != widget.surface) {
controller.dispose();
controller = _CompositorPlatformViewController(surface: widget.surface);
controller = _CompositorPlatformViewController(
surface: widget.surface,
compositor: widget.compositor,
);
}
}

Expand All @@ -90,7 +101,7 @@ class _SurfaceViewState extends State<SurfaceView> {
int? keycode = physicalToXkbMap[event.physicalKey.usbHidUsage];

if (keycode != null) {
compositor.platform.surfaceSendKey(
widget.compositor.platform.surfaceSendKey(
widget.surface,
keycode,
status,
Expand Down Expand Up @@ -120,10 +131,14 @@ class _SurfaceViewState extends State<SurfaceView> {
}

class _CompositorPlatformViewController extends PlatformViewController {
Surface surface;
final Surface surface;
final Compositor compositor;
Size size = const Size(100, 100);

_CompositorPlatformViewController({required this.surface});
_CompositorPlatformViewController({
required this.surface,
required this.compositor,
});

void setSize(Size size) {
this.size = size;
Expand Down
91 changes: 38 additions & 53 deletions demo/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import 'dart:io';
import 'dart:math';

import 'package:compositor_dart/compositor_dart.dart';
import 'package:compositor_dart/surface.dart';
Expand Down Expand Up @@ -54,45 +54,35 @@ class MyHomePage extends StatefulWidget {
}

class _MyHomePageState extends State<MyHomePage> {
int _counter = 0;
Compositor compositor = Compositor();
Surface? surface;
late Compositor compositor;
Map<int, Surface> surfaces = {};

_MyHomePageState() {
compositor.surfaceMapped.stream.listen((event) {
int? focusedSurface;

@override
void initState() {
super.initState();
compositor = Compositor();
compositor.surfaceMapped.stream.listen((Surface event) {
setState(() {
surface = event;
surfaces.putIfAbsent(event.handle, () => event);
focusedSurface = event.handle;
});
});
compositor.surfaceUnmapped.stream.listen((event) {
if (surface == event) {
setState(() {
surface = null;
});
}
});
}

void _incrementCounter() {
setState(() {
// This call to setState tells the Flutter framework that something has
// changed in this State, which causes it to rerun the build method below
// so that the display can reflect the updated values. If we changed
// _counter without calling setState(), then the build method would not be
// called again, and so nothing would appear to happen.
_counter++;
compositor.surfaceUnmapped.stream.listen((Surface event) {
setState(() {
surfaces.removeWhere((key, value) => key == event.handle);
if (surfaces.isNotEmpty) {
focusedSurface = surfaces.keys.last;
} else {
focusedSurface = null;
}
});
});
}

@override
Widget build(BuildContext context) {
Widget? surfaceView;
if (surface != null) {
surfaceView = SurfaceView(
surface: surface!,
);
}

// This method is rerun every time setState is called, for instance as done
// by the _incrementCounter method above.
//
Expand All @@ -103,9 +93,9 @@ class _MyHomePageState extends State<MyHomePage> {
onKeyEvent: (node, KeyEvent event) {
int? keycode = compositor.keyToXkb(event.physicalKey.usbHidUsage);

if (keycode != null && surface != null) {
if (keycode != null && focusedSurface != null) {
compositor.platform.surfaceSendKey(
surface!,
surfaces[focusedSurface]!,
keycode,
event is KeyDownEvent ? KeyStatus.pressed : KeyStatus.released,
event.timeStamp);
Expand All @@ -122,7 +112,7 @@ class _MyHomePageState extends State<MyHomePage> {
body: Center(
// Center is a layout widget. It takes a single child and positions it
// in the middle of the parent.
child: Column(
child: Row(
// Column is also a layout widget. It takes a list of children and
// arranges them vertically. By default, it sizes itself to fit its
// children horizontally, and tries to be as tall as its parent.
Expand All @@ -138,27 +128,22 @@ class _MyHomePageState extends State<MyHomePage> {
// axis because Columns are vertical (the cross axis would be
// horizontal).
mainAxisAlignment: MainAxisAlignment.center,
children: <Widget>[
const Text(
'You have pushed the button this many timesa:',
),
Text(
'$_counter',
style: Theme.of(context).textTheme.headline4,
),
Container(
color: Colors.amber,
padding: const EdgeInsets.all(8.0),
child: SizedBox(width: 500, height: 500, child: surfaceView),
),
],
children: surfaces.entries
.map((MapEntry<int, Surface> entry) => Container(
color: Colors
.primaries[Random().nextInt(Colors.primaries.length)],
child: SizedBox(
width: 500,
height: 500,
child: SurfaceView(
surface: entry.value,
compositor: compositor,
),
),
))
.toList(),
),
),
floatingActionButton: FloatingActionButton(
onPressed: _incrementCounter,
tooltip: 'Increment',
child: const Icon(Icons.add),
), // This trailing comma makes auto-formatting nicer for build methods.
),
);
}
Expand Down
13 changes: 3 additions & 10 deletions demo/pubspec.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ packages:
name: collection
url: "https://pub.dartlang.org"
source: hosted
version: "1.15.0"
version: "1.16.0"
compositor_dart:
dependency: "direct main"
description:
Expand All @@ -63,7 +63,7 @@ packages:
name: fake_async
url: "https://pub.dartlang.org"
source: hosted
version: "1.2.0"
version: "1.3.0"
flutter:
dependency: "direct main"
description: flutter
Expand Down Expand Up @@ -170,13 +170,6 @@ packages:
url: "https://pub.dartlang.org"
source: hosted
version: "0.4.9"
typed_data:
dependency: transitive
description:
name: typed_data
url: "https://pub.dartlang.org"
source: hosted
version: "1.3.0"
vector_math:
dependency: transitive
description:
Expand All @@ -185,5 +178,5 @@ packages:
source: hosted
version: "2.1.2"
sdks:
dart: ">=2.16.0 <3.0.0"
dart: ">=2.17.0-0 <3.0.0"
flutter: ">=1.17.0"
23 changes: 12 additions & 11 deletions src/surface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,18 @@ static void focus_view(struct fwr_view *view, struct wlr_surface *surface) {
/* Don't re-focus an already focused surface. */
return;
}
if (prev_surface) {
/*
* Deactivate the previously focused surface. This lets the client know
* it no longer has focus and the client will repaint accordingly, e.g.
* stop displaying a caret.
*/
struct wlr_xdg_surface *previous = wlr_xdg_surface_from_wlr_surface(
seat->keyboard_state.focused_surface);
assert(previous->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL);
wlr_xdg_toplevel_set_activated(previous->toplevel, false);
}
// TODO: we should manage changing focus on dart side
// if (prev_surface) {
// /*
// * Deactivate the previously focused surface. This lets the client know
// * it no longer has focus and the client will repaint accordingly, e.g.
// * stop displaying a caret.
// */
// struct wlr_xdg_surface *previous = wlr_xdg_surface_from_wlr_surface(
// seat->keyboard_state.focused_surface);
// assert(previous->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL);
// wlr_xdg_toplevel_set_activated(previous->toplevel, false);
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you remember what sort of exception you get?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check today and let you know

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

struct wlr_keyboard *keyboard = wlr_seat_get_keyboard(seat);
/* Activate the new surface */
if(view->surface->role == WLR_XDG_SURFACE_ROLE_TOPLEVEL) {
Expand Down