Refactor remaining Project and Build files

[Nirmal4G]

Changes

Misc refactors (Follow up of #3894, #3896 and #4068)

• Update variables names and message text.
• Update comments and fix casing, spelling mistakes.
• Rearrange lines for better diff for upcoming patches.

Update Notifications package

• Add ReadMe to Notifications package.
• Add ThirdPartyNotices.txt to the package.
• Place the ReadMe, Icon, License and Notices in the root of the package.

Fixup target UAP versions (non-breaking)

• In both Notifications and XAML Islands Unit Tests projects, We already package them up with lowest UAP version anyway. But the targets don't represent that. So, Fix the project and fallback targets with lowest compatible version instead of latest version.

Update ~OutputPath properties in projects

• Use Base~ output paths where possible.
• Use Count() item function where possible.
• Add missing OutputPath property to WAP projects.

PR Type

What kind of change does this PR introduce?

• Code style update (formatting only)
• Refactoring (no functional changes, no API changes)
• Build or CI related changes (non-breaking)

PR Checklist

Please check if your PR fulfills the following requirements:

• [x] Created a separate branch (other than main/master) in your fork
• [x] Based off latest main branch of toolkit
• [x] Pull Request doesn't include merge commits
• [x] Header has been added to all new source files
• [x] Contains NO breaking changes
• [x] Code follows all style conventions

Other information

• Please Rebase merge if possible.
• Please update the default message option to Default to pull request title and description in the Repository Pull Request settings to have a better commit message instead of Merge pull request #xxxx from repo/branch generic message.

[ghost]

Thanks Nirmal4G for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[michael-hawker]

@Nirmal4G there's a whole bunch of different types of changes here, some seem functional with respect to the targets of the Notifications.

There's not much more value in fixing comments in project files or re-organizing them. If there are things which can be removed because they're in common files great, but lets do that as a separate PR. Same if there are fixes that should be done for the notifications or something (though I don't think we've seen any issues at the moment).

[Nirmal4G]

@michael-hawker

I'm isolating the changes from #4181 that are not related to CPVM feature to figure out why some tests are failing there.

I can open a separate PR if that's what you want but... The notifications change is NOT breaking or intrusive or even a public change.

So, Can you please reopen this?

[michael-hawker]

The main thing that I was worried about for the Notifications was the changes to the versioning in the paths as I don't know how that effects the final package output.

[Nirmal4G]

@michael-hawker Don't worry, it doesn't affect the packaging. I assure you; this PR doesn't contain any breaking change!

[michael-hawker]

@Nirmal4G I've re-opened so you can re-run the CI. Just want to be weary still of the more nitpicky updates to project files to reduce churn for little value, as we have discussed previously.

[Nirmal4G]

Sure, I have reduced unnecessary changes. These changes do flow through the next PR.

I have a separate branch like winui for both UWP Toolkit and Desktop Toolkit repos that contains my final patches. Occasionally, I compare them and create necessary patches for upstream. I'll only create patches that I think would be beneficial.

For example, when you want to update the Notification project for newer UAP target, You'd have to update multiple places but now I've made it so that when you update the target version in the common props will update everywhere.

For the code style update for MVVM source generators project file, I followed what I had done in previous PR. I'm still working on the automation of code style analysis but in the meantime, I'll update the new files when possible.

[michael-hawker]

Thanks @Nirmal4G! We're just wrapping up the 7.1 release this week; so it'll take us a little bit to get to this one. I've flagged it for the next milestone for tracking.

[Nirmal4G]

@michael-hawker I guess it's okay but this patch only contains non-functional changes. It also does not modify build outputs. That's also one of the reasons why I separated this from the CPVM patch tree, so that we can merge this in sooner than latter.

If possible, I would like to merge this in along with the media controls' DesignTools update for the 7.1 release. But even I understand it'll be a stretch!

I'll be opening a PR soon for that, hopefully by tomorrow.

[michael-hawker]

Yeah, we're just in a state now where we're touching as little as possible. Will be easier to get this reviewed after release and in to bake while we're working on the next milestone. Just don't want to accidently introduce an issue if we happen to overlook something this close to release when it's even tangentially related to the build/project structure.

[Nirmal4G]

...there are a number of changes here I wouldn't approve...

These are all non-functional changes. Most of them are removals of redundant code. You can check the summary of individual commits on why the changes been made.

As I mentioned before, they are leftovers from previous refactor PRs. Since I was trying to make them diffable with comments (on changes), I had to separate them from those previous PRs.

[Nirmal4G]

…I don't want any of those file renames/moves you've been doing to the .NET projects…

Is there any reason why you don't want these files in a predictable location?

[Sergio0694]

"Is there any reason why you don't want these files in a predictable location?"

Because what is predictable for you might not be so for others. Those files were purposefully placed in a path that mirrored their respective namespace in the runtime, which made it very easy to tell at a glance where they were coming from when looking at the solution explorer. Furthermore, your changes are mixing public and internal types into the same folder, which is something that I specifically wanted to avoid by using that structure. Changing that is not about fixing build warnings, removing redundant code or anything like that, but it's purely a subjective change, which I don't agree with. And on top of this, like I said, I would like not to be any pending PRs that might make the split for the .NET Community Toolkit any trickier than it will already be 🙂

The other changes are fine, but please revert all file renames and moves in this PR.

[Nirmal4G]

…FYI, you might want to discuss the changes you'd like to do before actually doing the work to open a PR…

I agree. I did open the issue along with the PR but till now no one discussed in those issues, so I didn't push for any discussion for non-functional refactorings.

I already have my private fork which we are using it for ourselves which is way ahead. These are just cosmetic changes, I already did in my private fork. I think they are beneficial in the long run and also bring the repos closer so that in the near future I could contribute very narrow and specific set of changes without any refactorings.

This way the patches I provide will be clean and specific.

[Nirmal4G]

Sure, I'll revert the file moves but we should discuss to come up with a common place for shared and external code. For me atleast, this is isn't very clear.

[Sergio0694]

"I did open the issue along with the PR but till now no one discussed in those issues, so I didn't push for any discussion for non-functional refactorings."

For future reference, if an issue doesn't get replies it's likely just because the maintainers are busy at that time, we're not doing that on purpose, there's just many things to keep track of and we have limited time (and many of us are working on multiple projects at a time). If that happens again, just ping any of us in that issue before doing the actual work 😄

"I think they are beneficial in the long run"

I can appreciate that, I'm just saying that not everyone agrees on what exactly it is that is beneficial. I can agree with many of the refactorings you've been doing, and it's very cool to see you're so passionate about this area! I'm just saying that there might be multiple reasons why some decisions have been taken (such as in this specific case), so better to always ask before doing things.

"I'll revert the file moves"

Thank you 🙂

"we should discuss to come up with a common place for shared and external code"

I'm not sure I agree that we have a problem with this, but sure, we can certainly talk about this in the future, of course. Once the two repositories are split it'll also be easier to discuss each one separately to keep conversations smaller in scope.

[Nirmal4G]

I'm just saying that not everyone agrees on what exactly it is that is beneficial... I'm just saying that there might be multiple reasons why some decisions have been taken (such as in this specific case), so better to always ask before doing things.

True that. That's why I create a PR first with the changes I already have than making an issue first even for the changes to my private fork. It's better to see the changes as a whole and comprehend how it impacts visually and for contributions, than me trying to explain in words; I sometimes won't even understand my own! 😂

As they say, A picture is worth a thousand words, I say, A PR is worth a thousand comments. 😉

[ghost]

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

[Nirmal4G]

As per @Sergio0694's request, I have reverted the file moves. This patch tree was kept as before, just dropping only the last commit containing the file moves.

[Sergio0694]

@Nirmal4G Please stop posting messages like this. You've done this several times already, it's quite rude and pushy. If people haven't reviewed your PR is because they've been busy with other things (there's a lot of other open PRs as well, plus we can't just spend all day reviewing PRs anyway). Michael and the rest of the team will get to this when they can, don't worry 🙂

EDIT: for others reading, there were at least 2-3 messages before this one with just "🔔", that have been deleted by OP.

[Nirmal4G]

@Sergio0694 It was just a reminder. I thought if this is done, I can move on to other things. I'm so sorry if these remainders are distracting. I'll stop posting.

[michael-hawker]

@Nirmal4G really appreciate your enthusiasm, still deciding if we'll have another 7.1.x hotfix, and I'd want this to come in after we do that and start the next cycle, which we've been pushing back a bit. We may still merge #4395 before this though.

[Nirmal4G]

This is finally done. It took a while after #4395 since I had to separate my local repo to match the remote. A lot of my patches lead to lot of conflicts for these kinds of changes!!