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] Sort Navigation-Pane items alphabetically #31183

Closed
wants to merge 2 commits into from

Conversation

sanidhyas3s
Copy link
Contributor

Summary of the Pull Request

The Navigation Pane items are now sorted alphabetically based on their Display Names.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

The Navigation Pane items are now sorted
alphabetically based on their Display Names.

Fixes microsoft#31089, microsoft#23763
@Jay-o-Way
Copy link
Collaborator

Nice. Looks so simple! Just wondering - isn't there some sort of "ArraySort" function that could be applied to the "original" list?

@Jay-o-Way

This comment was marked as resolved.

@stefansjfw
Copy link
Collaborator

GitHub Check issue:

  Failed SvgPreviewControlShouldFillDockForWebView2WhenDoPreviewCalled [1 m]
  Error Message:
   Test method SvgPreviewHandlerUnitTests.SvgPreviewControlTests.SvgPreviewControlShouldFillDockForWebView2WhenDoPreviewCalled threw exception: 
System.ArgumentOutOfRangeException: Index 0 is out of range. (Parameter 'index')
  Stack Trace:
      at System.Windows.Forms.Control.ControlCollection.get_Item(Int32 index)
   at SvgPreviewHandlerUnitTests.SvgPreviewControlTests.SvgPreviewControlShouldFillDockForWebView2WhenDoPreviewCalled() in D:\a\_work\1\s\src\modules\previewpane\UnitTests-SvgPreviewHandler\SvgPreviewControlTests.cs:line 67
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

I bet it's unrelated to these changes, though.

yep. Flaky test. Rerunning
/azp run

@stefansjfw
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Jan 30, 2024

@crutkas is it even possible to locally test this when there are no translated resources locally available?

@Aaron-Junker
Copy link
Collaborator

@crutkas is it even possible to test this when there are no translated resources locally available?

You can just take the .pri file for settings (WinUI3Apps/PowerToys.Settings.pri) from the installation path of PowerToys and replace the one in the installation folder.

To be honest I never tried it with settings, but it works with other modules.

@htcfreek
Copy link
Collaborator

  1. Are we doing the same in OOBE, Settings UI, PowerToys plugin for PT Run and tray flyout's all apps list? - We should do the same in all menus.

  2. Does it sort based on English or based on translated names?

@sanidhyas3s
Copy link
Contributor Author

  1. Are we doing the same in OOBE, Settings UI, PowerToys plugin for PT Run and tray flyout's all apps list? - We should do the same in all menus.

We can if wanted, the same sorting process can be applied there.

  1. Does it sort based on English or based on translated names?

It should do it based on translated names (haven't tested it myself though, there's discussion above regarding this).

@htcfreek
Copy link
Collaborator

  1. Are we doing the same in OOBE, Settings UI, PowerToys plugin for PT Run and tray flyout's all apps list? - We should do the same in all menus.

We can if wanted, the same sorting process can be applied there.

I think we should do this for oobe to be aligned with the setting ux and maybe the tray flyout.

@Jay-o-Way
Copy link
Collaborator

Jay-o-Way commented Jan 30, 2024

My thoughts are expanding on this:

  • if we could use the same source for both navigation menus, they would always be the same
  • i see the solution is: create, remove, re-create. Wouldn't it be cleaner to create the menu from the start in code-behind? Maybe, then the code could also be used for the tray flyout?

I also wonder what it would look like if there is only a part translated to non-Latin languages.

@sanidhyas3s
Copy link
Contributor Author

sanidhyas3s commented Jan 30, 2024

My thoughts are expanding on this:

  • if we could use the same source for both navigation menus, they would always be the same
  • i see the solution is: create, remove, re-create. Wouldn't it be cleaner to create the menu from the start in code-behind?

We can (1) have all the items and their details in the code-behind and not in the XAML, so we sort and insert directly.
or (2) have them in a sort of a set somewhere, and just fetch it in the code-behind, sort and insert.

Edit: Can we combine the NavigationViewItem attributes from OobeShellPage and ShellPage?

Also, if we do want both of them to use the same sorted list, we either do the remove and insert sorted thing wherever we are storing it or fetch it from the one which is sorted earlier, both of which don't seem right.

So what we can and should do is I think <some-common-file.XAML> contains the list of items, with their NavigationViewItem attributes, and we fetch this list of items, sort, and add to our panes, and we do this for separately for all (here, both) navigation menus.

Also, what does OOBE mean?

@htcfreek
Copy link
Collaborator

Also, what does OOBE mean?

This is the "Welcome to PowerToys" window.

A common helper function is made which
sorts the items for both the shell pages.

Fixes microsoft#31089, microsoft#23763
@sanidhyas3s
Copy link
Contributor Author

I have made the Settings and OOBE consistently sorted and use a common helper function.

Re: create, remove, re-create workflow; I think that would need to stay as the sorting is relied on the Content filled into the display items, and any alternative I was able to come up with was just a more complex solution for a not so big problem.

@crutkas
Copy link
Member

crutkas commented Jan 31, 2024

I think this will add a perf hit on start up as we're removing and adding stuff to the DOM. I don't think this change should go in.

@sanidhyas3s
Copy link
Contributor Author

@crutkas Insertion into the DOM is necessary to be able to solve this problem as I don't think there's a way we can reorder in place with . So, will try for a solution that can cut-off the removal part and load the items into the DOM in a sorted way. Will that be fine?

@crutkas
Copy link
Member

crutkas commented Feb 1, 2024

Also we're exploring doing this:
image

This work would conflict with that.

@Jay-o-Way
Copy link
Collaborator

@crutkas Nice to see that concept! 😄 The larger issue would still apply to that new interface/menu, though. What are your thoughts for the best approach?

@sanidhyas3s
Copy link
Contributor Author

@crutkas Modules would still need to be sorted within those groups I guess. Though yes, the work would somewhat conflict.

For the current setup, I was thinking that we can put the items to be inserted/sorted in a separate xaml file, we can fetch, sort, and insert into the Navigation Panes based on the Content value of the items. Can't think of a faster approach, myself.

@crutkas
Copy link
Member

crutkas commented Feb 5, 2024

With the new system being proposed, would the main sections also be reordered?

Our way of doing it also is inline with windows settings. The order is the order. Having a fixed order also helps us determine quickly (and debug) what screen something is having an issue Regardless of language.

This would need to be an off by default setting as well.

@Jay-o-Way
Copy link
Collaborator

One of the problems that makes this difficult is the fact that some are translated and some are not.
Normally, in an app, it's either all or nothing.

@Jay-o-Way
Copy link
Collaborator

Also we're exploring doing this:

@crutkas would you be so kind to update/mention this in #23744?

@crutkas
Copy link
Member

crutkas commented Feb 29, 2024

Closing this out as the current system is intentionally "by design" for the navigation system

@crutkas crutkas closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants