Merge lp://staging/~dandrader/frame/smarter_backend into lp://staging/frame

* The Window class no longer needs to subclass std::enable_shared_from_this<Window>.

* Any functions that may fail need to return a UFStatus and provide the return result through a referenced parameter. All of the new functions here should do this. I forgot about this when I approved the previous merge proposal as well. We should go back and fix these up.

* There's a stray newline at the end of UFFrame::GetSharedTouchById.

Everything else looks good :). This should really help backend implementations get things right the first time.

> * Any functions that may fail need to return a UFStatus and provide the return
> result through a referenced parameter. All of the new functions here should do
> this. I forgot about this when I approved the previous merge proposal as well.
> We should go back and fix these up.

Isn't that extra status variable redundant in those cases? What's the benefit?

On 08/30/2012 11:01 AM, Daniel d'Andrada wrote:
>> * Any functions that may fail need to return a UFStatus and provide the return
>> result through a referenced parameter. All of the new functions here should do
>> this. I forgot about this when I approved the previous merge proposal as well.
>> We should go back and fix these up.
>
> Isn't that extra status variable redundant in those cases? What's the benefit?

Let's take a function like:

UFBackendTouch frame_backend_frame_get_touch_by_id(
     UFBackendFrame frame, UFTouchId id)

Without knowing exactly how this is implemented, there are two possible
recoverable error conditions:

1. The touch ID is invalid (UFStatusErrorInvalidTouch)
2. The allocation and initialization of UFBackendTouch could fail
(UFStatusErrorResources or UFStatusErrorGeneric)

The current implementation doesn't actually catch allocation and
initialization exceptions, but it could if we found that to be a problem.

The client could use this information to determine how to proceed. It's
almost impossible to recover from allocation errors, so the client could
abort. However, if the touch ID is invalid, it could stop processing
gestures for that frame and continue on. At the very least, it could log
a more meaningful error message, which has been helpful in debugging
issues in the past.

The other main reason is that it maintains the cohesiveness of the API.
The entire grail and frame API behaves like this. It makes sense to
continue using the same mechanism.

I'm not a huge fan of the API, tbh. I much prefer an exception based
approach. However, this is C, and there's only so much you can do with a
C API. There is one benefit to the API, however, and it becomes when you
actively check the return value.

UFBackendTouch be_touch;
assert(frame_backend_frame_get_touch_by_id(frame, id, &be_touch) ==
        UFStatusSuccess);

As opposed to:

UFBackendTouch be_touch;
assert((be_touch = frame_backend_frame_get_touch_by_id(frame, id)) !=
        nullptr);

My personal opinion is that the second approach is messy because you are
both assigning a value and using C's () operator to check the assigned
value is correct, all in one statement of code. You could split it up,
but then you're using more lines of code and it isn't any more
comprehensible versus the original approach, imo.

> On 08/30/2012 11:01 AM, Daniel d'Andrada wrote:
> >> * Any functions that may fail need to return a UFStatus and provide the
> return
> >> result through a referenced parameter. All of the new functions here should
> do
> >> this. I forgot about this when I approved the previous merge proposal as
> well.
> >> We should go back and fix these up.
> >
> > Isn't that extra status variable redundant in those cases? What's the
> benefit?
>
> Let's take a function like:
>
> UFBackendTouch frame_backend_frame_get_touch_by_id(
> UFBackendFrame frame, UFTouchId id)
>
> Without knowing exactly how this is implemented, there are two possible
> recoverable error conditions:
>
> 1. The touch ID is invalid (UFStatusErrorInvalidTouch)
> 2. The allocation and initialization of UFBackendTouch could fail
> (UFStatusErrorResources or UFStatusErrorGeneric)
>
> The current implementation doesn't actually catch allocation and
> initialization exceptions, but it could if we found that to be a problem.
>
> The client could use this information to determine how to proceed. It's
> almost impossible to recover from allocation errors, so the client could
> abort. However, if the touch ID is invalid, it could stop processing
> gestures for that frame and continue on. At the very least, it could log
> a more meaningful error message, which has been helpful in debugging
> issues in the past.

You're right.
Thanks for the explanation!

Updated

* In frame_backend_frame_borrow_touch_by_id(), if it fails because the touch ID doesn't exist, the touch pointer is reset to NULL. This should be nullptr instead of NULL, but I would argue it shouldn't be reset at all. One technique often employed by APIs is that either a function succeeds, or it fails and does not modify any state. It can be hard or practically impossible to do this in some circumstances, but here it is as simple as not resetting the touch pointer value. I can't give you any specific scenario under which this particular change would be helpful, but I prefer the API to behave like this as much as possible.

* In frame_backend_frame_give_touch(), I think *touch should be set to nullptr instead of 0. Or maybe I'm not realizing something that makes that not work?

Otherwise, I'm ok with everything. It's not exactly how I would do it, but I don't know of a 100% best alternative either.