Code review comment for lp:~brandontschaefer/mir/client-api-apply-remove-display-config

> 13 + * to get notified about hardware changes, so that the can apply a
> new config.
> Typo, the→they
>
> 15 + * \warning This request may be denied. Check that the request
> succeeded with mir_connection_get_error_message.
> You've copy-pasted these warnings. They don't apply; clients get notified via
> their registered error handler.
>
> 252 + mir::protobuf::Void * response,
> Nit: Coding style
>
> 291 + virtual void remove(std::shared_ptr<Session> const&) = 0;
> Not wild on the naming - could this be remove_session_configuration?
>
> 336 + else if ("remove_display" == invocation.method_name())
> And likewise, "remove_display_configuration" (or some equivalent)?

All good suggestions.

>
> 532 +void ms::MediatingDisplayChanger::remove(
> 533 + std::shared_ptr<mf::Session> const& session)
> 534 +{
> 535 + std::lock_guard<std::mutex> lg{configuration_mutex};
> 536 + config_map.erase(session);
> 537 + observer->session_configuration_removed(session);
> 538 +}
> Shouldn't this result in a hardware configuration change if (and only if) the
> session is currently focused? This will result in the configuration being set
> to the base config the *next* time the session is focused.

Hmm yes, i would think that sounds like a better behaviour.

>
> Which leads me into: please add some acceptance tests testing the expected
> behaviour. Particularly:
> 1) That calling remove_session_display_config from a client that hasn't set a
> session configuration is a no-op
> 2) That calling remove_session_display_config from a *focused* client that
> *has* set a session configuration results in a hardware configuration change
> 3) That calling remove_session_display_config from an *unfocused* client that
> *has* set a session configuration results in *no* hardware configuration
> change
> 4) That calling remove_session_display_config from an *unfocused* client that
> *has* set a session configuration results in the base configuration getting
> applied when the client becomes focused.

Good idea, I was thinking about doing some of these tests but ... wasnt sure.

I also wanted to write some unit test which matched the set session config but theres no accessing point to the config_map[]. Soo I think EXEPCT_CALLS(_) are still fine. Plus these other tests.