Mir

Code review comment for lp:~andreas-pokorny/mir/allow-transparent-server-buffers

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

1. Wrapping: Please wrap lines earlier, ideally before column 80. The human brain loses track of the next line to read otherwise...
49 +CascadedDisplayConfigurationPolicy::CascadedDisplayConfigurationPolicy(std::shared_ptr<graphics::DisplayConfigurationPolicy> const& l,
50 + std::shared_ptr<graphics::DisplayConfigurationPolicy> const& r)
Although the class name seems excessively long too. Not sure what to do about that.

2. I think guard macros are meant to follow the namespace?.. So:
   MIR_DEMO_SHELL_CASCADED_DISPLAY_CONFIGURATION_POLICY_H
becomes:
   MIR_EXAMPLES_CASCADED_DISPLAY_CONFIGURATION_POLICY_H_

3. It's possibly too easy to enable translucency. That's an unusual problem, but it's absolutely critical to performance that no shell implementer accidentally uses translucency. It should be clearly marked as "for greeter use only", somehow.

4. Per #3, please remove ALL translucency support from demo-shell. We don't want people copying demo-shell thinking it's a good idea to use translucency on shells when it will cripple performance. Perhaps create a "demo-greeter" based on examples/basic_server.cpp instead?

5. "if(" should have a space; "if ("

6. Please remove spaces: "mg::red_channel_depth( format )"

7. Half of this diff is not yours. It seems to have come from raof's "surfaceless" branch. Please resubmit this branch listing that one as the prerequisite. Then his work won't appear in your MP diff, and it will be half the size to review.

8. Tests for your new logic (other than pixel format utils); is it just
 tests/unit-tests/graphics/nested/test_nested_display_configuration.cpp
?

This isn't an exhaustive review, but I've written enough for now :)

review: Needs Fixing

« Back to merge proposal