Closed andrewbaxter closed 3 months ago
Thanks for the review! And whoops, I thought Draft:
would make it a draft like in gitlab... getting rusty here.
I made the changes above, and made a few other small changes while testing:
platform_impl
pub to access linux Window
- the name feels kind of internal-ish, but the pub
contents seems to be limited to types that are public and exposed elsewhere, so I don't think this is exposing sensitive internalsplatform_impl::EventLoopWindowTarget
from event_loop::EventLoopWindowTarget
and to get the gtk Application from that (required to create the gtk window)~FWIW I think event_loop.rs
and a couple other files I touched here aren't rustfmt'd (2 space indent, missing newlines) so I had to be careful when making changes - I think I've undone all the formatting but apologies in advance if I missed something.~ I'm not sure what happened, it looks like rust-analyzer ignored rustfmt.toml and reformatted everything, but now it's working again (and I fixed the formatting issues).
I think this is ready to go now.
With the changes to move methods into the traits, using an external gtk window now needs an invocation like:
<tao::window::Window as tao::platform::unix::WindowExtUnix<UserEvent>>::new_from_gtk_window(...)
which is pretty unintuitive. I'm not sure there's an easy way around that, but is there somewhere I could document that or provide an example?
you can juse use import the trait and use directly like this:
tao::platform::unix::WindowExtUnix;
let window = Window::from_gtk_window();
I think in this case the generic parameters for Window::from_gtk_window()
are ambiguous so you need to use the explicit syntax, but good point... I guess rust-analyzer will quick-fix the import and suggest <Window as ...>
so maybe it's more discoverable than I was thinking at first.
I made all the changes! I feel like this is pretty close now but let me know if there's anything else I missed. Thanks again for the reviews!
There are 1 changes which include tao with patch
Add another change file through the GitHub UI by following this link.
Read about change files or the docs at github.com/jbolda/covector
I had to force push so I can sign your commits, for future contributions, you need to setup commit signing, then you can sign past commit like this for example.
Thanks! Yeah fwiw I sign my commits, but I don't include personal information in my key so github refuses to verify them.
Fix #925
The existing
Window::new()
code can be roughly split into two parts: applying generic attributes to the window and connecting the window to the event loop. These can be fairly cleanly separated, so I did that - the newnew_from_gtk_window
method contains the latter, wherenew
now just contains the former and callsnew_from_gtk_window
for the latter.There are a few attributes that affect both the former and latter parts, and I presume in such cases the former is required for the latter to function properly. It's up to the caller to make sure that those prerequisites are met, but I'd like to add specific guidance to the new attribute struct documentation if this approach looks reasonable. (regarding
transparent
,cursor_moved
,fullscreen
, etc)I also made
set_skip_taskbar
public since it seemed simpler than adding another attribute, but happy to go another way.