-
-
Notifications
You must be signed in to change notification settings - Fork 271
Feature: Mainly for updates of Personalization #935
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
Feature: Mainly for updates of Personalization #935
Conversation
Honestly, I don't think the ComboButton is appropriate here. User can set things for both themes, it's not one-or-the-other. |
Maybe you can realize this with a tab interface in this case. |
Actually, only one theme setting will take effect at a time. In the original code, the option of this ComboBox initializes with the current theme of the computer, which is very intuitive. But your point of view is actually very correct. In the original Pick an accent color Page, it used the method you mentioned. If we use ComboBox, it may be ambiguous to users, because users will not know for the first time that different types can be adopted under different themes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quickly from my phone
Or maybe a Selector bar ? Let's give this UX a good night sleep. |
The new wallpaper page looks nice! A couple of notes:
This is really important and mandatory. Otherwise it becomes really confusing when trying to configure it (believe me, I experienced this a lot of times before we implemented this) |
Code quality: Improve WallpaperPickerPageDescribe
It's right. We use ComboBox is the better choice. Because in the configuration file, not all settings correspond to Light or Dark Mode. Component:
Position: Fill
Monitors:
- Id: '\\?\DISPLAY#SKG2725#7&e9a60b&0&UID256#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}' ScreenshotsPicture previewAvoid overlapping controlsColorful border |
Code quality: Improve performanceDescribe
SupplementIn fact, loading monitors in ViewModel will also cause some performance consumption. This is the test result:
Maybe we should separate this method and avoid using x:bind, but GlobalWallpaperPath depends on SelectMonitor (MonitorSettings), which in turn depends on Monitors. My mood: (;´д`)ゞ |
<RadioButton Content="{helpers:ResourceString Name=WallpaperComboBoxItemPicture}" /> | ||
<RadioButton Content="{helpers:ResourceString Name=WallpaperComboBoxItemPictureMM}" /> | ||
<RadioButton Content="{helpers:ResourceString Name=WallpaperComboBoxItemSolidColor}" /> | ||
<RadioButton Content="{helpers:ResourceString Name=ThemePickerTheme11Spotlight}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full string should be "Windows Spotlight". Extra idea: add a Hyperlink to ms-settings:personalization-background
? If we do, we can remove the redundancy in the separator, as in previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your idea. Although not all dividing lines can be merged, because multi-monitors and wallpaper filling methods are only suitable for multi-monitors mode. In the test, I can't seem to use NavigateUri
, which can't trigger correctly. So I ues Click
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't seem to use
NavigateUri
, which can't trigger correctly. So I uesClick
Hm, strange, this should just work... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this.
Idea: if it were me, I would say |
@Jay-o-Way I found a example. |
Yeah, getting monitor information from the windows API is unexplicably slow for some reason, which is why there is this caching mechanism. Previously, loading the monitor settings was done asynchronously on page open. Then the page opened quickly and the monitor strings were loaded in deferred. Afaik there's no way to speed this operation up, so at some point getting the name string is necessary. |
Actually, maybe I'm a little picky. IIn WinUI, we can completely rely on loading asynchronously, but we need to add something to View. Fortunately, the current processing method will not cause too much performance burden. |
monitor-selector.mp4Changing the monitor selection does not update the wallpaper path or preview |
Feature: Add the missing functionDescribe
Screenshots |
@ChenYiLins I'm gonna go ahead and merge in your changes into the branch such that we can start syncing in stuff :) |
Feature: Mainly for updates of WallpaperPickerPage
Describe
Refer to TimePage designed by @Jay-o-Way. The function is basically consistent with the original Page.
Screenshot
Take an overall view
Picture
Picture mm
Solid color