Fix the "Use this Display" button so that secondary displays can be disabled (#131)

[shavitmichael]

Fixes #131 Monitors are disabled by excluding them from any logical monitors during the ApplyMonitorsConfig call.

[jeremypw]

Thanks for the contribution! There is some lint - please check your changes pass testing with io.elementary.vala-lint. In particular there should be a space before an opening bracket. Don't worry about any pre-existing lint, we just do not want to add more here.

You have introduced a property is_active with an automatic setter but also introduced an explicit function set_active which sets this property. Do you need both ways of setting this property? It seems prone to conflict although I have not looked at it in detail yet.

Please confirm that all changes made are essential to fixing the linked issue.

[jeremypw]

Another issue:

• If a suspend and restore intervenes, re-enabling the internal monitor does not work properly.

• The confirmatory dialog is not always visible the first time (have to confirm with keyboard)

• After re-enabling, the monitor does not actually switch on the first time.

• If two suspends intervene with usage without the internal monitors used between suspends, I got to a state where I could not get the internal monitor to switch back on after many attempts even with logouts and reboots. Eventually I noticed that the properties menu of the internal monitor showed blank in the refresh-rate combo. When I selected a refresh rate then the monitor did switch on.

Nevertheless the login screen always appeared on the internal monitor OK.

[jeremypw]

This does not seem to work for one of my external monitors (connected to HDMI), but the other monitor (connected to VGA), switched on and off without problem. Not sure if this is a hardware or software issue at the moment.

This PR is probably exposing issues in other parts of the code unfortunately and we can only allow those operations where all serious knock-on effects have been addressed. :-(

[shavitmichael]

Thanks for the review!

Thanks for the contribution! There is some lint - please check your changes pass testing with io.elementary.vala-lint. In particular there should be a space before an opening bracket. Don't worry about any pre-existing lint, we just do not want to add more here.

Done :).

You have introduced a property is_active with an automatic setter but also introduced an explicit function set_active which sets this property. Do you need both ways of setting this property? It seems prone to conflict although I have not looked at it in detail yet.

Is there a canonical way of providing a custom setter implementation in valac? Would I have to explicitly define a member variable for it? I've made the default setter private for now, pending a better solution.

• If a suspend and restore intervenes, re-enabling the internal monitor does not work properly.

• After re-enabling, the monitor does not actually switch on the first time.

• If two suspends intervene with usage without the internal monitors used between suspends, I got to a state where I could not get the internal monitor to switch back on after many attempts even with logouts and reboots. Eventually I noticed that the properties menu of the internal monitor showed blank in the refresh-rate combo. When I selected a refresh rate then the monitor did switch on.

I wasn't able to reproduce these exactly, but I did run into similar issues where enabling a monitor would no longer work. I found that we were failing the set_configuration call because of incorrect sizes/arrangements

By closing and reopening the laptop lid It was possible to get to a state where the monitor appeared disabled in the plug but was actually switched on.

Latest commit now redraws the window when a config change is detected.

[jeremypw]

Is there a canonical way of providing a custom setter implementation in valac? Would I have to explicitly define a member variable for it?

Yes, the usual way of doing it is to have a private member holding the current value of the property e.g.:

private bool _is_active = true;
public bool is_active {
    get {
        return _is_active;
    }

    set {
        _is_active = is_mirror || value; // Aways active if is mirrored
    }
}

However, you cannot return a result this way. If you really need to do this it would be more consistent to have a public getter function as well and just use a private member to hold the value. Otherwise you have to access the value with one syntax and change it with a different syntax. Maybe better to name the function try_set_active () to indicate it can fail, and to distinguish from similar functions.

[jeremypw]

It really needs a thorough review of how properties of the various widgets are synchronised with virtual monitor properties (this matters more now you can deactivate monitors). Use bind_property () where possible. The comboboxes default active iters should be set as soon as the models are populated so that they cannot ever be -1.

[jeremypw]

@shavitmichael Please do not despair! We appreciate you efforts. This should be close to merging now, but if you really have no more time to work on it an elementary dev could adopt it, or fix any non-critical issues in a separate PR after it is merged.

[jeremypw]

@shavitmichael I am working on a PR for this branch which will (hopefully) finish it.

[jeremypw]

There are so many serious bugs in the master code it makes it hard to focus on fixing just this PR and determining whether isuues found are caused by this PR or not! I'll submit some other PRs as well I think.

[jeremypw]

I'll have very limited time until the weekend though :-(

[shavitmichael]

Thanks I definitely appreciate the help! I also found some time fix a minor issue with the "set primary" button on disabled monitors; we're definitely getting closer :) .

[jeremypw]

I started a branch here: https://github.com/elementary/switchboard-plug-display/tree/various-fixes, but I haven't had time to delint, test fully and separate out those fixes directly related to this issue yet. Please take from it whatever you like (if you need to). I will not have time to review your latest changes until Saturday.

[jeremypw]

I think I was confusing "scale" and "resolution (mode)" so I have removed a couple of comments.

[shavitmichael]

Answered and resolved some of the inline comments in the latest commit. Some questions are still open however. (Thanks again for the review and the help on the fixes! I've had a busy week and haven't had much time to work on this)

[jeremypw]

The main problem now is the messing up of the auto-layout when one or more of the monitors is disabled. I find that the two remaining monitors appear vertically offset (and cannot be made to appear aligned) although moving the the mouse pointer horizontally from one to the other shows that they are in fact aligned.

[praneetloke]

It doesn't look like there has been much progress on this for sometime now. What is needed here to get this through? I would love to see this get merged myself (and so will others affected by this issue). Trying to see if I can help in some way.

[jeremypw]

There was an outstanding issue when I last looked at this but the author has not resolved it yet. I'll have another look now that the latest master has been merged. If you are able to build and install this branch and test it with two or more monitors and report your findings that would help.

[praneetloke]

If you are able to build and install this branch and test it with two or more monitors and report your findings that would help.

I'll try to do this. I haven't explored the repo yet, but are there instructions somewhere on how to test this on my own installation of the OS? I haven't done this sort of thing before, so I am a total newbie at this.

Also I would only be able to test this by turning off by laptop's screen, and have display on the external monitor; basically just one screen output.

[jeremypw]

There are basic instructions in the README.md on this page: https://github.com/elementary/switchboard-plug-display but maybe not enough for a "newbie". More extensive information on installing the required tools are here: https://elementary.io/docs/code/getting-started#getting-started particularly the sections on the Basic Setup and Build System. Don't worry if it is too much - hopefully this will be progressed soon.

[praneetloke]

Alright. I managed to figure out how to build this locally and test it. It wasn't too bad after all.

Here's what I found:

Toggling the Use this Display switch causes a weird transition. Not sure if this is intended to be this way?

Switching off the built-in display and only having the external monitor on

Notice how the disabled display has moved to the right in the Display widget

The one nice thing I noticed is that the confirmation dialog now actually works. Previously, clicking the Restore previous configuration button did nothing for me, but now it correctly reverts to the previous configuration as I would expect it to.

[jeremypw]

@praneetloke Congrats on joining the ranks of source code builders and testers!

@shavitmichael Do you have the time to have a look at this PR again? If not I'll try and progress it.

[shavitmichael]

Sorry for the lack of progress here, but I no longer have the time to work on this.

I've lost quite a bit of context, but IIRC this fix surfaced a bunch of underlying issues in the existing codebase. IMO, as long as the user can't get in a bad state because of this PR however, it could be worth merging it as it is. Sure the interface might be a little buggy, but it's probably better than a disable button that does nothing...

[jeremypw]

I have pushed a PR (#228) based on this one that seems to solve the layout problems by displaying inactive displays separately as a button.

[shavitmichael]

That's awesome, thanks a lot for picking this bug back up Jeremy!

I'll close this PR and hope that yours moves moves forward :) .