Ciantic / VirtualDesktopAccessor

DLL for accessing Windows 11/10 Virtual Desktop features from e.g. AutoHotkey
MIT License
772 stars 93 forks source link

Improved code for supporting multiple Windows versions #93

Open Lej77 opened 7 months ago

Lej77 commented 7 months ago

Hi again, I read your comment in my previous PR #92 and supporting multiple Windows versions definitively complicates the code so it makes sense to limit the scope of the project.

Anyway, I made some improvements to the code for supporting multiple Windows versions and I guess you can merge them into the same branch as the other Windows 10 support:

DLL Size

I checked the size of the DLL (built in release mode) with and without the multiple-windows-versions feature:

Tests

I also ran all the tests and they did pass on Windows 10 (well, except for the ones using features that weren't supported):

running 12 tests
test tests::test_desktop_count ... ok
test tests::test_desktop_get ... ok
test tests::test_kill_explorer_exe_manually ... ok
test tests::test_listener_manual ... ok
test tests::test_desktop_moves ... ok
test tests::test_errors ... ok
test tests::test_move_notepad_between_desktops ... ok
test tests::test_switch_desktops_rapidly_manual ... ok
test tests::test_pin_notepad ... ok
test tests::test_pin_notepad_app ... ok
test tests::test_rename_desktop ... FAILED
test tests::test_threads ... FAILED
eythaann commented 2 months ago

@Lej77 Hi, how do you get the IIDs and know what was changed for each windows version?

Lej77 commented 2 months ago

@eythaann Unfortunately I didn't figure that out myself. I just used the same IIDs as Slion/VirtualDesktop: C# wrapper for the Virtual Desktop API on Windows 11.. I also looked at some earlier commtis in this repository. I link to some of these places in the docs for interfaces_multi.rs.

Under the Windows version support heading of the Slion/VirtualDesktop repo it is mentioned that you can use regedit to find the IIDs of the current Windows installation. So that could be worth trying but I don't know how to discover what functions exist on COM objects.

eythaann commented 2 months ago

oh I understand, I though that we can made the exposed api functions returns and error Err(UnsuportedWindowsVersion), actually the app just panics when is trying to get the unvalid COM object. I do a workaround just creating a helper function to check is I can use winvd, but return a Result::Err will be a easy way to handle error for all the Clients of this crate!.

edit: Also this won't be a breaking change, because the functions already return a Result enum.

image

Lej77 commented 2 months ago

I guess you want to check if the hardcoded IID matches the actually IID for the COM interfaces and return your Err(UnsuportedWindowsVersion) if that isn't the case? That could help but we might not need to know the real IID to do this since if the IID is incorrect then the query_service call will likely fail (since the IID is unlikely to be used by another interface). One issue with this is that it still would not detect a version mismatch where the IID was the same but the interface definition changed.

This PR does have code to detect the current Windows version and that might be useful to return an error when a too old Windows version is used. We can't really return errors for too new versions since we don't know in advance when the interfaces will change.

eythaann commented 2 months ago

oh sure, I'm sure that windows-rs crate have a way to test if a IID is valid as a COM object before create it. so it could be the solution, In my case I'm using this crate for a module on my last project Seelen-UI and some users report crash on last 24h2. I for now I just will disable the Virtual desktop module for 24h2 and later I will try to do some changes in my fork to later be added to this PR, mostly to avoid cases where the program may crash without allowing the client to handle the error.

Ciantic commented 2 months ago

FWIW, I know this doesn't sound helpful, but to me ideal multiple-version support is different DLLs.

On your application code look Windows version and LoadLibrary older DLL.

Windows 11 has already had 6 backward incompatible IIDs, and they are not just IIDs, they change the interfaces. Managing multiple old interfaces in one code base is quite difficult.

Ideally, I would keep only the latest code in the codebase and have old DLLs available.

Lej77 commented 2 months ago

@Ciantic I can understand that perspective but I disagree that the tradeoff isn't worth it. This is your library of course so I will just continue to maintain a fork with my changes. There are many advantages to supporting multiple versions in the same library:

The only disadvantage to supporting multiple versions is that it complicates the library code. That is quite a large issue but I think it is worth it, at least for me. It is quite self contained so it is really only the interfaces.rs file that needs changes, the rest of the library code can remain virtually unchanged.