canonical / mir

The Mir compositor
GNU General Public License v2.0
621 stars 99 forks source link

WindowManager::add_display()/remove_display() #2148

Open AlanGriffiths opened 3 years ago

AlanGriffiths commented 3 years ago

In one of the implementations of this interface SystemCompositorWindowManager we have:

    void add_display(mir::geometry::Rectangle const& area) override
    __attribute__((deprecated("Mir doesn't reliably call this: it is ignored. Use add_display_for_testing() instead")));

And, in this implementation, the outputs are tracked by the ActiveOutputsListener interface.

But the other implementation SystemCompositorWindowManager uses this function to track the outputs.

There's clearly something wrong with the diagnostic, one of the implementations, or several of these.

AlanGriffiths commented 1 year ago

The implementation of BasicWindowManager (which ignores this function) is clearly more widely used than SystemCompositorWindowManager which does.

As we don't need two ways to do the same thing, we should update SystemCompositorWindowManager to use the ActiveOutputsListener interface and remove this function.

mattkae commented 1 year ago

Investigation

  1. BasicWindowManager inherits from ActiveOutputsListener.
  2. ActiveOutputsListener appears to be responsible for managing the state of connected outputs. These methods only appear to be called during DisplayConfigurationListeners::configuration_applied. According to DisplayConfigurationObserver::configuration_applied, this is applied after the DisplayConfiguration has been applied to the hardware.

Problem

Solution

AlanGriffiths commented 1 year ago

Reopening as there has been no change (so far) to the mess described in this issue.

As mentioned elsewhere we've now removed any dependency on the suspect logic and could

Drop shell::WindowManager::add_display() et alia (which would resolve #2975)

mattkae commented 1 year ago

@AlanGriffiths going to resolve the remainder of this one this morning!