Code Quality: Migrated shared properties to props file

files-community / Files

A modern file manager that helps users organize their files and folders.

https://files.community

MIT License

35.52k stars 2.26k forks source link

Code Quality: Migrated shared properties to props file #16557

Open Lamparter opened 1 month ago

Lamparter commented 1 month ago

Resolved / Related Issues

Closes #14826

Steps used to test these changes

Built the app; checked for Roslyn analyser warnings

This PR finally adds a props file to manage mutual properties where appropriate. It has been built and tested and works perfectly.

I also wanted to implement CPM however haven't succeeded yet.

0x5bfa commented 1 month ago

Please revert changes in Files.sln
Please revert BOM removal from the UTF8 csproj files

Please add this

<FrameworkVersion>net8.0</FrameworkVersion>
<PlatformWinVersion>10.0.22621.0</PlatformWinVersion>
<PlatformWinMinVersion>10.0.19041.0</PlatformWinMinVersion>
<RuntimeIdentifier Condition="'$(Platform)' == 'x86'">win-x86</RuntimeIdentifier>
<RuntimeIdentifier Condition="'$(Platform)' == 'x64'">win-x64</RuntimeIdentifier>
<RuntimeIdentifier Condition="'$(Platform)' == 'arm64'">win-arm64</RuntimeIdentifier>

<TargetFramework>$(FrameworkVersion)-windows$(PlatformWinVersion)</TargetFramework>
<TargetPlatformMinVersion>$(PlatformWinMinVersion)</TargetPlatformMinVersion>

Please add pre-defined constants too https://github.com/files-community/Files/blob/76f36e021864ed67469833ed5f70d098c68acd9a/src/Files.App/Files.App.csproj#L29-L30
```
-DISABLE_XAML_GENERATED_MAIN
+$(DefineConstants);DISABLE_XAML_GENERATED_MAIN
```

0x5bfa commented 1 month ago

LGTM

yaira2 commented 1 month ago

Can you please rebase from main?

yaira2 commented 1 month ago

We're currently facing an issue where the x86 and ARM64 configurations are displaying an error for 'incorrect configurations'.

0x5bfa commented 1 month ago

I think Files.sln got messed up but we should recreate a new .sln and add one by one. One more concern is Win32 configuration; we should have named it x86.

However, for now, as far as current solution file doesn't cause configuration issue, LGTM.

Lamparter commented 1 month ago

Once this PR is merged, I'll create a completely new solution file in another PR and you can review.

0x5bfa commented 1 month ago

I have a pr that simplify the configurations of the c++ projects, so let's wait it too.

0x5bfa commented 2 weeks ago

What's wrong with this PR rn?

Lamparter commented 2 weeks ago

What's wrong with this PR rn?

The x86 version isn't working just yet I'm fixing it right now

Lamparter commented 1 week ago

This PR is now ready for review. I've tried my best to fix the issue with x86 configuration and please let me know if it doesn't work when you review it. As @0x5bfa recommended, I will open a PR with a completely new solution file after this one (and the associated CPM pr) is merged.