LIJI32 / SameBoy

Game Boy and Game Boy Color emulator written in C
https://sameboy.github.io/
Other
1.58k stars 206 forks source link

Add support for the newer Vista style select folder dialog #600

Closed CasualPokePlayer closed 3 months ago

CasualPokePlayer commented 3 months ago

Also does minor cleanups with the old dialog handling.

whindsaks commented 3 months ago

OleInitialize is a superset of CoInitialize, just changing that in other code without specifying why is a red flag. Drag&Drop and certain other things require OleInitialize. Not sure it matters here but please be sure you know what you are doing and why!

CasualPokePlayer commented 3 months ago

It is required for OLE drag-n-drop, which is different than regular drag-n-drop. https://learn.microsoft.com/en-us/cpp/mfc/drag-and-drop-ole?view=msvc-170

The older file dialog doesn't rely on OLE features in the first place. It's documented that the "new dialog style" requires apartment threading, and CoInitialize or OleInitialize may be used to init that. The wording more imply "if you happened to OleInitalize first in the application you already have apartment threaded enabled" rather than "OleInitalize should be used for this dialog" (considering drag-n-drop works fine with the older dialog). The docs do "recommend" OleInitalize if you require drag-n-drop functionality, although since drag-n-drop works regardless for the older dialog, that seems more for using other parts of the application using the OLE drag-n-drop (SameBoy doesn't bother with that / the init is scoped anyways).

LIJI32 commented 3 months ago

Which OS versions does do_legacy_open_folder_dialog target? If it's for Windows XP, you can remove the fallback entirely, as recent versions of SameBoy require Windows 7 or newer.

CasualPokePlayer commented 3 months ago

The legacy open folder dialog is required for Windows XP and below, and Windows Server "Core" (gui-less version, gui versions have the newer dialog)

LIJI32 commented 3 months ago

Then I'd rather have the "legacy" fallback removed, since these versions of Windows are no longer supported by SameBoy.

LIJI32 commented 3 months ago

Weirdly enough this won't compile on my setup. It still needs the shlobj.h include this file had before, but even after adding it back I'm getting compilation errors:

OpenDialog/windows.c:54:68: error: implicit declaration of function 'IID_PPV_ARGS' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    hr = CoCreateInstance(&CLSID_FileOpenDialog, NULL, CLSCTX_ALL, IID_PPV_ARGS(&dialog));
                                                                   ^
OpenDialog/windows.c:54:89: error: too few arguments to function call, expected 5, have 4
    hr = CoCreateInstance(&CLSID_FileOpenDialog, NULL, CLSCTX_ALL, IID_PPV_ARGS(&dialog));
         ~~~~~~~~~~~~~~~~     
LIJI32 commented 3 months ago

Manually expanding the IID_PPV_ARGS macro (which, despite MSVC's claims, doesn't seem to be defined in combaseapi.h) results in this error:

OpenDialog/windows.c:55:68: error: cannot call operator __uuidof on a type with no GUID
    hr = CoCreateInstance(&CLSID_FileOpenDialog, NULL, CLSCTX_ALL, __uuidof(IFileOpenDialog), (void *)&dialog);

It might be better to hardcode the GUID (not unlike the XAudio driver), since Microsoft's GUID magic macros never seem to work.