Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple file selection in ui_choose_file #559

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nspitko
Copy link
Contributor

@nspitko nspitko commented Aug 31, 2022

This is one of two PRs (The other in haxe for ui.hx) to add support for multiple selection in hl.UI.chooseFile. The haxe side PR can be found here: HaxeFoundation/haxe#10788

Both do weird things for dumb windows API reasons, so I'll explain my sins in advance. Documentation for this API can be found here: https://docs.microsoft.com/en-us/windows/win32/api/commdlg/ns-commdlg-openfilenamea (We use the widechar version, but otherwise the call pattern is identical).

To enable multi select, we really only need to set a flag. Since we just pass a simple Dynamic across the fence for this, I just key off a new field and set the related flag. Note we also have to set OFN_EXPLORER else you get the win 3.x era version of this dialogue... which is magical in its own right.

The elephant in the room though is the larger buffer, and the fact that we return the entire thing. The buffer size increase is because this API doesn't really have any mechanism for letting you ask for how many bytes you need; hence we should always oversize. And the size chosen is simply due to that being the largest buffer you can even send this API. We'll need to use a newer API if we want anything more sophisticated.

The reason we pass the whole buffer, instead of the old behavior of just copying the length, is because the return format is null terminated. So wcslen will only ever return the path. The haxe commit has the untangling code for converting this data into haxe strings. This code isn't performance critical, so the extra copy shouldn't be a huge deal, and String.fromUCS2 doesn't rely on buffer size.

@ncannasse
Copy link
Member

Hi,
Looks fine, the only issue I have is that the windows specific implementation (in particular the 2048 buffer size) is visible in the Haxe code (idx < 2048). If we can get rid of that so that another platform API can return a valid buffer with no 2048 size limit I think we're good and it can be merged.
Best,
Nicolas

@skial skial mentioned this pull request Aug 31, 2022
1 task
Fixes:
- Revert buffer size increase for fileStr
- Revert memcpy change; the old behavior was fine.
- Revert increase of op.nMaxFile. The old value is correct NOW, but was a crash under the old buffer sizing!
@nspitko
Copy link
Contributor Author

nspitko commented Sep 1, 2022

Pushed an update to both PRs.

That line was originally just a safety valve during testing, but removing and testing revealed a rather exciting bug: nMaxFile in the windows API is a character limit, not a buffer size! Since we're using the W version, the old value was actually a potential crash, and we just never hit it because of the windows path limit.

I fixed this and reverted some other unnecessary changes I made in the process. Buffer sizing is now accounting for wchar size, and appears to behave correctly when fed too many files (returns null)

@ncannasse
Copy link
Member

The final hl_copy_bytes value seems incorrect. maybe should be sizeof(outputFile) ?

@nspitko
Copy link
Contributor Author

nspitko commented Sep 10, 2022

Yep, that's clearly a bug. Fixed.

@MSGhero
Copy link

MSGhero commented Mar 4, 2023

I wonder if this helps the File.browse() issue I ran into HeapsIO/heaps#783

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants