Conversation
Package Changes Through 29967cbThere are 1 changes which include tao with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
|
Fixed CI |
|
can't wait for the day we're finally back with winit. happy to see you getting rid of lazy_static :)
are you sure that's not needed? i don't know what it does from the top of my head but winit also still has that code. |
It's not unsound, since ToUnicode takes a Edit: This doesn't totally explain that the current use of |
|
Dropped the |
|
The diffs migrating from |
|
Genuine question, what's benefit of switching from And if we want to do it, I would prefer we split it in a separate PR so when we squash merge them, we get 2 different commits for checking with git blame later Also, you'll need to sign your commits 🙃 |
I've seen Not really strong arguments, but grug dev see unnecessary dependency, grug dev remove. |
|
less macros = more good |
|
To not make the same mistake as in my other PR, here's a program that crashes fn main() {
use tao::window::Fullscreen;
let event_loop = tao::event_loop::EventLoop::new();
let win = tao::window::Window::new(&event_loop).unwrap();
std::thread::scope(|s| {
s.spawn(|| loop {
win.set_fullscreen(None);
});
s.spawn(|| loop {
win.set_fullscreen(Some(Fullscreen::Borderless(None)));
});
});
}
|
There was a problem hiding this comment.
I am not the most familiar with the Linux implementation, the PR's code looks good and correct to me though
For the theme part, I initially did it through Mutex in #937 and later @amrbashir changed it to RefCell e595125, so maybe there's a reason for this?
| fullscreen: RefCell::new(attributes.fullscreen), | ||
| inner_size_constraints: RefCell::new(attributes.inner_size_constraints), | ||
| preferred_theme: RefCell::new(preferred_theme), | ||
| fullscreen: RwLock::new(attributes.fullscreen), |
There was a problem hiding this comment.
I think the chances for read/write are not different by too much, maybe we should use Mutex here?
There was a problem hiding this comment.
I used a RwLock because that is the most direct port of a RefCell to the multithreaded case. A Mutex can't have multiple readers, RwLock and RefCell can.
But scopes here make it clear there can't be deadlocks regardless of Mutex or RwLock, so we can move to Mutex.
|
Any chance to merge this? We can switch to |
|
Looking at the code, it seems like we use
I don't really know the reason for us to mark the |
|
The code doesn't look very defensively written, but when I was taking a look at the |
|
Here's what I understood, we can only move the mod foo {
use std::rc::Rc;
unsafe impl Send for Foo {}
// UNSOUND: #[derive(Clone)]
pub struct Foo(Rc<()>);
impl Foo {
pub fn new() -> Foo {
Foo(Rc::new(()))
}
}
}
use foo::Foo;
fn main() {
let foo = Foo::new();
// safe, can only move all Rc handles across thread boundary at the same time
std::thread::spawn(move || foo);
// UNSOUND: if we can clone, split Rc handles across threads
// let handle = foo.clone();
// std::thread::spawn(move || handle);
}That said, the atomics inside the |
|
It is only fine if the ones we initially cloned to the |
|
I see there are some additional things to investigate, but if we can't resolve those in the near future, does it make sense to delay this?
|
3 participants
Setting the theme from multiple threads will result in undefined behavior on Linux.
WindowisSyncbutRefCellis not. The methods that set the theme and fullscreen property take a&Windowand will internally launder it into a&RefCellaccessible from multiple threads.@Legend-Master