Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings: XAML refactoring #29785

Closed
wants to merge 38 commits into from
Closed

Settings: XAML refactoring #29785

wants to merge 38 commits into from

Conversation

Jay-o-Way
Copy link
Collaborator

@Jay-o-Way Jay-o-Way commented Nov 10, 2023

Summary of the Pull Request

Overhaul of Settings & Welcome XAML files.

(invisible)

  • Consolidated namespaces and (Visual Studio) removed unneeded namespaces
  • Re-wrote bits of x:bind grammar: {x:Bind Mode=m, Path=p}{x:Bind p, Mode=m}
  • Fixed spelling: EnableEtraBoxesEnableExtraBoxes (imagesize.cs)
  • Re-wrote part on MouseUtils page where strings are concatenated
  • A number of ui:FontIcon elements still had FontFamily and/or FontSize declared
  • Trailing line endings

(visible)

  • A few adjustments in Margins and Spacings - some visible, some not
  • A few Styles (incl. OobeSubtitleStyle → SubtitleTextBlockStyle)
  • OOBE: the Hyperlink gets a new line
  • OOBE: module image is now centered
  • removed incorrect background for OOBE Peek page

PR Checklist

  • Part of: [Settings] UI suggestions #18866
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Localization: All end user facing strings can be localized

Detailed Description of the Pull Request / Additional comments

DataContext -> new pr

image
↳ (0.75.1) This padding should not be here; missing space next to keys

image
↳ (pr) SubtitleTextBlockStyle; Hyperlink on new line; padding fixed

Validation Steps Performed

Build with Visual Studio.

@Jay-o-Way Jay-o-Way added Product-Settings The standalone PowerToys Settings application Area-User Interface things that regard UX for PowerToys Issue-Refactoring We want to adjust code labels Nov 10, 2023
@htcfreek
Copy link
Collaborator

OOBE : the Hyperlink gets a new line\n

What about switching order between hyperlink and buttons? 🤔 (First hyperlink, then buttins.)

@Jay-o-Way
Copy link
Collaborator Author

Possible. But why? Links usually have a "less than important" feeling.

@htcfreek
Copy link
Collaborator

Possible. But why? Links usually have a "less than important" feeling.

I thought it might look better to have the "big" buttons after the "small" text and link, and not between.

@Jay-o-Way

This comment was marked as resolved.

@htcfreek
Copy link
Collaborator

htcfreek commented Nov 13, 2023

Ready for review.
I don't understand ☹️ How does .\.pipelines\applyXamlStyling.ps1 -Main work?

@Jay-o-Way
There is the VS plugin XamlStyler. You should install it and then use it to fix xaml code style.

https://marketplace.visualstudio.com/items?itemName=TeamXavalon.XAMLStyler2022

@Jay-o-Way Jay-o-Way requested a review from niels9001 November 26, 2023 15:37
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

I usually run this incantation when switching branches: git clean -ffxd

So, Settings doesn't crash for me if I run it directly from Visual Studio (PowerToys Run plugins, at least, since VCM page is disabled when running like that).

It still crashes if I run it through "Start without Debugging"(Ctrl+F5) or if I start the runner and then start Settings from the tray icon. (This is what I usually do when testing since it's the normal way to start the Settings apps)

On PowerToys Run page, when I try to expand one of the plugins, it crashes the Settings app.
When I try to open the VCM Settings page, it crashes Settings.

Does it crash for you as well?

I gave the PR a try again and it still crashes for me on these two places.

@htcfreek
Copy link
Collaborator

@Jay-o-Way , @jaimecbernardo
Did you both try with a clean build or did one of you only do rebuilds?

@Jay-o-Way Jay-o-Way added Help Wanted We encourage anyone to jump in on these and submit a PR. and removed Help Wanted We encourage anyone to jump in on these and submit a PR. labels Dec 21, 2023
@Jay-o-Way
Copy link
Collaborator Author

Jay-o-Way commented Dec 21, 2023

@jaimecbernardo @niels9001 GREAT NEWS!
I have found and fixed the cause of the Plug-in error. I had changed a SettingsCard + InfoBar to InfoBar, but that wasn't allowed. Which I could have known sooner, would I have read your comment better 🙄

Looks like something can't deal with an empty/null string as Image Source? When I do have an image readily set, and then use the button to ClearOverlayImageAction, the app freezes.

Important

This also happens in 0.76.2 (GA) so this is not an issue created in this PR!

@Jay-o-Way
Copy link
Collaborator Author

Guys, have a moment? 😇

@niels9001
Copy link
Contributor

Guys, have a moment? 😇

Most of us are out of office for the holidays. I'll make sure to get back to this once I'm back!

@niels9001 niels9001 self-assigned this Dec 29, 2023
@@ -2,51 +2,47 @@
x:Class="Microsoft.PowerToys.Settings.UI.Views.EnvironmentVariablesPage"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:controls="using:Microsoft.PowerToys.Settings.UI.Controls"
xmlns:controls1="using:CommunityToolkit.WinUI.Controls"
xmlns:custom="using:Microsoft.PowerToys.Settings.UI.Controls"
Copy link
Contributor

Choose a reason for hiding this comment

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

@Jay-o-Way Why is this namespace custom now? That's rather confusing actually, since they are using the .Controls namespace? That's why we use converters namespace for Converters

Can't we use controls for PowerToys controls, and toolkit or toolkitcontrols for all controls part of the Toolkit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, looking at the updated XAML.. toolkitcontrols is pretty verbose and makes the XAML harder to read :(.

Could we use tk:SettingsCard instead maybe? Or we do use custom for custom controls (it's used only in a handful of files) and use controls for Toolkit controls?

Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way Dec 29, 2023

Choose a reason for hiding this comment

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

Why is this namespace custom now?

and

looking at the updated XAML.. toolkitcontrols is pretty verbose and makes the XAML harder to read :(.

Could we use tk:SettingsCard instead maybe?In the main branch, custom is already (consistently) used through-out the OOBE xaml pages. Under the rest of SettingsXAML area/folder, controls, local, and custom are all used.

@niels9001 Both PowerToys and CommunityToolkit can have namespaces for Controls and Converters declared. That's four combinations. Simply using "controls" or "converters" is ambiguous. We can shorten "CommunityToolkit" to "ct" and "PowerToys" to "pt", but when both versions of Controls or Converters are used in the same file, we can't shorten it any more than that. (Aside from skipping vowels...)

Therefore I suggest

  • xmlns:ptcontrols="using:Microsoft.PowerToys.Settings.UI.Controls"
  • xmlns:ptconverters="using:Microsoft.PowerToys.Settings.UI.Converters"
  • xmlns:ctcontrols="using:CommunityToolkit.WinUI.UI.Controls"
  • xmlns:ctconverters="using:CommunityToolkit.WinUI.Converters"

#18866 (comment)

Images:

image
image
image

@Jay-o-Way
Copy link
Collaborator Author

Jay-o-Way commented Dec 29, 2023

@niels9001 you're right. The longer it takes, the bigger it gets. I should re-do this into a few prs.

  • Namespaces
  • Bindings
  • Visual improvements
  • etc.

@Jay-o-Way Jay-o-Way marked this pull request as draft December 29, 2023 17:05
@Jay-o-Way Jay-o-Way removed Needs-Review This Pull Request awaits the review of a maintainer. revisit-0.77 labels Dec 29, 2023
@Jay-o-Way Jay-o-Way closed this Dec 29, 2023
@Jay-o-Way Jay-o-Way deleted the Settings-XAML-tweaks branch December 30, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface things that regard UX for PowerToys Issue-Refactoring We want to adjust code Product-Settings The standalone PowerToys Settings application
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants