I mostly commented the first patch for the tests...
we need I'm afraid more tests to discriminate whether the scan mask is bound to directory handle or to the directory itself
reading MSDN doc [1] suggests that it's likely the former; but I never trust MSDN doc, hence the need to more tests, esp with two handles opened to the same directory and test the mixing of calls
if it's the case, then it means that tweaking the fd cache is wrong (as you in your second patch), and we likely need another layer for storing the mask
[1] https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-nti…
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_89400
eric pouech (@epo) commented about dlls/ntdll/tests/directory.c:
> + InitializeObjectAttributes(&attr, &ntdirname, OBJ_CASE_INSENSITIVE, 0, NULL);
> +
> + /* Open a handle for our test directory */
> + status = pNtOpenFile(&dirh, SYNCHRONIZE | FILE_LIST_DIRECTORY, &attr, &io, FILE_SHARE_READ,
> + FILE_SYNCHRONOUS_IO_NONALERT | FILE_OPEN_FOR_BACKUP_INTENT | FILE_DIRECTORY_FILE);
> + ok( status == STATUS_SUCCESS, "failed to open dir '%s', ret 0x%lx, error %ld\n", testdir, status, GetLastError() );
> + if (status != STATUS_SUCCESS)
> + {
> + skip("can't test if we can't open the directory\n");
> + goto done;
> + }
> +
> + /* Verify that updated windows 8 and higher behaviour is supported */
> + if (!winetest_platform_is_wine && !test_NtQueryDirectoryFile_mask(dirh, TRUE, atestfile, TRUE, TRUE))
> + run_updated_tests = FALSE;
> + if (!winetest_platform_is_wine && !test_NtQueryDirectoryFile_mask(dirh, TRUE, notatestfile, FALSE, TRUE))
question: do you need both calls to test_NtQueryDirectoryFile_mask to invalidate a platform?
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6904#note_89399
On Sun Dec 1 21:39:49 2024 +0000, Robbert van der Helm wrote:
> Thanks for the detailed write up! I only ran into this commit after
> bisecting the changes between Wine 9.21 and 9.22, so I'm almost
> certainly missing some context!
> The problem I'm facing seems to be that as soon as `SWP_STATECHANGED` is
> set and `window_set_wm_state()` gets called, `data->wm_state_serial`
> gets stuck on a nonzero value since it's set to the X11 connection's
> next sequence number and is then never cleared again. That causes
> `window_update_client_config()` to always bail before it does anything,
> which in turn causes the `NtUserPostMessage( hwnd,
> WM_WINE_WINDOW_STATE_CHANGED, 0, 0 )` called from
> `X11DRV_ConfigureNotify()` to not do anything anymore.
> This doesn't seem to cause any problems under normal use cases, since
> normally once a window is mapped, its coordinates relative to the root
> window will never suddenly change (not sure if there's a spicy
> interaction with multiple monitors though). But in yabridge I have to
> reparent a Wine window to another window, and to make that work I've
> always had to send these `ConfigureNotify` events to inform Wine about
> the window's actual position on screen. Otherwise it thinks it's located
> in the top left corner of the screen, and all mouse events will be at an offset.
> I can open a regression bug report on bugs.winehq.org, but since what
> yabridge is doing is almost certainly not the ended way to interact with
> Wine, I thought I'd check here first.
`wm_state_serial` should be cleared when the window state actually changes and the `WM_STATE` PropertyNotify event is received, calling `window_wm_state_notify` which clears it.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6569#note_89396
This PR updates the behaviour of `NtQueryDirectoryFile`, bringing it in line with current Windows behaviour. The need for this update was discovered when attempting to build the Unreal Engine with MSVC under Wine. In certain cases conditional include statements do not behave as expected, due to MSVC depending on undocumented behaviour of `NtQueryDirectoryFile`.
We ran tests on multiple versions of Windows, and discovered that the behaviour has changed since the original Wine implementation, but the documentation has not. The source code for our test tool, and a set of results can be found [here](https://github.com/TensorWorks/NtQueryDirectoryFile-Test). As of Windows 8, calling `NtQueryDirectoryFile` with a re-used handle, a new mask, and setting the `RestartScan` flag to True, causes the cached results to be erased and a new scan to be performed with the updated mask. Currently, Wine performs as did earlier versions of Windows, where the changed mask is ignored, and the cache is reused. This can cause `NtQueryDirectoryFile` under Wine to falsely report that files exist, when they do not.
This PR corrects this behaviour, invalidating the cache when required. Implementing this exposed further undocumented behaviour of `NtQueryDirectoryFile`, where a search for a non-existent file will return either `STATUS_NO_MORE_FILES` or `STATUS_NO_SUCH_FILE`, depending on whether or not the handle had been previously used regardless of the value of `RestartScan`. This was reflected in a `winetest` which allowed for the response to be either `STATUS_SUCCESS` or `STATUS_NO_MORE_FILES`. This test has been updated to only allow current Windows behaviour, and `NtQueryDirectoryFile` will return either `STATUS_NO_MORE_FILES` or `STATUS_NO_SUCH_FILE` as appropriate.
This patch also adds unit tests for the new behaviour of `NtQueryDirectoryFile`. These tests pass when running `winetest` under Windows, and under Wine with these changes in place, but they will fail under older versions of Wine.
--
v3: ntdll: Test updated NtQueryDirectoryFile behaviour if scan is reset with a new mask
https://gitlab.winehq.org/wine/wine/-/merge_requests/6904
On Sun Dec 1 21:51:09 2024 +0000, Jinoh Kang wrote:
> This looks wrong. Unless I've mistaken, this should be the thread that
> *initiated* the wait (like, the one that created or associated the wait
> completion packet), not `current`.
I'd advise you to test WaitCompletionPacket againat waitable objects that use `get_wait_queue_thread()`, including keyed events, mutexes, and message queue objects (not exposed via Win32 API, only at win32k level).
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6911#note_89385
Jinoh Kang (@iamahuman) commented about server/thread.c:
> return 0;
> }
>
> +int is_obj_signaled( struct object *obj )
> +{
> + struct wait_queue_entry wait_entry;
> + struct thread_wait wait = {0};
> +
> + if (!obj->ops->signaled)
> + return 0;
> +
> + wait.thread = current;
This looks wrong. Unless I've mistaken, this should be the thread that *initiated* the wait (like, the one that created or associated the wait completion packet), not `current`.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6911#note_89384
On Sun Dec 1 20:18:56 2024 +0000, Rémi Bernon wrote:
> Hi, I don't think this checks does what you describe.
> This specific `if` is there for *unmanaged* windows, which are X
> override-redirect windows which are fully under Wine control, and which
> won't receive WM_STATE property changes. We clear the `wm_state_serial`
> for them, in order to allow sending more window state changes without
> waiting, as well as window configure requests. That if doesn't set
> wm_state_serial, it clears it, and it shouldn't prevent the
> `ConfigureNotify` to be processed, rather the other way around.
> For *managed* windows, when Wine requests a window state change, the
> Window Manager is supposed to change the `WM_STATE` property accordingly
> once it is done, and we should then receive the corresponding
> `PropertyNotify` event. We still process `ConfigureNotify` events, but
> we don't apply them to the Win32 state until all the requested changes
> have been processed.
> If the `WM_STATE` property isn't modified or isn't present, then yes the
> Win32 state will appear to be stuck forever. This is however a bug in
> the window manager, according to the ICCCM, and we can really only
> support ICCCM-compatible window manager (or just uncheck "allow the
> window manager to manage the windows" in `winecfg`).
> In any case, `ConfigureNotify` events are always processed, and applied
> to the Win32 state when there's no reason to wait. XEMBED support may be
> what makes a difference here, as we used only for systray embedded
> windows. I don't think any other use case was very intended before or
> well supported, and there's assumptions being made that embedded windows
> are always systray windows
> It's probably possible to support it better, and maybe even easier to do
> so now that we track X parent and embedder windows without leaking them
> into the Win32 space anymore. If there's a regression, please open a bug
> on bugs.winehq.org, describe your use case with reproducible steps, and
> we will try to fix it.
Thanks for the detailed write up! I only ran into this commit after bisecting the changes between Wine 9.21 and 9.22, so I'm almost certainly missing some context!
The problem I'm facing seems to be that as soon as `SWP_STATECHANGED` is set and `window_set_wm_state()` gets called, `data->wm_state_serial` gets stuck on a nonzero value since it's set to the X11 connection's next sequence number and is then never cleared again. That causes `window_update_client_config()` to always bail before it does anything, which in turn causes the `NtUserPostMessage( hwnd, WM_WINE_WINDOW_STATE_CHANGED, 0, 0 )` called from `X11DRV_ConfigureNotify()` to not do anything anymore.
This doesn't seem to cause any problems under normal use cases, since normally once a window is mapped, its coordinates relative to the root window will never suddenly change (not sure if there's a spicy interaction with multiple monitors though). But in yabridge I have to reparent a Wine window to another window, and to make that work I've always had to send these `ConfigureNotify` events to inform Wine about the window's actual position on screen. Otherwise it thinks it's located in the top left corner of the screen, and all mouse events will be at an offset.
I can open a regression bug report on bugs.winehq.org, but since what yabridge is doing is almost certainly not the ended way to interact with Wine, I thought I'd check here first.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6569#note_89383
On Sun Dec 1 18:46:02 2024 +0000, Robbert van der Helm wrote:
> Hi!
> This specific check causes `ConfigureNotify` events to no longer be
> processed by top level windows (i.e. managed) windows. as
> `wm_state_serial` will be set but never cleared again. For the last
> couple years I've been sending `ConfigureNotify` events to a reparented
> window to embed Wine windows into other X11 windows, but this specific
> check breaks that for Wine 9.22. Wine's XEmbed support (which never
> worked quite as well as the reparent+`ConfigureNotify` approach) also
> does not seem to work anymore with recent Wine releases.
> My question is: is this check necessary to prevent other undesirable
> interactions and if so, what's the current best way to send X11 window
> configuration events to Wine windows?
> More context: https://github.com/robbert-vdh/yabridge/issues/382#issuecomment-2510155844
Hi, I don't think this checks does what you describe.
This specific `if` is there for *unmanaged* windows, which are X override-redirect windows which are fully under Wine control, and which won't receive WM_STATE property changes. We clear the `wm_state_serial` for them, in order to allow sending more window state changes without waiting, as well as window configure requests. That if doesn't set wm_state_serial, it clears it, and it shouldn't prevent the `ConfigureNotify` to be processed, rather the other way around.
For *managed* windows, when Wine requests a window state change, the Window Manager is supposed to change the `WM_STATE` property accordingly once it is done, and we should then receive the corresponding `PropertyNotify` event. We still process `ConfigureNotify` events, but we don't apply them to the Win32 state until all the requested changes have been processed.
If the `WM_STATE` property isn't modified or isn't present, then yes the Win32 state will appear to be stuck forever. This is however a bug in the window manager, according to the ICCCM, and we can really only support ICCCM-compatible window manager (or just uncheck "allow the window manager to manage the windows" in `winecfg`).
In any case, `ConfigureNotify` events are always processed, and applied to the Win32 state when there's no reason to wait. XEMBED support may be what makes a difference here, as we used only for systray embedded windows. I don't think any other use case was very intended before or well supported, and there's assumptions being made that embedded windows are always systray windows
It's probably possible to support it better, and maybe even easier to do so now that we track X parent and embedder windows without leaking them into the Win32 space anymore. If there's a regression, please open a bug on bugs.winehq.org, describe your use case with reproducible steps, and we will try to fix it.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6569#note_89382
Robbert van der Helm (@robbert-vdh) commented about dlls/winex11.drv/window.c:
> if (!data->embedded) XIconifyWindow( data->display, data->whole_window, data->vis.screen );
> break;
> }
> +
> + /* override redirect windows won't receive WM_STATE property changes */
> + if (!data->managed) data->wm_state_serial = 0;
Hi!
This specific check causes `ConfigureNotify` events to no longer be processed by top level windows (i.e. managed) windows. as `wm_state_serial` will be set but never cleared again. For the last couple years I've been sending `ConfigureNotify` events to a reparented window to embed Wine windows into other X11 windows, but this specific check breaks that for Wine 9.22. Wine's XEmbed support (which never worked quite as well as the reparent+`ConfigureNotify` approach) also does not seem to work anymore with recent Wine releases.
My question is: is this check necessary to prevent other undesirable interactions and if so, what's the current best way to send X11 window configuration events to Wine windows?
More context: https://github.com/robbert-vdh/yabridge/issues/382#issuecomment-2510155844
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6569#note_89380
The idea is to use common helper to convert between CP_ACP and WCHAR strings first. Later all functions will be switched to support UTF-8 at the same time.
--
v2: msvcrt: Call _wfullpath in _fullpath function.
msvcrt: Call _wgetdcwd in _getdcwd function.
msvcrt: Call _wgetcwd in _getcwd function.
msvcrt: Call _wchdir in _chdir function.
msvcrt: Call _wrmdir in _rmdir function.
msvcrt: Call _wmkdir in _mkdir function.
https://gitlab.winehq.org/wine/wine/-/merge_requests/6937
The idea is to use common helper to convert between CP_ACP and WCHAR strings first. Later all functions will be switched to support UTF-8 at the same time.
--
v3: msvcrt: Call _wfullpath in _fullpath function.
msvcrt: Call _wgetdcwd in _getdcwd function.
msvcrt: Call _wgetcwd in _getcwd function.
https://gitlab.winehq.org/wine/wine/-/merge_requests/6937
Recent changes allowed the Wayland driver to properly deal with numpad keys depending on numlock status. This MR ensures that numlock and other lock state is properly synced, and concludes the fix for https://bugs.winehq.org/show_bug.cgi?id=56397.
This MR also syncs the key press state when a window gains keyboard focus, including any modifier press state. Note that we currently ignore the modifier press information from the `modifier` event, since: 1. it doesn't differentiate between left-right keys, 2. mod press state changes will be normally preceded by a matching key event, so we are able to sync properly. However, if we find that we need to handle mod press state changes without corresponding key events, we will need to implement some sensible way to sync these, too.
Although I would like for the driver work exclusively in terms of scancodes, I still needed to use vkeys in this case, since this is how wineserver expresses keyboard state at the moment. This means that I had to introduce/duplicate some extra scan->vkey logic in the driver (e.g., numpad keys based on numlock state) to get things right.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/5712
This MR adds support for getting a Bluetooth adapter's properties from its corresponding `org.bluez.Adapter1` object, and making them available to userspace via device properties and the `IOCTL_BTH_GET_LOCAL_INFO` ioctl, updating these properties whenever a `PropertiesChanged` signal is received for the adapter.
It also adds code for creating and removing radio PDOs on receiving `InterafacesAdded` and `InterfacesRemoved` signals from BlueZ, respectively.
--
v2: winebth.sys: Implement IOCTL_BTH_GET_LOCAL_INFO.
winebth.sys: Update radio PDO properties on receiving PropertiesChanged for an org.bluez.Adapter1 object.
winebth.sys: Remove the corresponding radio PDO on receiving InterfacesRemoved for a org.bluez.Adapter1 object.
winebth.sys: Create new radio PDOs on receiving InterfacesAdded for objects that implement org.bluez.Adapter1.
winebth.sys: Set radio PDO properties from the device's corresponding org.bluez.Adapter1 object properties.
https://gitlab.winehq.org/wine/wine/-/merge_requests/6936