Mir

Code review comment for lp:~robertcarr/mir/demo-shell

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

1. Tab key only works as root. Is that expected?

2. Wrong licenses:
    examples/demo-shell/application_switcher.*
    examples/demo-shell/demo_shell_placement_strategy.*

3. Lots of returns in bool me::ApplicationSwitcher::handles. I think it would be easier to understand even as a large sequence of A && B && C &&...

4. Missing const. But is that already a pre-existing problem?
    132 + bool handles(MirEvent const& event);

5. Holistically I think we have a potential problem with size and complexity. I mean, how small can you make a minimal shell? If a minimal shell requires this much code then we've made some design mistakes. The two features it has should not require hundreds of lines of code. Again, this is not a new problem introduced by this proposal but it is now most visible. I propose that we limit ourselves to one demo shell for some time while we figure out how to reduce the effort required on the shell side. It would be a bad thing if we imposed this kind of complexity on every new shell. Even if there is only one production shell.

review: Needs Fixing

« Back to merge proposal