Merge lp://staging/geis/client-arch into lp://staging/geis

Added a trunk synch to resolve some merge conflicts.

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!

On 10/13/2011 04:53 PM, Chase Douglas wrote:
> 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.

There is a backwards-compatible API change in include/geis/geis.h with
the addition of the GEIS_INIT_UTOUCH_DBUS_BACKEND defined constant.

> 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.

This was apparently clobbered by a last-minute merge from trunk with
manual conflict resolution. I will re-add the back end selection to the
geisview tool and make it command-line configurable, and resubmit.

Changing the default back end to the dbus-client is a little premature
until a proper daemon is set up complete with launch scripts (dbus
service?). This should wait until after the round of plugin
refactoring. Changing the default back-end is a one-line change.

> 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.

The program under tools is a test driver tool. Writing a proper daemon
application with attendant start and stop scripts, logging, and control
options should be a separate task not a part of the internal library
changes designed to enable that task.

All of the changes made under this merge request were made under the
original assumption that the geis server would run as a compiz plugin.
None of them are incompatible with running a standalone server and
neither do they require it.

We should consider making a "utouchd" daemon a separate project from the
utouch-geis library.

> 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).

I will provide better instructions with the above-mentioned changes to
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?

Yes, quite. The only known functional deficit is proper reconnection
from the client when the server has disconnected and then reconnected
again. After verbal discussions we decided to defer this functionality
to later in the interest of getting the client-server merge completed.

> On 10/13/2011 04:53 PM, Chase Douglas wrote:
> > 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.
>
> There is a backwards-compatible API change in include/geis/geis.h with
> the addition of the GEIS_INIT_UTOUCH_DBUS_BACKEND defined constant.

Argh... I see now that although the unmerged revisions list below starts at commit 187, the client-arch commits really start at commit 159. I will need to re-review to include those commits as well.

> > 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.
>
> This was apparently clobbered by a last-minute merge from trunk with
> manual conflict resolution. I will re-add the back end selection to the
> geisview tool and make it command-line configurable, and resubmit.
>
> Changing the default back end to the dbus-client is a little premature
> until a proper daemon is set up complete with launch scripts (dbus
> service?). This should wait until after the round of plugin
> refactoring. Changing the default back-end is a one-line change.

I'm not sure it's premature. The client-arch won't be used unless you have a server running, in which case we can probably assume the user knows what they are doing. The main reason I would like to switch the default is so we can test current clients of geis without having to recompile them. And, it would be switchable merely by running or killing the geis server. I will probably run the tools/ server by default in my installs just so we have some good testing.

> > 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.
>
> The program under tools is a test driver tool. Writing a proper daemon
> application with attendant start and stop scripts, logging, and control
> options should be a separate task not a part of the internal library
> changes designed to enable that task.
>
> All of the changes made under this merge request were made under the
> original assumption that the geis server would run as a compiz plugin.
> None of them are incompatible with running a standalone server and
> neither do they require it.
>
> We should consider making a "utouchd" daemon a separate project from the
> utouch-geis library.

Ok, that sounds reasonable.

> > 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?
>
> Yes, quite. The only known functional deficit is proper reconnection
> fr...

On 10/13/2011 09:03 PM, Chase Douglas wrote:
>
> Argh... I see now that although the unmerged revisions list below starts at commit 187, the client-arch commits really start at commit 159. I will need to re-review to include those commits as well.

Yeah, it was a big set of changes with a number of synchs with trunk
along the way.

> I'm not sure it's premature. The client-arch won't be used unless you have a server running, in which case we can probably assume the user knows what they are doing. The main reason I would like to switch the default is so we can test current clients of geis without having to recompile them. And, it would be switchable merely by running or killing the geis server. I will probably run the tools/ server by default in my installs just so we have some good testing.

So you're suggesting autodiscovery with fallback. OK, I can do that.
Would like that as part of this merge or should we open a bug to track
that work?

Note that clients would not need any kind of recompile regardless.

>> The only known functional deficit is proper reconnection
>> from the client when the server has disconnected and then reconnected
>> again. After verbal discussions we decided to defer this functionality
>> to later in the interest of getting the client-server merge completed.
>
> Ok, that's reasonable for now. If we merge the branch like this, we should open a bug to track this missing functionality.

Absolutely.