Skip to content

Conversation

Jack251970
Copy link
Member

@Jack251970 Jack251970 commented Feb 21, 2025

Unify settings panel design

Unify desgin in InstalledPluginDisplayKeyword.xaml, JsonRPCPluginSettings.cs and settings panel xaml files of all plugins. All designed resources are in CustomControlTemplate.xaml.

Also fix JsonRPC plugin settings empty issues in JsonRPCSettings.cs. (Since this is import part for testing JsonRPC settings panel, I put it in this PR)

Fix:#2577, #3182 (InitializeAsync())

TODO list

  • InstalledPluginDisplayKeyword.xaml
  • JsonRPCPluginSettings.cs (To test )
  • BrowserBookmark
  • Calculator
  • Explorer
  • PluginManager
  • Program
  • Shell
  • Sys
  • WebSearch

Test

  1. JsonRPC

image

  1. Dotnet

image

This comment has been minimized.

@Jack251970
Copy link
Member Author

@onesounds Hi, I’d like to get your idea on the design for the settings panel. Which option do you think is better, or do you feel they both work well?

Left aligned version:

Screenshot 2025-02-21 222837

Right aligned version:

Screenshot 2025-02-21 222837 - Copy

@Jack251970
Copy link
Member Author

@onesounds Hi, I’d like to get your idea on the design for the settings panel. Which option do you think is better, or do you feel they both work well?

Left aligned version:

Screenshot 2025-02-21 222837

Right aligned version:

Screenshot 2025-02-21 222837 - Copy

@Flow-Launcher/team Hello everyone, I’d like to get your feedback on this design. What are your thoughts?

@cibere
Copy link
Contributor

cibere commented Feb 24, 2025

@Flow-Launcher/team Hello everyone, I’d like to get your feedback on this design. What are your thoughts?

I definitely like the left align more.

With the right align, it seems unnecessary to add the separation and make it a little harder to match the label to the input. for me at least, it takes me a little bit longer to match the label to the inputs on the right align vs the left align

@onesounds
Copy link
Contributor

For the check box type, left alignment is recommended, and for others, right alignment is recommended. (I referred to powertoy run and Windows 11's settings window as a reference.) I've tried to unify this a few times, but it wasn't easy because there were a lot of plug-ins with complicated settings. Anyway, the basic design recommends the Power Toy layout.

@Jack251970
Copy link
Member Author

@Flow-Launcher/team Hello everyone, I’d like to get your feedback on this design. What are your thoughts?

I definitely like the left align more.

With the right align, it seems unnecessary to add the separation and make it a little harder to match the label to the input. for me at least, it takes me a little bit longer to match the label to the inputs on the right align vs the left align

Thanks for your reply!❤

@Jack251970
Copy link
Member Author

For the check box type, left alignment is recommended, and for others, right alignment is recommended. (I referred to powertoy run and Windows 11's settings window as a reference.) I've tried to unify this a few times, but it wasn't easy because there were a lot of plug-ins with complicated settings. Anyway, the basic design recommends the Power Toy layout.

Thanks for your reply!❤ I wonder why for the check box type, left alignment is recommended?

image

As you can see in Windows Settings, it is also right-aligned. 😢

@Jack251970
Copy link
Member Author

For the check box type, left alignment is recommended, and for others, right alignment is recommended. (I referred to powertoy run and Windows 11's settings window as a reference.) I've tried to unify this a few times, but it wasn't easy because there were a lot of plug-ins with complicated settings. Anyway, the basic design recommends the Power Toy layout.

NVM. I confused Toggle Button and Check Box.

@VictoriousRaptor
Copy link
Contributor

Thanks for your reply!❤ I wonder why for the check box type, left alignment is recommended?

Just some personal idea:

Right alignment (or justified alignemnt if you consider labels and controls as a whole line) is harder to match label and control as cibere mentioned. but windows 11 in your example image has a box containing the label and control so it's not a big deal. As in our program we don't have boxes so left alignment is better IMO.

Here's an example of windows 11 using left alignment.

image

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@Jack251970 Jack251970 added kind/ui related to UI, icons, themes, etc and removed 1 min review labels Feb 27, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs (2)

396-399: ⚠️ Potential issue

Potential NullReferenceException in checkbox event handler.

The code uses the non-null assertion operator (!) on IsChecked, which could lead to a NullReferenceException. A null-coalescing operator would be safer.

- Settings[attributes.Name] = ((CheckBox)sender).IsChecked!;
+ Settings[attributes.Name] = ((CheckBox)sender).IsChecked ?? defaultValue;

141-143: ⚠️ Potential issue

Missing null check in CreateSettingPanel method.

The method should verify NeedCreateSettingPanel() is true before continuing, as the comment suggests but the code doesn't implement this check.

- // No need to check if NeedCreateSettingPanel is true because CreateSettingPanel will only be called if it's true
- // if (!NeedCreateSettingPanel()) return null;
+ // Verify preconditions to ensure this method is called correctly
+ if (!NeedCreateSettingPanel()) 
+ {
+     return null;
+ }
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml (1)

93-136: Grid layout with numerous row definitions could be simplified.

While the grid structure provides good organization, having 40+ row definitions makes maintenance challenging. Consider grouping related settings into nested panels or user controls to reduce the complexity of the main grid.

- <Grid.RowDefinitions>
-     <RowDefinition />
-     <RowDefinition />
-     <!-- many more row definitions -->
- </Grid.RowDefinitions>
+ <Grid.RowDefinitions>
+     <!-- Use fewer rows for main sections -->
+     <RowDefinition Height="Auto" /> <!-- General Settings -->
+     <RowDefinition Height="Auto" /> <!-- Native Context Menu -->
+     <RowDefinition Height="Auto" /> <!-- Preview Panel -->
+     <RowDefinition Height="Auto" /> <!-- Everything Setting -->
+     <RowDefinition Height="Auto" /> <!-- Customise Action Keywords -->
+     <RowDefinition Height="Auto" /> <!-- Quick Access Links -->
+     <RowDefinition Height="Auto" /> <!-- Index Search Excluded Paths -->
+ </Grid.RowDefinitions>

Then use nested Grid or StackPanel controls in each section with their own row definitions.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1d625c9 and 782c11c.

📒 Files selected for processing (6)
  • Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs (5 hunks)
  • Flow.Launcher/Resources/CustomControlTemplate.xaml (1 hunks)
  • Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1 hunks)
  • Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml (3 hunks)
  • Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml (4 hunks)
  • Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml
  • Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml
  • Flow.Launcher/Resources/CustomControlTemplate.xaml
🔇 Additional comments (13)
Plugins/Flow.Launcher.Plugin.Calculator/Main.cs (1)

158-168: Great refactoring to modernize and improve code quality!

The changes to the GetDecimalSeparator method offer several improvements:

  1. Making the method static aligns with its behavior since it only uses static members and doesn't rely on instance state.
  2. Fixed the typo in variable name from systemDecimalSeperator to systemDecimalSeparator.
  3. Refactored to use modern C# switch expression syntax instead of a traditional switch statement, making the code more concise and readable.

These changes improve code clarity and maintainability without altering the functionality.

Plugins/Flow.Launcher.Plugin.Explorer/Views/ExplorerSettings.xaml (3)

19-26: Well-structured style definition for consistency.

The new HeaderTextBlockStyle provides consistent styling for section headers, using resource references for margins which enhances design uniformity across the application.


407-424: Good use of WrapPanel for checkbox layout.

The WrapPanel provides a responsive layout for the checkboxes, allowing them to flow naturally as the window resizes, which improves the user experience.


139-197: General settings layout follows consistent patterns.

The general settings section is well-structured with proper spacing and alignment. The consistent use of margin resources and proper alignment properties creates a clean interface.

Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs (9)

11-11: Good practice enabling nullable reference types.

Enabling nullable reference types helps catch potential null reference exceptions at compile time rather than runtime.


22-26: Improved type safety with nullable object types.

Converting dictionary properties and fields to use nullable object types (object?) increases type safety and makes null handling explicit, which reduces the risk of NullReferenceExceptions.


28-34: Extraction of UI constants improves consistency.

Pulling margin, padding, and size values from application resources ensures UI consistency across different parts of the application and makes future design changes easier.


38-58: Improved handling of JSON deserialization.

The enhanced type conversion for JsonElement values ensures settings are properly interpreted based on their ValueKind, which fixes potential type mismatch issues when loading settings.


64-82: Cleaner initialization logic with early returns.

The use of continue statements for validation checks improves readability by reducing nesting, following the "fail fast" principle and making the code easier to understand.


87-87: Added defensive check for null or empty settings.

This early return prevents unnecessary processing when there are no settings to update, which is a good defensive programming practice.


109-115: Robust type checking for checkbox values.

The improved parsing logic handles different value types more gracefully, properly converting string representations to boolean values and providing sensible defaults.


172-174: Clean logic using negative condition to simplify code flow.

The inverted condition reduces nesting and makes the code more readable by handling special cases early, following good control flow practices.


485-485: Fixed "separator" spelling for consistency.

The spelling was corrected from "seperator" to "separator", ensuring consistency with other parts of the code.

This comment has been minimized.

@Jack251970 Jack251970 requested a review from onesounds March 20, 2025 13:36

This comment has been minimized.

Copy link

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

❌ Errors Count
❌ forbidden-pattern 22
⚠️ ignored-expect-variant 1
⚠️ non-alpha-in-dictionary 19

See ❌ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

@Jack251970 Jack251970 requested a review from Yusyuriv March 20, 2025 14:03
@onesounds onesounds dismissed Yusyuriv’s stale review March 20, 2025 14:05

for the merge~

@Jack251970 Jack251970 merged commit dda008f into Flow-Launcher:dev Mar 20, 2025
4 checks passed
@Jack251970 Jack251970 added this to the 1.20.0 milestone Mar 20, 2025
@jjw24 jjw24 removed the enhancement New feature or request label Apr 10, 2025
@Jack251970 Jack251970 linked an issue Jun 6, 2025 that may be closed by this pull request
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working kind/ui related to UI, icons, themes, etc

Projects

None yet

6 participants