-
Notifications
You must be signed in to change notification settings - Fork 567
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
Conversation
Druid shell x11 pln
One additional thing I found: Closing the window via the window manager's Is this something I missed in |
This is trivial except for the unification of 'RunLoop' and 'Application', which I may or may not have managed correctly.
This also includes some general ci tweaks to make things a bit more sensible, optimistically.
Update to master
There was a problem hiding this 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
[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"] } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :/
There was a problem hiding this comment.
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...
} | ||
} | ||
|
||
impl Into<StrOrChar> for KeyCode { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this 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 😁
I've been playing around with the examples using the X11 back end on Wayland Issues:
Maybe issues, maybe not implemented yet:
|
So on X11 could confirm the same issues but:
|
There was a problem hiding this 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 👍
lazy_static! { | ||
static ref XCB_CONNECTION: XcbConnection = XcbConnection::new(); | ||
} | ||
|
||
thread_local! { | ||
static WINDOW_MAP: RefCell<HashMap<u32, XWindow>> = RefCell::new(HashMap::new()); | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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 👍
Okay, I think a recap would be useful here:
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. |
@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 😅 ) |
I was thinking the same thing, and would be happy to help if possible! |
Sounds good, and would be very happy to have any issue opened for outstanding stuff. |
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. |
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 todruid
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 indruid
. 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.
Some other stuff isn't implemented quite yet -- but again, more details below.
What is implemented
druid-shell
does. I don't think it's compiling quite yet indruid
, 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 withcargo run --example perftest --features use_x11
in thedruid-shell
directory on an X11 system.WindowBuilder
,set_title
,show
,close
,bring_to_front_and_focus
. I go over stuff that is explicitly not implemented yet in the next section.render()
or toXWindow
; haven't decided the better API yet).What is explicitly not implemented yet
druid
?)Ugly, ugly hacks
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 toXWindow
inside the RunLoop object. Because of this, I needed to pass theRunLoop
instance intoWindowBuilder
, so that we can connect the new window in that map, so that we can handle events. This breaks the API thatdruid
was using previously, becauseWindowBuilder::build
now has a new argument. This causes us to not be able to use the X11 backend indruid
itself, which I would like to resolve as part of this PR. What do?TODO(x11/architecture)
to see areas of code that are affected by this.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.