Skip to content
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.

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

[WIP] Basic WM #14

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
56 changes: 46 additions & 10 deletions compositor_dart/lib/compositor_dart.dart
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 @@ -20,16 +21,45 @@ class Surface {
final int pid;
final int gid;
final int uid;

final Compositor compositor;
final bool isPopup;
final int parentHandle;

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

required this.isPopup,
required this.parentHandle,
});

@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;
}
Copy link
Member

Choose a reason for hiding this comment

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

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.


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

class _CompositorPlatform {
Expand Down Expand Up @@ -79,9 +109,11 @@ class _CompositorPlatform {
],
);
}
}

final Compositor compositor = Compositor();
Future<void> surfaceFocusViewWithHandle(int handle) async {
await channel.invokeMethod("surface_focus_from_handle", [handle]);
}
}

class Compositor {
static void initLogger() {
Expand Down Expand Up @@ -111,17 +143,21 @@ class Compositor {
pid: call.arguments["client_pid"],
gid: call.arguments["client_gid"],
uid: call.arguments["client_uid"],
compositor: this,
isPopup: call.arguments["is_popup"],
parentHandle: call.arguments["parent_handle"],
);
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
38 changes: 31 additions & 7 deletions compositor_dart/lib/surface.dart
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,15 @@ class _MeasureSize extends SingleChildRenderObjectWidget {

class SurfaceView extends StatefulWidget {
final Surface surface;
final Compositor compositor;
final Function(Surface surface)? onPointerClick;

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

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

@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,
onPointerClick: widget.onPointerClick,
);
}
}

Expand All @@ -90,7 +105,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 +135,16 @@ class _SurfaceViewState extends State<SurfaceView> {
}

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

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

void setSize(Size size) {
this.size = size;
Expand Down Expand Up @@ -155,6 +176,9 @@ class _CompositorPlatformViewController extends PlatformViewController {
Offset scrollAmount = Offset.zero;
if (event is PointerDownEvent) {
eventType = pointerDownEvent;
if (onPointerClick != null) {
onPointerClick!(surface);
}
} else if (event is PointerUpEvent) {
eventType = pointerUpEvent;
} else if (event is PointerHoverEvent) {
Expand Down
8 changes: 8 additions & 0 deletions demo/lib/constants.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import 'package:flutter/material.dart';

final systemBorderRadius = BorderRadius.circular(6);

const double windowWidth = 400;
const double windowHeight = 500;
const double initialPositionX = 200;
const double initialPositionY = 70;
142 changes: 64 additions & 78 deletions demo/lib/main.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import 'dart:io';

import 'package:compositor_dart/compositor_dart.dart';
import 'package:compositor_dart/surface.dart';
import 'package:demo/constants.dart';
import 'package:demo/window.dart';
import 'package:flutter/gestures.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

Expand Down Expand Up @@ -54,58 +55,55 @@ class MyHomePage extends StatefulWidget {
}

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

int? focusedSurface;
late double mousePositionX;
late double mousePositionY;

@override
void initState() {
super.initState();

_MyHomePageState() {
compositor.surfaceMapped.stream.listen((event) {
mousePositionX = 0;
mousePositionY = 0;

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;
});
}
compositor.surfaceUnmapped.stream.listen((Surface event) {
setState(() {
surfaces.removeWhere((key, value) => key == event.handle);
if (surfaces.isNotEmpty) {
focusedSurface = surfaces.keys.last;
} else {
focusedSurface = 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++;
});
void focusView(int handle) {
if (focusedSurface != handle) {
compositor.platform.surfaceFocusViewWithHandle(handle);
focusedSurface = handle;
}
}

@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.
//
// The Flutter framework has been optimized to make rerunning build methods
// fast, so that you can just rebuild anything that needs updating rather
// than having to individually change instances of widgets.
return Focus(
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 @@ -115,50 +113,38 @@ class _MyHomePageState extends State<MyHomePage> {
autofocus: true,
child: Scaffold(
appBar: AppBar(
// Here we take the value from the MyHomePage object that was created by
// the App.build method, and use it to set our appbar title.
title: Text(widget.title),
),
body: Center(
// Center is a layout widget. It takes a single child and positions it
// in the middle of the parent.
child: Column(
// 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.
//
// Invoke "debug painting" (press "p" in the console, choose the
// "Toggle Debug Paint" action from the Flutter Inspector in Android
// Studio, or the "Toggle Debug Paint" command in Visual Studio Code)
// to see the wireframe for each widget.
//
// Column has various properties to control how it sizes itself and
// how it positions its children. Here we use mainAxisAlignment to
// center the children vertically; the main axis here is the vertical
// 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),
),
],
body: SizedBox.expand(
child: MouseRegion(
onHover: (PointerHoverEvent event) {
mousePositionX = event.position.dx;
mousePositionY = event.position.dy;
},
child: Stack(
children: surfaces.entries.map((MapEntry<int, Surface> entry) {
final isPopup = entry.value.isPopup;

return Window(
initialX: isPopup ? mousePositionX : initialPositionX,
initialY: isPopup ? mousePositionY : initialPositionY,
shouldDecorate: !isPopup,
onTap: () => focusView(entry.key),
child: SizedBox(
width: windowWidth,
height: windowHeight,
child: SurfaceView(
surface: entry.value,
compositor: compositor,
onPointerClick: (Surface surface) {
focusView(surface.handle);
}),
),
);
}).toList(),
),
),
),
floatingActionButton: FloatingActionButton(
onPressed: _incrementCounter,
tooltip: 'Increment',
child: const Icon(Icons.add),
), // This trailing comma makes auto-formatting nicer for build methods.
),
);
}
Expand Down
Loading