Code review comment for lp:~vanvugt/mir/surface-types

Alan,

"Shell in Display"
I kind of agree. This was the last part I wrote and I later saw one or two alternative places to attach Shell other than inside Display. But from a conceptional standpoint, can we discuss why the "shell" is not a 1:1 part of a "display"?

"#include <mir/surface.h>"
Yes this is correct because it is included from a public system header (shell.h). Hence angle brackets are required.

"__MIR_COMMON_SURFACE_H__"
Yeah I can change the style no problem.

"MIR_SURFACE_TYPE"
This is C code. Not C++. What is the established convention for C-compatible enums that I should change to?

"src/display.cpp"
It's not really a confusing place because it's the same directory where other sources relating to it already reside. If I put display.cpp in a subdir then it has to be built into a static library and then bundled into the server library. Seems like an overkill for one small source file right now.

"using namespace mir"
I prefer this because it makes the code much more readable, and will argue to the very end that the convention seen in existing mir code is ugly, hard to read and more time-consuming to maintain. It is never confusing where a symbol comes from if it is only defined in one place in the project.

"Application logic doesn't belong in ApplicationMediator."
Yeah I had the same feeling. Just haven't yet found a cleaner place to put it where the logic had full access to all the objects in question.

"required vs optional"
I don't care enough to argue. Will change it.

All in all, I think that's a good initial review. Good only because I only disagree deeply with one of the points made.