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

Check if gtk is initialised in get_monitors before assuming so in get_monitors #1946

Merged
merged 6 commits into from
Sep 12, 2021

Conversation

JAicewizard
Copy link
Contributor

@JAicewizard JAicewizard commented Aug 30, 2021

fixes #1941

@jneem
Copy link
Collaborator

jneem commented Sep 7, 2021

Do you know why the CI is failing? Should it be gtk::init?

@JAicewizard
Copy link
Contributor Author

uhh, im not sure how that slipped past me. no its supposed to be GDK, but I needed to scope it first. Maybe a rebase mistake.

@jneem
Copy link
Collaborator

jneem commented Sep 7, 2021

Ok, because I think gdk::init exits on failure, whereas gtk::init doesn't. (I'm not sure, because the rust bindings don't seem to include gdk_check_init, and the docs for gdk::init are empty...). I suspect we don't want to exit on failure here, and even if we do, I think we need a better error message before exiting.

@JAicewizard
Copy link
Contributor Author

Yes gtk::init does not terminate the program on failure, instead it falls back to a textual interface setup presumable without GDK.

If we want to use gdk::DisplayManager we need to init gdk, if thats indirectly via gtk or not we somehow need to init gdk. Using the gtk::init might succeed in that call specifically (with an error), but later it will crash when trying to use gdk::DisplayManager anyways.

@jneem
Copy link
Collaborator

jneem commented Sep 7, 2021

I mean if gtk::init gives an error, then we can just print an informative error message and return Vec::new(), right? There's no need to create the gdk::DisplayManager if we know that it will crash...

@jneem
Copy link
Collaborator

jneem commented Sep 12, 2021

Oh, I think this fixes #1941, right, not #1800.

@JAicewizard
Copy link
Contributor Author

I am such a messy person sometimes, yeah you're right.

@jneem jneem merged commit 650ae0d into linebender:master Sep 12, 2021
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.

Panic on Screen::get_monitors()
2 participants