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

dbus: DisplayConfig: support monitor configuration #912

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

valpackett
Copy link

happy new year 🎉 this here PR extends the org.gnome.Mutter.DisplayConfig implementation enough to make the GNOME display settings work

niri-gnome-monitor-settings.webm

(XDG_CURRENT_DESKTOP=gnome gnome-control-center)

Copy link
Owner

@YaLTeR YaLTeR left a comment

Choose a reason for hiding this comment

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

Hey, thanks, this is pretty cool! Left some comments, also prepare to upgrade to zbus 5 since I'll be merging that PR soon.

@@ -45,6 +82,137 @@ pub struct LogicalMonitor {

#[dbus_interface(name = "org.gnome.Mutter.DisplayConfig")]
impl DisplayConfig {
async fn get_resources(
Copy link
Owner

Choose a reason for hiding this comment

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

Is this method actually needed? The panel shows up as is for me, and I cannot actually find any calls to GetResources in gnome-control-center: https://gitlab.gnome.org/search?search=GetResources&nav_source=navbar&project_id=660&group_id=8&search_code=true&repository_ref=main

Copy link
Author

Choose a reason for hiding this comment

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

The thing that calls it is gnome-rr but I forgot where I encountered usage of that API… ah. Not for Settings, but it's required for gsd-color. I'd like to get Night Light working, at least via an external client that would read g-s-d's temperature output and do wlr-gamma-control-unstable-v1 based on it, but for that gsd-color must actually work

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe in a separate PR then?

Copy link
Owner

Choose a reason for hiding this comment

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

Bumping. Could you remove this change from this PR?

src/dbus/mutter_display_config.rs Outdated Show resolved Hide resolved
src/dbus/mutter_display_config.rs Outdated Show resolved Hide resolved
src/dbus/mutter_display_config.rs Outdated Show resolved Hide resolved
@valpackett valpackett force-pushed the push-qpmvznoktkvn branch 3 times, most recently from d0f738b to 9291b91 Compare January 4, 2025 05:14
@valpackett
Copy link
Author

Fixed commit descriptions, improved style according to discussions above, rebased on top of the zbus 5 changes, added fractional scales 🚀

@@ -45,6 +82,137 @@ pub struct LogicalMonitor {

#[dbus_interface(name = "org.gnome.Mutter.DisplayConfig")]
impl DisplayConfig {
async fn get_resources(
Copy link
Owner

Choose a reason for hiding this comment

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

Bumping. Could you remove this change from this PR?

@@ -800,6 +800,22 @@ pub enum Transform {
Flipped270,
}

impl Transform {
/// Return numeric identifier matching the `transform` enum of the `wl_output` interface.
pub fn to_wayland_id(self) -> u32 {
Copy link
Owner

Choose a reason for hiding this comment

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

This is not a "to wayland id", this is a "to mutter-specific private dbus interface id". So it should be left inside mutter_display_config.rs.

Copy link
Author

Choose a reason for hiding this comment

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

The actual numbers do match wayland.xml, not sure why you think mutter could ever switch to different numbers 🙄

Copy link
Owner

Choose a reason for hiding this comment

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

It couldn't. But this code was written specifically to convert according to the mutter xml, and isn't used anywhere else

src/dbus/mod.rs Outdated Show resolved Hide resolved
connector
)));
}
let msgs = vec![
Copy link
Owner

Choose a reason for hiding this comment

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

In comparison, the wlr-output-management impl creates and sets a complete new output configuration from scratch. Wouldn't that also be more appropriate here?

As far as I understand, the functional difference is that it is applied all at once, without repositioning outputs every step. That said, I think your approach should result in the same outcome anyway? So I guess this code can stay as is.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, that's possible? Right, I'm not restricted to ipc related API now that I'm using the channels..

Copy link
Author

Choose a reason for hiding this comment

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

Had to extract the find-or-create-config-by-name part from apply_transient_output_config but I think it does look better this way, creating the niri_config::Output from scratch.

Actually the outcome wasn't exactly the same: apply_transient_output_config was calling reload_output_config after every action…

@valpackett valpackett force-pushed the push-qpmvznoktkvn branch 2 times, most recently from 7e9b5aa to 8c22145 Compare January 5, 2025 22:26
This enables gnome-control-center to apply display configuration
changes. Only temporarily, persistence is ignored currently.
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.

2 participants