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

Improve handling of file paths in the windows DnD handler #980

Merged
merged 4 commits into from
Jun 28, 2019
Merged

Improve handling of file paths in the windows DnD handler #980

merged 4 commits into from
Jun 28, 2019

Conversation

streifmi
Copy link
Contributor

This changes FileDropHandler::iterate_filenames to use the exact length returned by DragQueryFileW instead of relying on MAX_PATH.

I've additionally replaced all calls to mem::uninitialized with mem::zeroed (on windows only).

  • Tested on all platforms changed
  • cargo fmt has been run on this branch

@streifmi streifmi changed the title Improve handling of file paths in the windows DnD hander Improve handling of file paths in the windows DnD handler Jun 25, 2019
@Osspial
Copy link
Contributor

Osspial commented Jun 27, 2019

Hi, and thanks for the PR! Sorry about the delay in looking at this.

Couple points:

  • I'm assuming you're making this because it seems you can have files longer than MAX_PATH. Could you throw a comment in here documenting that reasoning for switching away?
  • Could you add a CHANGELOG entry? This is a meaningful change that will affect Winit's behaviors in some cases, so we should document that the change was made.
  • Why change from uninitialized to zeroed? All of the places I can see it being used get immediately overwritten by the Windows API, so it shouldn't be having any ill effects.

streifmi added 4 commits June 28, 2019 07:19
by replacing the call to mem::uninitialized with mem::zeroed and change
file name retrieval to use buffers of exact length as reported
by DragQueryFileW instead of relying on MAX_PATH.
@streifmi
Copy link
Contributor Author

streifmi commented Jun 28, 2019

Yes, exactly. If you want to test this, you have to first enable long path support on Windows 10 and then use a file manager that supports it (explorer.exe doesn't), like One Commander. If the application you drag the file from doesn't support long paths, it will either use the short path or if that's disabled generate no event at all. Note also that this alone won't help you much, if you then want to e.g. read that file. See this issue, for example.

I did the change from uninitialized to zeroed because uninitialized will be deprecated in favor of MaybeUninit. We could wait for MaybeUninit to reach the stable branch instead, of course.

Added the CHANGELOG entry and the comment, as requested.

@ghost
Copy link

ghost commented Jun 28, 2019

Why change from uninitialized to zeroed? All of the places I can see it being used get immediately overwritten by the Windows API, so it shouldn't be having any ill effects.

The Windows API generally expects structs to be zeroed rather than uninitialized, and this is mentioned in their API documentation. I remember having run into this before because I hadn't zeroed some struct that it expected to be zeroed. We should always use zeroed in this context rather than uninitialized or MaybeUninit.

@Osspial Osspial merged commit 5bf303f into rust-windowing:master Jun 28, 2019
kosyak pushed a commit to kosyak/winit that referenced this pull request Jul 10, 2019
…wing#980)

* Make FileDropHandler::iterate_filenames more robust

by replacing the call to mem::uninitialized with mem::zeroed and change
file name retrieval to use buffers of exact length as reported
by DragQueryFileW instead of relying on MAX_PATH.

* Change remaining calls of uninitialized to zeroed

* Run rustfmt

* Add CHANGELOG entry and comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants