search

files-community / Files

A modern file manager that helps users organize their files and folders.
https://files.community
MIT License
34.61k stars 2.2k forks source link

Code Quality: Replace Vanara with CsWin32 #15000

Open gumbarros opened 8 months ago

gumbarros commented 8 months ago

Description

Related to https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/syslib1050-1069.

Concerned code

Example:


[DllImport("api-ms-win-core-timezone-l1-1-0.dll", SetLastError = true)]
public static extern bool FileTimeToSystemTime(
    ref FILETIME lpFileTime,
    out SYSTEMTIME lpSymTime);stemTime);

to

[LibraryImport("api-ms-win-core-timezone-l1-1-0.dll", SetLastError = true)]
public static extern bool FileTimeToSystemTime(
    ref FILETIME lpFileTime,
    out SYSTEMTIME lpSystemTime);

Gains

Requirements

Comments

No response

gumbarros commented 8 months ago

https://learn.microsoft.com/en-us/dotnet/standard/native-interop/pinvoke-source-generation

Another reference explaining the benefits. I think this is an important issue.

0x5bfa commented 7 months ago

I like this idea. Feel free to work on it with @yaira2’s approval.

yaira2 commented 7 months ago

Sounds good, it might be worth splitting this into multiple PRs to make it easier to test and review.

0x5bfa commented 7 months ago

Thanks for the initial try. Do you know CsWin32? If you know, you can replace them with API generated by CsWin32. Using DllImport or LibraryImport is deprecation in Files app because we plan to use CsWin32 instead of those code and Vanara.

gumbarros commented 7 months ago

Just checked about CsWin32, looks promising because of source generator usage like LibraryImportAttribute. Will make a try tonight at NativeFindStorageItemHelper.cs

gumbarros commented 7 months ago

Don't know if this is expected behavior when migrating: https://github.com/microsoft/CsWin32/issues/1167

0x5bfa commented 7 months ago

I'm not sure, but this function in C language uses void*. I assume that whether adding in or out and even ref is defined in win32metadata or CsWin32 repo.

#include <fileapifromapp.h>

WINSTORAGEAPI HANDLE FindFirstFileExFromAppW(
  LPCWSTR            lpFileName,
  FINDEX_INFO_LEVELS fInfoLevelId,
  LPVOID             lpFindFileData, // HERE
  FINDEX_SEARCH_OPS  fSearchOp,
  LPVOID             lpSearchFilter,
  DWORD              dwAdditionalFlags
) noexcept;

You can use Marshal.PtrToStructure:

using System.Runtime.InteropServices;

Marshal.PtrToStructure((nint)lpFindFileData, typeof(WIN32_FIND_DATA));

And you don't need to migrate all of inside of NativeFindStorageItemHelper because it would make us unreviewable so why not migrate step by step. Also, if you're struggling with some issues you can skip, or you can pick functions up from Win32PInvoke that I made in my PR #15075 after merged because I gathered methods into single file and I think it's a good play ground as well for you.

0x5bfa commented 7 months ago

Now you can see Files.App.Helpers.Win32 Win32PInvoke class. We can replace them with CsWin32 piece by piece (starting from about 10 functions).

0x5bfa commented 1 month ago

[!IMPORTANT]

  • Please Use CsWin32 instead of replacing with LibraryImport. This Win32PInvoke class should be going to contain undocumented native functions defined through LibraryImport or DllImport when possible.
  • Don't use "*FromApp" functions. They are intended to be used on UWP. Instead, use native ones.
  • E is easy, M is medium, D is difficult. If anyone else can take a look, it'd be good to start from ones marked as E.

Documented native methods (targets to convert to CsWin32)

Undocumented native functions