globally it looks better
a couple of stylistic remarks, but you need to go further for not using errorlevel inside the function and its helper
perhaps, it's worthwhile to split the patch in two:
* first one to no longer use error level internally, adapt list_directory prototype... (in the details, it's fine to use return errorlevel = ERROR\_\* while parsing the arguments in case of error, but when this is done all the rest of the code should use only the return_code (and set errorlevel when exiting WCMD_dir))
* the second one to plug in the ctrl-c handling and breaking from the various loops
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94825
eric pouech (@epo) commented about programs/cmd/directory.c:
> * FIXME: Assumes 24-line display for the /P qualifier.
> */
>
> -static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int level) {
> +static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int level, RETURN_CODE *return_code) {
nitpick: IMO it's more readable to return a RETURN_CODE, and pass a DIRECTORY_STACK\*\* to update the pointer (you likely did it this way to minimize change, but let's take the opportunity of a change to sanitize a bit the current spaghetti code)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94822
eric pouech (@epo) commented about programs/cmd/directory.c:
> thisDir = tempDir;
> }
> }
> + /* Free remaining structures if we broke out of previous loop due to error. */
> + while (dirStack != NULL) {
this likely could be folded in code line 510, by inserting between 510 & 511:
if (return_code != NO_ERROR) dirStack = NULL;
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94821
eric pouech (@epo) commented about programs/cmd/directory.c:
> }
>
> /* Trailer Information */
> - if (trailerReqd) {
> + if (trailerReqd && return_code == NO_ERROR) {
> WCMD_dir_trailer(prevEntry->dirName);
> }
>
> - if (num_empty && !num_with_data)
> + if (return_code == STATUS_CONTROL_C_EXIT)
actually, we could (later on) have other values as return code...
so IMO this should rather be written as:
`if (return_code != NO_ERROR || (num_empty && !num_with_data))`
` errorlevel = ERROR_INVALID_FUNCTION;`
`else`
` errorlevel = NO_ERROR;`
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94820
eric pouech (@epo) commented about programs/cmd/directory.c:
> /* Clear any errors from previous invocations, and process it */
> errorlevel = NO_ERROR;
> prevEntry = thisEntry;
> - thisEntry = WCMD_list_directory (thisEntry, 0);
> + thisEntry = WCMD_list_directory (thisEntry, 0, &return_code);
> if (errorlevel)
this should use return_code not error level
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94819
eric pouech (@epo) commented about programs/cmd/directory.c:
> if ((file_total + dir_total == 0) && (level == 0)) {
> SetLastError (ERROR_FILE_NOT_FOUND);
> WCMD_print_error ();
> errorlevel = ERROR_INVALID_FUNCTION;
this shouldn't set errorlevel but return_code
and likely using ERROR_FILE_NOT_FOUND to mark that listing was empty
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94818
eric pouech (@epo) commented about programs/cmd/directory.c:
> WCHAR fname[MAX_PATH];
> WCHAR ext[MAX_PATH];
> unsigned num_empty = 0, num_with_data = 0;
> + RETURN_CODE return_code = NO_ERROR;
>
> errorlevel = NO_ERROR;
this should not be necessary (in fact starting from here up the exit of the function, you should only deal with return_code... errorlevel should be set upon exiting)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7322#note_94817
On Sun Feb 16 07:21:41 2025 +0000, Brian Wright wrote:
> Bah. There were too many changes to X11DRV_ConfigureNotify for printf
> debugging, and I wouldn't know what to change that also keeps other
> programs from breaking.
> I can provide detailed logs for 9.21 (or any interim versions), but
> that's about all I can do
For the record, all of these `ConfigureNotify` events were a hack to let Wine know where on screen the window is, which happened to work way better than it should and it resulted in more more reliable behavior than Wine's built in XEmbed support at the time. Ideally, yabridge would just use XEmbed instead and Wine would keep track of the screen coordinates.
If you enable yabridge's XEmbed support then it stops sending these `ConfigureNotify` messages and it will use XEmbed instead. But I also have not yet been able to get that to work as expected with the latest Wine version. I spent a couple hours debugging Wine's X11 driver last Friday but I haven't figured out what goes wrong or what's missing yet.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6569#note_94810
Make both method calls;
1. Consistently written with parameter names as per spec.
2. Fix parameter validation to be consistent with spec.
3. Fix szNameBuf parameter semantics as per spec.
4. Fix szNameBuf casing str search as per spec.
5. Factor out common code into TLB_ helpers vastly improves readability.
--
v25: dlls/oleaut32: Replace infinite loops with for loops
dlls/oleaut32: Use consistent SegDir field names
This merge request has too many patches to be relayed via email.
Please visit the URL below to see the contents of the merge request.
https://gitlab.winehq.org/wine/wine/-/merge_requests/7286