Re: [3/5] comctl32: Implement TaskDialogIndirect as a custom dialog box (try 2)
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.
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?
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: 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?
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.
Nikolay Sivov <bunglehead(a)gmail.com> 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: 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?
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. -- Alexandre Julliard julliard(a)winehq.org
Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
Nikolay Sivov <bunglehead(a)gmail.com> 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: 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?
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
Joachim Priesner <joachim.priesner(a)web.de> 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). -- Alexandre Julliard julliard(a)winehq.org
Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
Joachim Priesner <joachim.priesner(a)web.de> 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.
Joachim Priesner <joachim.priesner(a)web.de> writes:
Am Dienstag, 24. Februar 2015 schrieb Alexandre Julliard:
Joachim Priesner <joachim.priesner(a)web.de> 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. -- Alexandre Julliard julliard(a)winehq.org
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?
participants (3)
-
Alexandre Julliard -
Joachim Priesner -
Nikolay Sivov