Code Quality: Use SourceGenerator to simplify `DependencyProperty` #11587

Poker-sang · 2023-03-07T13:33:25Z

Use Source Generator to simplify DependencyProperty

Validation
How did you test these changes?

Built and ran the app
Tested the changes for accessibility

cinqmilleans · 2023-03-07T15:00:41Z

It's awesome. Let's go one step further and use nameof to avoid typing or editing errors.

[DependencyProperty("ViewModel")]
->
[DependencyProperty(nameof(ViewModel))]

Poker-sang · 2023-03-07T15:04:04Z

It's awesome. Let's go one step further and use nameof to avoid typing or editing errors.

[DependencyProperty("ViewModel")] -> [DependencyProperty(nameof(ViewModel))]

Thank you. But the property ViewModel is also generated, so it cannot be written like this for it is recursive, I believe.

cinqmilleans · 2023-03-07T15:12:33Z

Yes you are right.

src/Files.SourceGenerator/Utilities.cs

Poker-sang · 2023-04-03T03:15:59Z

Can this PR be approved, or does it need to be modified or something?

yaira2 · 2023-04-03T15:41:53Z

Can this PR be approved, or does it need to be modified or something?

The changes should be discussed with the other contributors first, we started discussing it on Discord but there wasn't a definite conclusion yet.

0x5bfa

I'm on board this introduction.

Probably you need to split up changes into adding Files.SourceGenerator and applying changes because making changes to Files.App will cause file conflists when it takes a lot time to be merged. Can you revert changes in Files.App for the time being(I don't want you to waste)?

Creating a new PR for this and copying files or merging upstream with GitHub Desktop will help you.

src/Files.SourceGenerator/TypeWithAttributeGenerator.cs

Poker-sang · 2023-05-15T05:29:57Z

Ok, I will fix this.

0x5bfa · 2023-05-15T10:04:33Z

Great! I’m sorry for inconvenient.
Can you apply to a variable ‘ViewModel’ in ‘UserControl/PathBreadcrumb.xaml.cs’ so that reviewers always can validate these changes without file conflicts? I was thinking which file is less changed lately. That file is good to go!

The second I confirm beneficial effect, then I will approve the PR

Poker-sang · 2023-05-15T10:54:43Z

Sure

0x5bfa

Looks good to me. Thank you!!

Files.sln

d2dyno1 · 2023-05-16T19:59:08Z

I think we can move forward with this until CommunityToolkit adds their own implementation 🙂

src/Files.SourceGenerator/Utilities/Helper.cs

d2dyno1 · 2023-05-16T20:02:58Z

Could you also add license info to each file?

Poker-sang · 2023-05-17T01:23:07Z

OK.

0x5bfa

LGTM!

hecksmosis

LGTM

…erty` (#11587)" This reverts commit 922e528.

src/Files.App/UserControls/PathBreadcrumb.xaml.cs

…encyProperty` (#11587)"" This reverts commit 697f1e1.

0x5bfa · 2023-05-28T04:41:10Z

@Poker-sang Thank you for contributing to the community! We investigated further and discovered that the app will fail to compile using the x86 & ARM64 configurations. In order to continue releasing updates, it seems that we'll have to revert these changes. We apologize for the inconvenience and we'll be happy to merge the changes once a solution is found.

Poker-sang · 2023-05-28T04:57:29Z

@0x5bfa I'm sorry that my PR took up your time to troubleshoot the problem. But I don't encounter such problem when I use it myself (same with the source generator in CommunityToolkit.Mvvm). Can you tell me how to reproduce this kind of problem for my further improvement?

0x5bfa · 2023-05-28T05:04:15Z

Can you tell me how to reproduce this kind of problem

Building with x86 configuration in Visual Studio.

You can see also the build log here: https://dev.azure.com/filescommunity/Files/_build/results?buildId=19041&view=results

Poker-sang · 2023-05-28T05:31:53Z

It is weird. I have no problems building with x86 config. Do you have any error logs when building with VS or something?

0x5bfa · 2023-05-28T05:40:50Z

Build with Azure Pipeline log https://dev.azure.com/filescommunity/Files/_build/results?buildId=19041&view=results
Build with GitHub Actions log https://github.com/0x5bfa/Files/actions/runs/5102586150

yaira2 · 2023-05-28T05:46:27Z

@Poker-sang what happens if you change the build configuration from debug to preview?

Poker-sang · 2023-05-28T05:51:00Z

@yaira2 I can still successfully build and deploy it (I am using VS2022)

Poker-sang · 2023-05-28T06:11:49Z

2023-05-28T05:26:12.8886800Z CompilerServer: server failed - server rejected the request due to analyzer / generator issues 'Could not load file or assembly 'file:///C:\Users\runneradmin\AppData\Local\Temp\VBCSCompiler\AnalyzerAssemblyLoader\ff7d303dfe604105b8be1433d592f9d8\20\Files.SourceGenerator.dll' or one of its dependencies. An attempt was made to load a program with an incorrect format.' - 15c10098-e7ac-4e6f-860e-0c16ce365c0f
2023-05-28T05:26:14.3194118Z CSC : warning CS8034: Unable to load Analyzer assembly D:\a\Files\Files\src\Files.SourceGenerator\bin\x86\Release\netstandard2.0\Files.SourceGenerator.dll : Could not load file or assembly 'file:///D:\a\Files\Files\src\Files.SourceGenerator\bin\x86\Release\netstandard2.0\Files.SourceGenerator.dll' or one of its dependencies. An attempt was made to load a program with an incorrect format. [D:\a\Files\Files\src\Files.App\Files.App.csproj]

Looks like an error occurred with the loaded DLL. Maybe we loaded x64 dll (but we used AnyCPU config)?

0x5bfa · 2023-05-28T07:16:27Z

Reverted changes(in local). Successful.

0x5bfa · 2023-05-28T07:20:59Z

Looks like an error occurred with the loaded DLL. Maybe we loaded x64 dll (but we used AnyCPU config)?

Probably. Alghough I added configurations respectively instead of Any CPU and added configuration and pubxml file configuration to the SourceGenerator project, I couldn't make it. Probably, this is referencing issue.

Poker-sang · 2023-05-28T08:06:58Z

@0x5bfa I appreciate that you can help me so much. My source generator in another project can be build successfully by Github Action. I'm comparing if there was any differences that caused the failure.
https://github.com/Pixeval/Pixeval/blob/main/.github/workflows/build.yml

Poker-sang · 2023-05-28T10:19:12Z

@yaira2 @0x5bfa The problem is solved. It seems that this is because the solution's platform map is not followed by GitHub action. Thanks for Huo Yaoyuan guidance.
If you add <PlatformTarget>AnyCPU</PlatformTarget> to SourceGenerator.csproj, then the problem is solved.
See: https://github.com/Poker-sang/Files/actions/runs/5103653039/jobs/9174045550

hecksmosis reviewed Mar 7, 2023

View reviewed changes

src/Files.SourceGenerator/Utilities.cs Outdated Show resolved Hide resolved

yaira2 added the needs - code review label Mar 9, 2023

yaira2 changed the title ~~Refactor: Use SourceGenerator to simplify DependencyProperty~~ Code Quality: Use SourceGenerator to simplify DependencyProperty Mar 9, 2023

yaira2 requested a review from d2dyno1 March 9, 2023 04:27

0x5bfa reviewed May 15, 2023

View reviewed changes

src/Files.SourceGenerator/TypeWithAttributeGenerator.cs Outdated Show resolved Hide resolved

Poker-sang requested a review from 0x5bfa May 15, 2023 05:58

Poker-sang closed this May 15, 2023

Poker-sang deleted the main2 branch May 15, 2023 06:00

Poker-sang restored the main2 branch May 15, 2023 06:02

Poker-sang reopened this May 15, 2023

0x5bfa previously approved these changes May 16, 2023

View reviewed changes

heftymouse suggested changes May 16, 2023

View reviewed changes

Files.sln Outdated Show resolved Hide resolved

Poker-sang dismissed 0x5bfa’s stale review via 18bdf6d May 16, 2023 16:41

d2dyno1 suggested changes May 16, 2023

View reviewed changes

src/Files.SourceGenerator/Utilities/Helper.cs Outdated Show resolved Hide resolved

Poker-sang requested a review from d2dyno1 May 17, 2023 01:53

0x5bfa approved these changes May 17, 2023

View reviewed changes

Poker-sang added 3 commits May 17, 2023 23:40

add SourceGenerator

9cce828

use block-scoped namespace

b8a31c4

Add example

c4cd1d3

Poker-sang added 2 commits May 17, 2023 23:40

fix AnyCPU in .sln

43d1c03

add license

48f6bcc

0x5bfa requested a review from hecksmosis May 18, 2023 15:28

hecksmosis reviewed May 18, 2023

View reviewed changes

0x5bfa requested review from yaira2 and heftymouse May 19, 2023 14:07

d2dyno1 approved these changes May 20, 2023

View reviewed changes

yaira2 merged commit 922e528 into files-community:main May 21, 2023

yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 21, 2023

Poker-sang deleted the main2 branch May 21, 2023 02:33

yaira2 added a commit that referenced this pull request May 21, 2023

Revert "Code Quality: Use SourceGenerator to simplify `DependencyProp…

697f1e1

…erty` (#11587)" This reverts commit 922e528.

yaira2 reviewed May 21, 2023

View reviewed changes

src/Files.App/UserControls/PathBreadcrumb.xaml.cs Show resolved Hide resolved

yaira2 added a commit that referenced this pull request May 21, 2023

Revert "Revert "Code Quality: Use SourceGenerator to simplify `Depend…

7e59fb0

…encyProperty` (#11587)"" This reverts commit 697f1e1.

Poker-sang restored the main2 branch May 21, 2023 18:17

Poker-sang deleted the main2 branch May 23, 2023 02:37

Poker-sang mentioned this pull request May 28, 2023

Fix: Fixed SourceGenerator build failure #12471

Merged

8 tasks

Uh oh!

Code Quality: Use SourceGenerator to simplify DependencyProperty #11587

Code Quality: Use SourceGenerator to simplify DependencyProperty #11587

Uh oh!

Conversation

Poker-sang commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cinqmilleans commented Mar 7, 2023

Uh oh!

Poker-sang commented Mar 7, 2023

Uh oh!

cinqmilleans commented Mar 7, 2023

Uh oh!

Uh oh!

Poker-sang commented Apr 3, 2023

Uh oh!

yaira2 commented Apr 3, 2023

Uh oh!

0x5bfa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Poker-sang commented May 15, 2023

Uh oh!

0x5bfa commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Poker-sang commented May 15, 2023

Uh oh!

0x5bfa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

d2dyno1 commented May 16, 2023

Uh oh!

Uh oh!

d2dyno1 commented May 16, 2023

Uh oh!

Poker-sang commented May 17, 2023

Uh oh!

0x5bfa left a comment

Choose a reason for hiding this comment

Uh oh!

hecksmosis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0x5bfa commented May 28, 2023

Uh oh!

Poker-sang commented May 28, 2023

Uh oh!

0x5bfa commented May 28, 2023

Uh oh!

Poker-sang commented May 28, 2023

Uh oh!

0x5bfa commented May 28, 2023

Uh oh!

yaira2 commented May 28, 2023

Uh oh!

Poker-sang commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Poker-sang commented May 28, 2023

Uh oh!

0x5bfa commented May 28, 2023

Uh oh!

0x5bfa commented May 28, 2023

Uh oh!

Poker-sang commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Poker-sang commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Code Quality: Use SourceGenerator to simplify `DependencyProperty` #11587

Code Quality: Use SourceGenerator to simplify `DependencyProperty` #11587

Poker-sang commented Mar 7, 2023 •

edited

Loading

0x5bfa left a comment •

edited

Loading

0x5bfa commented May 15, 2023 •

edited

Loading

Poker-sang commented May 28, 2023 •

edited

Loading

Poker-sang commented May 28, 2023 •

edited

Loading

Poker-sang commented May 28, 2023 •

edited

Loading