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

[SHELL32] Show "size on disk" in file/folder properties #3107

Merged
merged 39 commits into from Sep 10, 2020
Merged

[SHELL32] Show "size on disk" in file/folder properties #3107

merged 39 commits into from Sep 10, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 28, 2020

Purpose

  • Current design of ReactOS shows blank "size on disk" field in file & folders properties dialog from Explorer
  • Current design does not take credit of C++ functions for String management (using CString class)
  • Current design use MAX_PATH sized buffers for strings not related to file path.

JIRA issue: CORE-12559

image

Proposed changes

image

dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
@binarymaster binarymaster added the enhancement For PRs with an enhancement/new feature. label Aug 28, 2020
@binarymaster
Copy link
Member

This code has been proposed by Amber in comments of the Jira Ticket

Cc @amber8706 🙂

@ghost
Copy link
Author

ghost commented Aug 28, 2020

Ready to go ?

@ghost ghost requested review from binarymaster, katahiromz and HBelusca August 28, 2020 14:28
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
Kyle Katarn and others added 2 commits August 28, 2020 17:24
Co-authored-by: Katayama Hirofumi MZ <katayama.hirofumi.mz@gmail.com>
Co-authored-by: Katayama Hirofumi MZ <katayama.hirofumi.mz@gmail.com>
@Extravert-ir
Copy link
Member

This is a wrong approach to get "size on disk" information about a file.
"Size on disk" is what filesystem driver is responsible for and only it can reliably tell you how many bytes are reserved on disk for this file (for example, small files can be combined into one sector, files can be compressed, etc)

The right way is to use GetFileInformationByHandleEx API for that
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex

@ghost
Copy link
Author

ghost commented Aug 28, 2020

This is a wrong approach to get "size on disk" information about a file.
"Size on disk" is what filesystem driver is responsible for and only it can reliably tell you how many bytes are reserved on disk for this file (for example, small files can be combined into one sector, files can be compressed, etc)

The right way is to use GetFileInformationByHandleEx API for that
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getfileinformationbyhandleex

Yes but the part of API we'd need is Vista+ only : https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_allocation_info

From a dev perspective, would you recommend to use this even in a NT5.2 compatibility context ?

@Extravert-ir
Copy link
Member

Extravert-ir commented Aug 28, 2020

Yes but the part of API we'd need is Vista+ only

You have two options here: use the function from kernel32_vista or manually call NtQueryInformationFile

@HBelusca
Copy link
Contributor

Can't GetFileInformationByHandle (not ex) be used too? As you know, Windows 2000/XP/2003 are able to show an estimation of "Show size on disk" even when they don't have this -Ex API.

@ghost
Copy link
Author

ghost commented Aug 28, 2020

Can't GetFileInformationByHandle (not ex) be used too? As you know, Windows 2000/XP/2003 are able to show an estimation of "Show size on disk" even when they don't have this -Ex API.

Looking at GetFileInformationByHandle here : https://docs.microsoft.com/en-us/windows/win32/api/fileapi/ns-fileapi-by_handle_file_information it seems that only logical size is returned

dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
@katahiromz katahiromz self-requested a review September 1, 2020 11:49
Kyle Katarn and others added 4 commits September 1, 2020 19:41
Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>
Co-authored-by: Katayama Hirofumi MZ <katayama.hirofumi.mz@gmail.com>
Co-authored-by: Katayama Hirofumi MZ <katayama.hirofumi.mz@gmail.com>
Co-authored-by: Katayama Hirofumi MZ <katayama.hirofumi.mz@gmail.com>
@ghost ghost requested review from Extravert-ir and katahiromz September 1, 2020 17:48
@ghost
Copy link
Author

ghost commented Sep 2, 2020

Would it be possible to merge now that no more comments are open ?

@ghost
Copy link
Author

ghost commented Sep 3, 2020

Now using HeapAlloc/HeapFree rather than fixed size array... and the code now crashes.
Help needed.

@ghost
Copy link
Author

ghost commented Sep 3, 2020

To be rewritten with CString as per discussion with @learn-more

@ghost
Copy link
Author

ghost commented Sep 3, 2020

Reverted to MAX_PATH as discussed with @learn-more in Mattermost. Moving to dynamic buffers everywhere will be a dedicated PR.

Current implementation works fine on "C:\ReactOS" (including sub-folders) but does not work properly with "c:\Documents and Settings", raising

err:(dll/win32/shell32/dialogs/filedefext.cpp:1399) FindFirstFileW C:\Documents and Settings\?Default User\* failed
err:(dll/win32/shell32/dialogs/filedefext.cpp:1399) FindFirstFileW C:\Documents and Settings\?All Users\* failed
err:(dll/win32/shell32/dialogs/filedefext.cpp:1399) FindFirstFileW C:\Documents and Settings\?NetworkService\* failed
err:(dll/win32/shell32/dialogs/filedefext.cpp:1399) FindFirstFileW C:\Documents and Settings\?Administrator\* failed

Under investigation (where does this damn "?" comes from....)

@ghost ghost requested a review from learn-more September 5, 2020 14:21
Copy link
Member

@learn-more learn-more left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the memory leaks, the nit is up to you if you want to change it

dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.cpp Outdated Show resolved Hide resolved
dll/win32/shell32/dialogs/filedefext.h Outdated Show resolved Hide resolved
@ghost ghost requested a review from learn-more September 6, 2020 19:34
@learn-more learn-more merged commit 7d44c1c into reactos:master Sep 10, 2020
@ghost ghost deleted the fileprop branch September 10, 2020 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix For bugfix PRs. enhancement For PRs with an enhancement/new feature.
Projects
None yet
7 participants