Feature: Add network discovery prompt when turned off#18560
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a Network Discovery warning InfoBar to the main UI when viewing an empty Network folder, with a shortcut into Windows network advanced settings.
Changes:
- Introduces a
ShellViewModel.IsNetworkDiscoveryInfoBarOpenflag and updates it based on folder emptiness and current directory. - Adds a new
InfoBartoMainPage.xamland wires it to the active pane’sShellViewModelviaPropertyChanged. - Simplifies
GridLayoutPage.xamlgrid row usage by removing explicit row definitions/assignments in the shown hunk.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Files.App/Views/MainPage.xaml.cs | Tracks active ShellViewModel to show/hide the Network Discovery InfoBar and handles InfoBar actions/events. |
| src/Files.App/Views/MainPage.xaml | Adds the Network Discovery InfoBar and shifts grid rows to accommodate it. |
| src/Files.App/Views/Layouts/GridLayoutPage.xaml | Removes explicit row definitions and Grid.Row assignments in the root grid snippet. |
| src/Files.App/ViewModels/ShellViewModel.cs | Adds IsNetworkDiscoveryInfoBarOpen and sets it when in the Network folder and empty. |
| src/Files.App/Strings/en-US/Resources.resw | Adds localized strings for the new InfoBar title/message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0x5bfa
force-pushed
the
fix-9735
branch
2 times, most recently
from
ee6962b to
67c5517
Compare
|
We need better check for the network discovery being turned off. I have an idea. |
| <value>Open settings</value> | ||
| </data> | ||
| <data name="NetworkDiscoveryInfoBarTitle" xml:space="preserve"> | ||
| <value>Network discovery might be turned off</value> |
There was a problem hiding this comment.
| <value>Network discovery might be turned off</value> | |
| <value>Network discovery is turned off</value> |
| <value>Network discovery might be turned off</value> | ||
| </data> | ||
| <data name="NetworkDiscoveryInfoBarMessage" xml:space="preserve"> | ||
| <value>Network devices may not appear when network discovery is turned off. Open Windows Settings to enable it.</value> |
There was a problem hiding this comment.
| <value>Network devices may not appear when network discovery is turned off. Open Windows Settings to enable it.</value> | |
| <value>Network devices will not appear when network discovery is turned off. Open Windows Settings to enable it.</value> |
| </local:BaseGroupableLayoutPage.Resources> | ||
|
|
||
| <Grid x:Name="RootGrid" ContextFlyout="{x:Bind BaseContextMenuFlyout}"> | ||
| <Grid.RowDefinitions> |
There was a problem hiding this comment.
Why were the RowDefinitions removed?
There was a problem hiding this comment.
THis wasn't used and thought its not worth creating a separate PR.
Should I revert?
There was a problem hiding this comment.
We might as well remove it, thanks for noticing!
| TabIndex="2" | ||
| Visibility="{x:Bind ViewModel.ShowToolbar, Mode=OneWay}" /> | ||
|
|
||
| <InfoBar |
There was a problem hiding this comment.
Is there a chance we'll want to reuse this info bar for other purposes as well? If so, we'll want to make it more generic with support for customizing the content and action.
There was a problem hiding this comment.
I'm not sure. Since there's a Button specific to this scenario, I don't think we can reuse this InfoBar.
There was a problem hiding this comment.
Sounds good for now, just something to keep in mind for the future.
|
|
||
| private async void OpenNetworkAdvancedSettingsButton_Click(object sender, RoutedEventArgs e) | ||
| { | ||
| await Windows.System.Launcher.LaunchUriAsync(new Uri("ms-settings:network-advancedsettings")); |
There was a problem hiding this comment.
It would be better to have this directly enable it for the current network like Windows File Explorer does or go to ms-settings:network-advancedsharing as that where it can be enabled.
There was a problem hiding this comment.
This would be ideal, but we should also consider the risks of modifying a setting in Windows from Files.
There was a problem hiding this comment.
Not sure what risk there would be if it's done the same way as Windows File Explorer.
There was a problem hiding this comment.
File Explorer is signed by Windows, which means auto-elevation is supported by default. However Files is not signed by Windows so it requires to prompt a UAC dialog. I found the implementation of this pop bar in Explorer and did the same, it didn't work for Files (it did work on CLI though). I'm not sure it's that worth it to look into deeper.
There was a problem hiding this comment.
I've discovered that if you create a Task Scheduler task with "Run with highest privileges" enabled, you can launch certain processes with administrative privileges simply by invoking that task.
There was a problem hiding this comment.
We have an exact process for that purpose in Files.App.Server but it's not currently working (works but doesn't work for a certain case; when the consumer is launched as admin). When it gets resolved, we would certainly should add an option to directly enable this.
But for the time being, we should just let users open Settings and enable it.
There was a problem hiding this comment.
In that case, it can be change to go to ms-settings:network-advancedsharing as it's more direct way to enable it.
|
Just a slight aesthetic nit pick. Could you please adjust the corner radius for the InfoBar so it matches the pane rounding, and the toolbar rounding. |
yair100
added
ready for review
Resolved / Related Issues
Steps used to test these changes
Settings > Network & internet > Advanced network settings > Advanced sharing settingsNetworkfrom the sidebar