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

Add mode option to mirror #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TRPB
Copy link

@TRPB TRPB commented Feb 10, 2023

Adding the config option is step 1, I'll open another PR in the Wayfire branch which checks the option and sets the mode.

This is step 1, I'll also add a the a PR to the Wayfire branch which checks this.

This adds support for a config option like:

[option:DP-2]
mode = mirror DP-1 1920x1080@60000

Reason

Currently Mirroring works like this:

          case output_config::MODE_MIRROR:
            state.source = OUTPUT_IMAGE_SOURCE_MIRROR;
            state.mode   = select_default_mode();
            state.mirror_from = mode.get_mirror_from();
            break;

This sets the monitors default mode and mirrors another output. This can have side effects and unexpected behaviour when the mirrors resolution and refresh rate don't match the outputs. For example, if you mirror a 60hz screen on a 144hz screen the frames don't sync up neatly and it looks strangely jerky, by forcing the output to 120hz or 60hz there are fewer display issues.

There are also times when we might not want to use the default mode. I might prefer my monitor to run at the same resolution of the display being mirrored to scale in the hardware rather than software.

While we could just copy the source's mode here, we might try to copy a monitor that runs at 60hz to a monitor that runs at 59.9hz, not solving the problem. Rather than trying to guess at a best match, let's let the user decide.

My next PR is going to adjust the logic above to set the mode from the config if it's set, otherwise use the existing behaviour.

Copy link
Member

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

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

The feature makes sense in general. However I think the implementation should be made to just use sscanf multiple times.

@@ -437,6 +437,7 @@ TEST_CASE("wf::output_config::mode_t")
"1920x1080@59",
"1920x 1080 @ 59000",
"mirror eDP-1",
"mirror DP-1 1920x1080@60000",
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to also add some checks in the test that this is properly parsed.

@@ -1147,7 +1156,11 @@ stdx::optional<wf::output_config::mode_t> wf::option_type::from_string(
rr *= 1000;
}

return wf::output_config::mode_t{w, h, rr};
if (from.empty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Formatting: { on a new line.

ss >> from; // the mirror word
if (!(ss >> from))
if (ss >> from)
Copy link
Member

Choose a reason for hiding this comment

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

I am confused, is the code not supposed to continue running after that, so that we can parse the mode as well?

Also, I think it may be simpler (and more flexible wrt. whitespace handling) to try sscanf multiple times. One time with a format string like mirror %s <optionally the mode>, and if that fails, then scan a normal resolution, like the code below does.

Choose a reason for hiding this comment

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

59-1.txt
I've modified the logic a bit so the program execution is not prematurely finished before the mode string parsed. So now it works - but the code is a bit ugly. I am not C++ expert ufortunately. But for personal use that works.

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.

3 participants