Re: [3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Nikolay Sivov]

On 23.02.2015 15:42, Joachim Priesner wrote:
> - if (IS_INTRESOURCE(text))
> - {
> -
>    SetLastError(S_OK);
> -
>    LoadStringW(instance, LOWORD(text), (LPWSTR)result, 0);
> -
>    return HRESULT_FROM_WIN32(GetLastError());
> - }

You don't need SetLastError(), especially not with HRESULT value. And 
GetLastError() should only be used when LoadStringW() fails.

The rest looks good enough to me.

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Joachim Priesner]

Am Dienstag, 24. Februar 2015 schrieb Nikolay Sivov:
> On 23.02.2015 15:42, Joachim Priesner wrote:
> > - if (IS_INTRESOURCE(text))
> > - {
> > -
> >    SetLastError(S_OK);
> > -
> >    LoadStringW(instance, LOWORD(text), (LPWSTR)result, 0);
> > -
> >    return HRESULT_FROM_WIN32(GetLastError());
> > - }
>
> You don't need SetLastError(), especially not with HRESULT value. And 
> GetLastError() should only be used when LoadStringW() fails.

Thanks for the feedback, again. The rationale behind SetLastError is that the following three cases can occur:
 1) LoadStringW succeeds, string length > 0 => return value > 0
 2) LoadStringW succeeds, string length == 0 => return value 0
 3) LoadStringW fails => return value 0, error code available via GetLastError.

Because LastError is not modified in case 2), I have to set it to S_OK beforehand in order to distinguish between 2) and 3). Does that make sense?

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Nikolay Sivov]

On 24.02.2015 17:37, Joachim Priesner wrote:
> Am Dienstag, 24. Februar 2015 schrieb Nikolay Sivov:
> > On 23.02.2015 15:42, Joachim Priesner wrote:
> > > - if (IS_INTRESOURCE(text))
> > > - {
> > > -
> > >    SetLastError(S_OK);
> > > -
> > >    LoadStringW(instance, LOWORD(text), (LPWSTR)result, 0);
> > > -
> > >    return HRESULT_FROM_WIN32(GetLastError());
> > > - }
> >
> > You don't need SetLastError(), especially not with HRESULT value. And
> > GetLastError() should only be used when LoadStringW() fails.
>
> Thanks for the feedback, again. The rationale behind SetLastError is that the following three cases can occur:
> - LoadStringW succeeds, string length > 0 => return value > 0
> - LoadStringW succeeds, string length == 0 => return value 0
> - LoadStringW fails => return value 0, error code available via GetLastError.
>
> Because LastError is not modified in case 2), I have to set it to S_OK beforehand in order to distinguish between 2) and 3). Does that make sense?

Ok, what happens on windows if resource length is 0? If it still works 
and text is simply empty you can detect a failure another way - by 
testing returned resource pointer (I'd expect NULL if resource id was 
not found), and in that case use last error.

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Alexandre Julliard]

Nikolay Sivov  writes:
> On 24.02.2015 17:37, Joachim Priesner wrote:
> > Am Dienstag, 24. Februar 2015 schrieb Nikolay Sivov:
> > > On 23.02.2015 15:42, Joachim Priesner wrote:
> > > > - if (IS_INTRESOURCE(text))
> > > > - {
> > > > -
> > > >    SetLastError(S_OK);
> > > > -
> > > >    LoadStringW(instance, LOWORD(text), (LPWSTR)result, 0);
> > > > -
> > > >    return HRESULT_FROM_WIN32(GetLastError());
> > > > - }
> > >
> > > You don't need SetLastError(), especially not with HRESULT value. And
> > > GetLastError() should only be used when LoadStringW() fails.
> >
> > Thanks for the feedback, again. The rationale behind SetLastError is that the following three cases can occur:
> > - LoadStringW succeeds, string length > 0 => return value > 0
> > - LoadStringW succeeds, string length == 0 => return value 0
> > - LoadStringW fails => return value 0, error code available via GetLastError.
> >
> > Because LastError is not modified in case 2), I have to set it to S_OK beforehand in order to distinguish between 2) and 3). Does that make sense?
>
> Ok, what happens on windows if resource length is 0? If it still works
> and text is simply empty you can detect a failure another way - by
> testing returned resource pointer (I'd expect NULL if resource id was
> not found), and in that case use last error.

There shouldn't be any need to distinguish. If you get 0 then there's no
defined string and you use the fallback.

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Joachim Priesner]

Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
> Nikolay Sivov  writes:
> > On 24.02.2015 17:37, Joachim Priesner wrote:
> > > Am Dienstag, 24. Februar 2015 schrieb Nikolay Sivov:
> > > > On 23.02.2015 15:42, Joachim Priesner wrote:
> > > > > - if (IS_INTRESOURCE(text))
> > > > > - {
> > > > > -
> > > > >    SetLastError(S_OK);
> > > > > -
> > > > >    LoadStringW(instance, LOWORD(text), (LPWSTR)result, 0);
> > > > > -
> > > > >    return HRESULT_FROM_WIN32(GetLastError());
> > > > > - }
> > > >
> > > > You don't need SetLastError(), especially not with HRESULT value. And
> > > > GetLastError() should only be used when LoadStringW() fails.
> > >
> > > Thanks for the feedback, again. The rationale behind SetLastError is that the following three cases can occur:
> > > - LoadStringW succeeds, string length > 0 => return value > 0
> > > - LoadStringW succeeds, string length == 0 => return value 0
> > > - LoadStringW fails => return value 0, error code available via GetLastError.
> > >
> > > Because LastError is not modified in case 2), I have to set it to S_OK beforehand in order to distinguish between 2) and 3). Does that make sense?
> >
> > Ok, what happens on windows if resource length is 0? If it still works
> > and text is simply empty you can detect a failure another way - by
> > testing returned resource pointer (I'd expect NULL if resource id was
> > not found), and in that case use last error.
>
> There shouldn't be any need to distinguish. If you get 0 then there's no
> defined string and you use the fallback.

Windows makes that distinction though:
 - String of length zero: TaskDialogIndirect succeeds, but no text is displayed for that parameter
 - Invalid resource: TaskDialogIndirect fails with ERROR_RESOURCE_NAME_NOT_FOUND

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Alexandre Julliard]

Joachim Priesner  writes:
> Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
> > There shouldn't be any need to distinguish. If you get 0 then there's no
> > defined string and you use the fallback.
>
> Windows makes that distinction though:
> - String of length zero: TaskDialogIndirect succeeds, but no text is displayed for that parameter
> - Invalid resource: TaskDialogIndirect fails with ERROR_RESOURCE_NAME_NOT_FOUND

This is broken behavior, I don't see much point in reproducing this
(unless of course you can find an app that depends on it).

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Joachim Priesner]

Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
> Joachim Priesner  writes:
> > Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
> > > There shouldn't be any need to distinguish. If you get 0 then there's no
> > > defined string and you use the fallback.
> >
> > Windows makes that distinction though:
> > - String of length zero: TaskDialogIndirect succeeds, but no text is displayed for that parameter
> > - Invalid resource: TaskDialogIndirect fails with ERROR_RESOURCE_NAME_NOT_FOUND
>
> This is broken behavior, I don't see much point in reproducing this
> (unless of course you can find an app that depends on it).

I'd argue that a) the goal of "bug-for-bug" compatibility is really easy to achieve in this case and b) it saves debugging time in the long run should there be an application that does rely on it (even if only accidentally).

But I'm fine with just ignoring invalid string resources (instead of returning failure), which should be the "safe" variant.

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Alexandre Julliard]

Joachim Priesner  writes:
> Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
> > Joachim Priesner  writes:
> > > Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
> > > > There shouldn't be any need to distinguish. If you get 0 then there's no
> > > > defined string and you use the fallback.
> > >
> > > Windows makes that distinction though:
> > > - String of length zero: TaskDialogIndirect succeeds, but no text is displayed for that parameter
> > > - Invalid resource: TaskDialogIndirect fails with ERROR_RESOURCE_NAME_NOT_FOUND
> >
> > This is broken behavior, I don't see much point in reproducing this
> > (unless of course you can find an app that depends on it).
>
> I'd argue that a) the goal of "bug-for-bug" compatibility is really
> easy to achieve in this case and b) it saves debugging time in the
> long run should there be an application that does rely on it (even if
> only accidentally).

OTOH if an app depends on some missing string (say a resource in a
builtin dll), seeing empty text will be a lot easier to figure out than
if you don't get the dialog at all.

[3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)

[Joachim Priesner]

> Ok, what happens on windows if resource length is 0? If it still works 
> and text is simply empty

Yes, this is the case.
> you can detect a failure another way - by 
> testing returned resource pointer (I'd expect NULL if resource id was 
> not found), and in that case use last error.

No, if the resource is not found, lpBuffer remains unchanged on Windows and isn't set to NULL.

I could of course set it to NULL beforehand instead of setting SetLastError. But the current solution uses less code than that. (Except of course I should use "SetLastError(ERROR_SUCCESS)" instead of S_OK, as you noted.) Which one do you prefer?