Code review comment for lp://staging/geis/client-arch

Revision history for this message
Chase Douglas (chasedouglas) wrote :

A ton of code that looks very clean. I really like the changes from an aesthetic point of view. It's obvious that you put a lot of thought into how things should be structured!

Here's what I'm looking for in reviewing this code:

1. Are there any "black box" changes?
2. Does the code look clean? Does it appear to do what is stated in the merge request?
3. Does it work?
4. Are there any issues highlighted by the merge proposal description?

Note that I'm not doing a line by line review. There's too much code to review for that level of detail, and you're a trusted member of the development team.

For item 1, there are no changes in include/. Since there are no api changes, and it's assumed there are no abi changes, this criteria is fulfilled.

For item 2, the code is definitely clean. It does appear to do what is stated in the merge request in whole.

However, I don't see any change in the default backend choice, and I don't see any change in geisview that would cause it to use the new client-server backend. Am I missing something, or was this lost in a merge or rebase? I would prefer to see the default backend change to the client-server backend with a fallback to the xcb backend.

The geis server is under tools/. Is this implementation merely an example, or is it the defacto server that should be used? If it's the latter, I would prefer to move it to the top level. I feel putting it in tools/ gives the wrong impression that it is not an essential part of the project.

For item 3, I'm not ready to give an answer yet since I don't know that geisview is using the new architecture. (On IRC, Stephen explained how to test: tools/geis-server/geis-server and then tools/geisview/geisview).

For item 4, I would like to know what is left out by the statement in the merge proposal: "I think at this point (most) functionality is there." Please detail which functionality is missing?

Overall, I'm extremely happy with the changes, assuming the testing shows it works :). I look forward to merging the branch once these issues are resolved!

review: Needs Information

« Back to merge proposal