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

X11 implementation of druid-shell #599

Merged
merged 36 commits into from
Apr 6, 2020
Merged

X11 implementation of druid-shell #599

merged 36 commits into from
Apr 6, 2020

Conversation

crsaracco
Copy link
Contributor

@crsaracco crsaracco commented Mar 4, 2020

This PR implements the first bullet point of #475: "bare-minimum X11 backend".

As stated in #475, I am opting to keep GTK as the main Linux backend since it is much more feature-complete; you have to explicitly ask for this new X11 backend by using --features use_x11.

I had to make some architectural hacks in the druid-shell code to make this work, so I don't expect this to plug right in to druid itself quite yet. I hope to either hash this problem out in this PR, or identify a plan of attack going forward to align all of the backends together so that they all work in druid. More details on this below.

@cmyr laid out a good goal for this first PR in Zulip, so I've tried to follow it as closely as possible here.

I think a good goal for an initial PR would be that all of the interfaces are present (e.g. the crate can compile) and that the most essential functionality is in place; this would mean at least window handling, painting, and keyboard and mouse event handling.

Some other stuff isn't implemented quite yet -- but again, more details below.

What is implemented

  • All interfaces are present, and the crate compiles! ... at least druid-shell does. I don't think it's compiling quite yet in druid, because of the hacks I had to write (see below). This is okay-ish for now though, since this code is all behind a feature flag. You can test this with cargo run --example perftest --features use_x11 in the druid-shell directory on an X11 system.
  • Most of the window handling functionality is in place: WindowBuilder, set_title, show, close, bring_to_front_and_focus. I go over stuff that is explicitly not implemented yet in the next section.
  • Painting the window works (including animations), with two exceptions:
    • The draw surface doesn't resize when the window does; it will always stay the same as the initial window dimensions. This should be a relatively easy fix (I need to get the resize dimensions somehow, and then I either need to pass the dimensions to render() or to XWindow; haven't decided the better API yet).
    • Requesting the next redraw during animations is currently a terrible hack, but I'll go over that below.
  • Keyboard events are working, thank you @perlindgren! There was some discussion about the best way to implement this in the future, so this feature is currently in more of a "proof-of-concept, hey-look-we-can-handle-keyboard-events" state, rather than a "100% finished product" state. We expect that the actual implementation might change in the future, but some more discussion is needed I think.
  • Mouse events are roughly in the same state as keyboard events, AFAIK (but thanks again to @perlindgren for implementing this too!)

What is explicitly not implemented yet

  • menus: I think we're going to have to draw these ourselves for X11 (???). There may be some other way to do it by requesting the window manager to do it for you, but I didn't dig too deep into it yet. Suggestions welcome here.
  • errors
  • timers
  • the "idle handle"
  • custom cursors
  • file dialogs
  • DPI scaling
  • the clipboard
  • locales (I think this is still a TODO for the rest of druid?)

Ugly, ugly hacks

  • X11 uses a "global" XCB connection to the X server (see application.rs). We handle events by fetching them from this global connection, parsing the event, and figuring out which window to route the event to for event handling purposes. Because of this, I need to store a map from window ID to XWindow inside the RunLoop object. Because of this, I needed to pass the RunLoop instance into WindowBuilder, so that we can connect the new window in that map, so that we can handle events. This breaks the API that druid was using previously, because WindowBuilder::build now has a new argument. This causes us to not be able to use the X11 backend in druid itself, which I would like to resolve as part of this PR. What do?
    • Grep for TODO(x11/architecture) to see areas of code that are affected by this.
  • AFAIK, there is no good way to sync window redrawing to the screen's refresh rate. AFAIK, there's also no system timer to use in X11 to schedule stuff to happen asynchronously in the future. To (temporarily) remedy this, I just manually figure out the screen's refresh rate and sleep (!) for however long that is. I think I'm going to need to write some sort of redraw scheduler to handle this for a multi-window setup? Suggestions definitely welcome here.

Next steps

This feature is partly an exploration into whether or not we can use Druid for VST plugins; we plan on working on #468 next, and make a proof-of-concept VST that works in a Digital Audio Workstation (DAW). We are hoping that this works out pretty smoothly, but there may be opportunities to identify some issues with Druid architecture (for certain use-cases) if not.

@crsaracco
Copy link
Contributor Author

One additional thing I found: Closing the window via the window manager's X button doesn't terminate the program. You have to manually ^C it to get it to quit.

Is this something I missed in druid-shell, or is this something I need to go implement in XCB code?

This is trivial except for the unification of 'RunLoop' and
'Application', which I may or may not have managed correctly.
@cmyr cmyr mentioned this pull request Mar 5, 2020
cmyr and others added 3 commits March 5, 2020 16:35
This also includes some general ci tweaks to make things a bit
more sensible, optimistically.
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Okay a bunch of little observations, this seems broadly okay though?

druid-shell/Cargo.toml Outdated Show resolved Hide resolved
[target.'cfg(target_os="linux")'.dependencies]
cairo-rs = { version = "0.8.0", default_features = false }
cairo-rs = { version = "0.8.0", default_features = false, features = ["xcb"] }
Copy link
Member

Choose a reason for hiding this comment

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

it's annoying that we need to always have these on for linux; it means that our x11 backend depends on gtk. I'm not sure what the best solution is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I guess we could technically just split that and make more-specific cfg targets?

Copy link
Member

Choose a reason for hiding this comment

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

unfortunately you can't use feature targets in this way. :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, dang. Sounds like it should be a feature. Wonder if there's a Rust / Cargo feature request laying around somewhere...

druid-shell/examples/perftest.rs Outdated Show resolved Hide resolved
druid-shell/examples/shello.rs Outdated Show resolved Hide resolved
druid-shell/examples/shello.rs Outdated Show resolved Hide resolved
}
}

impl Into<StrOrChar> for KeyCode {
Copy link
Member

Choose a reason for hiding this comment

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

a few things going on here: StrOrChar is not intended to be used directly; it is a way to allow the KeyEvent constructor to take either a str or a char.

More importantly, KeyCode cannot be used on its own to determine the string value of a key press. The KeyCode refers to a specific physical key; to interpret this we need to map it through the user's current locale and keyboard layout. (I think we talked about this? It's on zulip somewhere)

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'm unsure what to do here too -- paging @perlindgren for help)

druid-shell/src/platform/x11/keycodes.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/runloop.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
@crsaracco crsaracco mentioned this pull request Mar 16, 2020
15 tasks
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

I am happy to approve and merge on the condition that nobody holds me accountable later 😁

@cmyr cmyr added the S-waiting-on-author waits for changes from the submitter label Mar 22, 2020
@luleyleo luleyleo added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Apr 5, 2020
@luleyleo
Copy link
Collaborator

luleyleo commented Apr 5, 2020

I've been playing around with the examples using the X11 back end on Wayland
Will try it on X11 next.

Issues:

  • Window flashes white when resizing quickly
  • Resizing the window on the left site makes it blink temporarily towards the direction I drag on the right. This goes completely crazy when sizing it < 0 width, the window just flies away to the right 😂
    Does not happen when resizing on the right side
  • Application does not exit when closing the window
  • Some background flashing when scrolling (dragging the scroll bar) or when hovering over buttons in the list demo

Maybe issues, maybe not implemented yet:

  • No keyboard modifiers? At least shift does not work
  • No scrolling

@luleyleo
Copy link
Collaborator

luleyleo commented Apr 5, 2020

So on X11 could confirm the same issues but:

  • They are all much more subtle, especially the flashing
  • The issue with the window flying exists here as well but behaves slightly different:
    On Wayland the window gets smashed to the right end of my monitor, then stays there
    On X11 the window keeps cycling through my monitors until I stop
    In both cases I can make it large again by dragging left

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

I've added some comments which I would prefer to be solved before merging but looks really good overall 👍

Comment on lines +32 to +38
lazy_static! {
static ref XCB_CONNECTION: XcbConnection = XcbConnection::new();
}

thread_local! {
static WINDOW_MAP: RefCell<HashMap<u32, XWindow>> = RefCell::new(HashMap::new());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These seem to be used only inside Application, so I don't see the reason for them to be static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to call get_connection, get_screen_num, and add_xwindow from other modules, and (a reference to) Application doesn't seem to be passed around at this time -- so those methods need to be static.

Ideally these can just move to member fields in Application in the future, though.

druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/x11/window.rs Outdated Show resolved Hide resolved
druid-shell/src/common_util.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

All good now, thanks 👍

@crsaracco
Copy link
Contributor Author

Okay, I think a recap would be useful here:

  • First, the PR description/comment/thing is a bit out of date now. I can update it if anyone thinks it would be worth it (for referencing in the future, for example)
  • Closing the window doesn't actually terminate the program; I'm not sure if I missed some druid/druid-shell code to do this, or if I missed some xcb code to do this. Any insight? If it's not something obviously druid-related, it's probably XCB-related.
  • Keyboard and mouse event support are in a "proof-of-concept" phase, and will likely need some work in the future to make it production-grade. There are some lingering and unresolved conversations buried in the Zulip history about it, so we'll probably have to dig those up. I think we can leave these as-is for this first PR though -- it should be good enough to start POCing VSTs (and I don't think anyone else currently has an X11 backend usecase yet?).
    • In particular: nope, no keyboard modifiers yet. Not sure how complicated that is -- might be easy, might be difficult.
    • Scrolling support in particular isn't implemented yet, but that actually should be a fairly quick fix -- you should just need to follow the pattern of the other mouse/keyboard events and enable the scroll-related mask in the window event mask (I'd have to look up which one it is). Could fix it now or leave it for now, indifferent.
  • Window redraw flashes: I noticed this on my system too. It might be related to the terrible way I'm refreshing the screen for animations (sleeping), or it may be that I just need to figure out double-buffering the draws or something. Good enough to POC VSTs IMO, but definitely need to be fixed in the future.
  • The window flying error sounds hilarious, haha. I couldn't repro on my DE (Cinnamon), but that should probably be fixed too. VSTs aren't typically resized, so we probably won't notice that there.

I think that's everything -- does anyone consider any of these blocking? I can see what I can do to implement them; otherwise, I'll make sure they're noted in #475.

@luleyleo
Copy link
Collaborator

luleyleo commented Apr 5, 2020

@cmyr I think we could merge this now.

Might be interesting to create issues for the things that still need to be done and maybe create a project for the X11 back end, that could make further contributions to this a lot easier (although that is quite some organizational work 😅 )

@crsaracco
Copy link
Contributor Author

I was thinking the same thing, and would be happy to help if possible!

@cmyr cmyr merged commit a8d062a into linebender:master Apr 6, 2020
@cmyr
Copy link
Member

cmyr commented Apr 6, 2020

Sounds good, and would be very happy to have any issue opened for outstanding stuff.

@crsaracco
Copy link
Contributor Author

I've updated the "Progress" list in #475 to reflect all of the findings here.

As of right now there are 15 unchecked items in that list -- I'm not sure if you want me to flood the issue tracker writing up stuff for all 15, but I'd be happy to if that's what's desired.

For now, I figure what I'll do is "trickle" the issues into the tracker and write up the N things I find most important to fix next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants