Skip to content

Conversation

gus33000
Copy link
Contributor

@gus33000 gus33000 commented Nov 13, 2021

Resolved / Related Issues

Details of Changes

This pull request brings full ARM64 native support to the Files application. This improves greatly the user experience for ARM64 users as well as overall performance and resource usage of the application, in environments were power optimizations matter the most. It has been verified working on an ARMv8 device running in ARM64 context under Windows 11 Build 22499 for ARM64 Processors. This pull request achieves this by:

  • Converting the Files.Launcher project to the newly released .NET 6
  • Fixing API calls that were broken due to ABI changes with newer .NET versions
  • Migrating away from the deprecated System.Management library (more information available here: System.Management doesn't work on Windows versions without .NETFramework dotnet/runtime#45143) and instead using the supported Microsoft Management Infrastructure (MMI) library
  • Apply a set of project configuration workarounds due to case sensitive MSBuild scripts for Windows Application Packaging Projects and an incompatibility with newer .NET releases
  • Updating the CI pipeline files and ensuring they work on azure pipeline (The regular non release pipeline has been tested working)
  • Enabling ARM64 support for all project components in the solution via configuration
  • Enabling ARM64 support for Custom Open Dialog and registering it inside Files.Launcher*

*This feature does not seem to work, the option reverts by itself and it looks like the component never gets registered. I am not too sure about the status of this feature. The code to register the components looked like it was meant for X86 only and surely would not work on AMD64 or anything else, due to it attempting to register x86 first, which would not work on X64 or ARM64 packages, maybe this needs to be looked into. I doubt this is an issue caused by my PR.

Turns out it this was caused by a different issue fixed in the last commit. The CustomOpenDIalog component however does not get built by the CI. Referencing the project inside Files.Launcher.csproj makes it build however the xcopy task fails which halts the build process. I believe this issue is not caused by my PR considering the revert commit makes it like before, which is copy the file if it exist or ignore.

Validation

  • Built and ran the app
  • Tested the changes for accessibility (Does not apply here)
  • Tested the non release CI pipeline
  • Ensured ACL dialogs were working as well as MTP device detection due to MMI changes

Screenshots (optional)

Screenshot (13)
Screenshot (14)
Screenshot (15)
Screenshot (16)

@gus33000
Copy link
Contributor Author

gus33000 commented Nov 13, 2021

I have reverted 9d5d7e5 because there seem to be an issue with the pipeline setup in this repository where the C++ component does not actually get queued for build, it did on my tests, which is a bit weird. I can remove said commit once this issue is fixed however once again, in my tests, the pipeline worked on my azure devops instance, so I do not understand this issue. (Moreover, it appears the custom file dialog functionality ain't quite working anyway). The application also did build locally just fine.

@gus33000 gus33000 force-pushed the area-arm64support branch 2 times, most recently from e16874e to 6b12c94 Compare November 13, 2021 20:54
@yaira2 yaira2 requested a review from gave92 November 14, 2021 00:57
@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Nov 14, 2021
@gus33000
Copy link
Contributor Author

gus33000 commented Nov 14, 2021

Screenshot (7)

QuickLook detection confirmed working, as well as device detection on ARM64.
Context Menus work, icons too, ACL dialog as well.

IMG_5901.MOV
Files.UWP.ARM64.MOV

@gus33000
Copy link
Contributor Author

Custom file open dialog was verified working by manually registering it (given the option is hidden in the app) and building a package locally (given CI does not build the component). Video below

Files.Custom.Open.Dialog.MOV

.

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

Really nice work! Thank you ❤️

@lukeblevins lukeblevins self-requested a review November 15, 2021 14:10
Copy link
Contributor

@lukeblevins lukeblevins left a comment

Choose a reason for hiding this comment

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

Great work, Gus! I'll take care of moving to System.Text.Json in a future PR.

@lukeblevins lukeblevins added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Nov 15, 2021
@yaira2 yaira2 merged commit 13e923a into files-community:main Nov 15, 2021
@yaira2 yaira2 mentioned this pull request Nov 15, 2021
@yaira2
Copy link
Member

yaira2 commented Nov 15, 2021

@gus33000 is the full trust process working for you in release mode?

@gus33000
Copy link
Contributor Author

@gus33000 is the full trust process working for you in release mode?

Yes it is, see all of my videos posted above.

@gave92
Copy link
Member

gave92 commented Dec 4, 2021

@gus33000 could you take a look at the changes made to DeviceWatcher.cs?
I believe this:

CimInstance obj = e.NewEvent.Instance;

should be:

CimInstance obj = e.NewEvent.Instance.CimInstanceProperties["TargetInstance"].Value

@gus33000
Copy link
Contributor Author

gus33000 commented Dec 5, 2021

@gus33000 could you take a look at the changes made to DeviceWatcher.cs? I believe this:

CimInstance obj = e.NewEvent.Instance;

should be:

CimInstance obj = e.NewEvent.Instance.CimInstanceProperties["TargetInstance"].Value

Looking into it now, just woke up so that might take some time. From testing the above's code worked but I can see if your suggestion makes a difference

@gus33000
Copy link
Contributor Author

gus33000 commented Dec 5, 2021

@gave92 Had a look with a debugger, you're right looks like an issue stepped in here. I'm going to do a PR to address this. Thanks for shedding light on this!
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.NET error

4 participants