Re: [PATCH 2/7] oleacc: Add get_state function for edit client accessible object.

[Connor McAdams]

On Wed, Sep 22, 2021 at 07:19:44PM +0200, Piotr Caban wrote:
> Hi Connor,
>
> On 9/22/21 6:20 PM, Connor McAdams wrote:
> > +typedef struct {
> > - HRESULT (*get_state)(IAccessible *, VARIANT, VARIANT *);
> >
> > +} WinClassVtbl;
>
> I think it's better to pass Client* as first argument. Please also avoid
> camel-case in new code.
> > -static HRESULT WINAPI Client_get_accState(IAccessible *iface, VARIANT varID, VARIANT *pvarState)
> > +static void default_client_get_state(IAccessible *iface, VARIANT *var_state)
> >   {
> >       Client *This = impl_from_Client(iface);
> >       GUITHREADINFO info;
> >       LONG style;
> > - TRACE("(%p)->(%s %p)\n", This, debugstr_variant(&varID), pvarState);
> > -
> > - if(convert_child_id(&varID) != CHILDID_SELF) {
> > -
> >    V_VT(pvarState) = VT_EMPTY;
> > -
> >    return E_INVALIDARG;
> > - }
> > -
> > - V_VT(pvarState) = VT_I4;
> > - V_I4(pvarState) = 0;
> > - V_VT(var_state) = VT_I4;
> > - V_I4(var_state) = 0;
> >  style = GetWindowLongW(This->hwnd, GWL_STYLE);
> >  if(style & WS_DISABLED)
> > -
> >    V_I4(pvarState) |= STATE_SYSTEM_UNAVAILABLE;
> > -
> >    V_I4(var_state) |= STATE_SYSTEM_UNAVAILABLE;
> > else if(IsWindow(This->hwnd))
> > -
> >    V_I4(pvarState) |= STATE_SYSTEM_FOCUSABLE;
> > -
> >    V_I4(var_state) |= STATE_SYSTEM_FOCUSABLE;
> > info.cbSize = sizeof(info);
> > if(GetGUIThreadInfo(0, &info) && info.hwndFocus == This->hwnd)
> > -
> >    V_I4(pvarState) |= STATE_SYSTEM_FOCUSED;
> > -
> >    V_I4(var_state) |= STATE_SYSTEM_FOCUSED;
> > if(!(style & WS_VISIBLE))
> > -
> >    V_I4(pvarState) |= STATE_SYSTEM_INVISIBLE;
> > -
> >    V_I4(var_state) |= STATE_SYSTEM_INVISIBLE;
> >
> > +}
> > -
> >
> > +static HRESULT WINAPI Client_get_accState(IAccessible *iface, VARIANT varID, VARIANT *pvarState)
> > +{
> > - Client *This = impl_from_Client(iface);
> > -
> > - TRACE("(%p)->(%s %p)\n", This, debugstr_variant(&varID), pvarState);
> > -
> > - if (This->vtbl.get_state)
> > -
> >    return This->vtbl.get_state(iface, varID, pvarState);
> > -
> > - if(convert_child_id(&varID) != CHILDID_SELF) {
> > -
> >    V_VT(pvarState) = VT_EMPTY;
> > -
> >    return E_INVALIDARG;
> > - }
> > -
> > - default_client_get_state(iface, pvarState);
> > -
> > return S_OK;
> > }
>
> Isn't it enough to call vtbl.get_state near the end of the function? I'm
> asking about something along these lines (I've also changed get_state
> header):
> static HRESULT WINAPI Client_get_accState(IAccessible *iface, VARIANT varID,
> VARIANT *pvarState)
> {
> ...
>     info.cbSize = sizeof(info);
>     if(GetGUIThreadInfo(0, &info) && info.hwndFocus == This->hwnd)
>         V_I4(pvarState) |= STATE_SYSTEM_FOCUSED;
>     if(!(style & WS_VISIBLE))
>         V_I4(pvarState) |= STATE_SYSTEM_INVISIBLE;
> if(This->vtbl.get_state)
>     return This->vtbl.get_state(This, &V_I4(pvarState))
> return S_OK;
>
> }

Oh, problem with this:
Not all classes are restricted to having a varID of CHILDID_SELF for
their get_accState implementation. I.e, listbox controls have individual
state values for each item in the list. Not having the vtbl function
called from the start would break this.