Add firewall application along with other small improvements.

[labre-rdc]

Changes proposed in this pull request

• Add firewall application.
• Fix a few potential null dereferences in some validators and converters.
• Add ValidationErrorTemplate for checkboxes.
• Allow ValidationError to be clicked out of the way (restores on hovering/keyboard focus).
• Fix ChildWindowStyle restricting the window size to 500 height instead of setting a default height.
• Reuse existing validators in empty variants where applicable.
• Make ListHelper.Modify generic. Avoid duplicates.
• PowershellHelper: Support commands exceeding command line length limit.
• NetworkInterfaceView: Fix all XAML warnings.
• ProfileChildWindow: Always scale to 85% of main window width. Required for wider content.
• Localized enum conversion (to/from int/string)
• Fixed 2 warnings in ProfileView while being at it.

Related issue(s)

• Fixes [New App] Firewall module/application #3259

Copilot generated summary

Provide a Copilot generated summary of the changes in this pull request.

Copilot summary

{generated summary}

To-Do

• Update documentation to reflect this changes
• Update changelog to reflect this changes
• Replace PR number placeholders in changelog.

Contributing

By submitting this pull request, I confirm the following:

• I have read and understood the contributing guidelines and the code of conduct.
• I have have added my name, username or email to the contributors list or don't want to.
• The code or resource is compatible with the GNU General Public License v3.0.

[BornToBeRoot]

@labre-rdc thanks, i will review it when i have some time 😄

[labre-rdc]

Sorry for the reference error. I’ll handle it tomorrow.

[BornToBeRoot]

No worries. I don't think I'll be able to review it until next week.

[labre]

Thanks for the review. I'm currently in vacation until excluding Friday.

Regarding localization: I assumed, that I can (at least) add keys as required, but apparently they are supposed to be added within Transifex. Reverting that commit was not enough, because I already tried to rebase/merge as required to stay out of merge conflicts. I will remove the keys and try to provide something importable for Transifex. Can you point me to some documentation on supported formats for key imports? I can provide them with German localization or without it. Just state your preference.

Since real testing (and screenshots) was not really possible without language changes, the commit was at least temporarily required.

[BornToBeRoot]

The default strings (English) must be added to “Strings.resx” (and “Strings.Designer.cs”). Transifex regularly checks for changes in this file in the repository. When changes are made, Transifex is updated and the new strings can be translated.
When a language is fully translated (or during a manual synchronization), Transifex sends a PR with the file Strings..resx, which is automatically validated by AppVeyor and then merged by Mergify.

I only commit these two files and discard the rest (Visual Studio automatically updates them with an empty string...).

---

But I restored it and pushed it to this branch, so you don't need to make any changes to the language 😄 I'll try to review the rest over the weekend. I think i will make some minor changes to the UI to align it with the other tools (like moving the buttons to the bottom so it will also resize correct)

[labre-rdc]

The default strings (English) must be added to “Strings.resx” (and “Strings.Designer.cs”). Transifex regularly checks for changes in this file in the repository. When changes are made, Transifex is updated and the new strings can be translated. When a language is fully translated (or during a manual synchronization), Transifex sends a PR with the file Strings..resx, which is automatically validated by AppVeyor and then merged by Mergify.

I only commit these two files and discard the rest (Visual Studio automatically updates them with an empty string...).

I see. Well I considered it unreliable for testing, because the empty strings did apparently not fall back to the default culture for me in WPF (maybe I missed something), which means I was presented with empty buttons and headers when a non-English display language was used. For that reason I added the English translation and translated to German while being at it. This was also reasonable, because the UI elements would scale/wrap differently with other string lengths etc. As a side note, I think, you use Visual Studio in German language, because Strings.Designer.cs contains auto-generated german comments. This causes merge conflicts, when other contributors use IDEs in different languages.

But I restored it and pushed it to this branch, so you don't need to make any changes to the language 😄 I'll try to review the rest over the weekend. I think i will make some minor changes to the UI to align it with the other tools (like moving the buttons to the bottom so it will also resize correct)

Okay, fine for me, if you want to handle that. I might process a few of the Copilot review notes today or in the next few days.

[labre-rdc]

Here is the Demo profile I used for the screenshots. Should you wish to do other style changes (maybe even after the merge), you can update the screenshots accordingly.

Demo.json

[BornToBeRoot]

Sorry—I just got around to writing this. Thanks again. I’ll update the screenshots and documentation as soon as this is implemented.

After reviewing the code and experimenting a bit with the firewall module, I noticed a few minor bugs:

• If you switch between views (Firewall -> something else -> Firewall) and Firewall_LocalPortsHistoryConfig is null, a NullReferenceException occurs
• In some cases, a profile’s firewall rules are overwritten (I think they aren’t passed as a value—but I don’t remember the steps to reproduce this)
• The validation isn’t optimal either (editing a row in a DataGrid sometimes behaves strangely in WPF...). You can apply invalid rules, and the PowerShell command is then generated based on the input.

Anyway—before we spend time fixing this and reviewing the rest of the code, I’ve been thinking about tweaking the design a bit to make the behavior similar/consistent with the tools we already have (e.g., HostsFileEditor, ARP Table). Don't take this as criticism—I'm sure there were good reasons for implementing it this way in your project. I just want to keep the user experience consistent across all tools.

I think the HostsFileEditor is a good reference here:

• The “Main” view should display the currently active rules (similar to the Hosts file editor, which shows what is currently in the Hosts file...). In the current behaviour you apply rules, then delete them (don't apply), restart NetworkManager, and the rules remain active but are no longer displayed in the view.
• By adding/editing individual rules via a dialog, we could improve validation -> E.g. enable the OK button when validation passes / show a hint and message with the validation error (this is broken in DataGrid in WPF)
• I think it should be possible to delete a single rule without affecting the others—currently, all rules are deleted and then reapplied (theoretically, all active connections are dropped—I haven’t tested this) -> maybe someone uses this to manage the rules on their web server and then this would affect users.
• And i think we won't need a local history or the option to change the syntax... All other tools use ; to seperate ports - we can just "translate" them to the correct synatx we need for PowerShell... (this could also introduce other bugs -> a profile is saved with ; and then the syntax is changed... the PortRangeToPortSpecificationConverter will fail?)
• EDIT 1:
  I think all rules can start with "NETworkManager - *" - if i look at the names in the Windows firewall settings, this should be fine. And you know directly who created it (without looking into the docs to see we use the NwM_ syntax...)

I’m not sure how to design a view for the profiles... but I would do that after reworking the main view / adding a dialog.

The idea in Paint -.-

I will start to work on this - You're welcome to help out if you'd like.

[labre]

Sorry—I just got around to writing this. Thanks again. I’ll update the screenshots and documentation as soon as this is implemented.

Sure, thanks for your effort.

After reviewing the code and experimenting a bit with the firewall module, I noticed a few minor bugs:

* If you switch between views (Firewall -> something else -> Firewall) and `Firewall_LocalPortsHistoryConfig` is null, a `NullReferenceException` occurs

I could have overlooked that one, but maybe it was also introduced by the refactoring. The VS refactoring tools did break de-/serialization already 3 years ago by removing public modifiers for properties being serialized. Does not seem to have changed.

* In some cases, a profile’s firewall rules are overwritten (I think they aren’t passed as a value—but I don’t remember the steps to reproduce this)

The profiles firewall rules are always overwritten when a profile is selected and the rule list is changed, as documented, because I considered it bad UX design to drop user settings when reapplying the selected profile (for instance). I like that behavior. It might not align with the behavior in other applications though. From the tip of my mind, I would say, that you can easily remove this behavior in the FirewallViewModel within the RuleChangedEventHandler. It needs to persist for changes of the settings (without any profiles available). Otherwise rules would not be stored without profiles.

* The validation isn’t optimal either (editing a row in a DataGrid sometimes behaves strangely in WPF...). You can apply invalid rules, and the PowerShell command is then generated based on the input.

I actually tested it quite thoroughly for invalid input. Can you give an example, how the (logged) PowerShell command did include invalid rule’s configuration? Before sending the PR, invalid data never reached the model. That is the concept. So you can configure invalid rules and they get marked red, but the last valid setting is still stored in the model. The only rules being skipped are program rules, where the program has been deleted outside of NETworkManager’s control when applying them (they are marked invalid in this case). Did you just check, whether a rule marked invalid is created? They are still valid, otherwise the Windows Firewall would not create them.

Anyway—before we spend time fixing this and reviewing the rest of the code, I’ve been thinking about tweaking the design a bit to make the behavior similar/consistent with the tools we already have (e.g., HostsFileEditor, ARP Table). Don't take this as criticism—I'm sure there were good reasons for implementing it this way in your project. I just want to keep the user experience consistent across all tools.

I see your point and no offense taken. The main reason I deviated from the main UX design was to give a good example on how to improve things. For instance, the RuleGrid is based on IPScanner’s DataGrid, but the Chevron button contrast has been increased. The invalid rule style was added. Double clicks can be used to expand and collapse rows. Its details view contains configuration, which would be also a nice view for the configuration of multiple network interfaces. The RuleGrid is reused in the ProfileChildWindow. And so on.

I think the HostsFileEditor is a good reference here:

* The “Main” view should display the currently active rules (similar to the Hosts file editor, which shows what is currently in the Hosts file...). In the current behaviour you apply rules, then delete them (don't apply), restart NetworkManager, and the rules remain active but are no longer displayed in the view.

Yes, I thought about checking the current state of enabled rules and display, e.g., a column with checkbox, which would also allow to enable/disable individual rules in a configuration, however I considered it an extension, so for now the module just displays the settings (without profiles) or profile settings. Displaying rules not being in the
selected profile would also not make any sense, would it? With a boolean column and the Firewall model being able to check and show the enabled NETworkManager rules this could be done though. I considered it unnecessary work (for now).

* By adding/editing individual rules via a dialog, we could improve validation -> E.g. enable the OK button when validation passes / show a hint and message with the validation error (this is broken in DataGrid in WPF)

Yes, that seems simple, but the Windows Firewall does not work like that (see above). A rule might invalidate later. The invalid rules are already marked by a read border. The invalid fields are already marked with the ValidationErrorTemplate. I do not see the improvement.

* I think it should be possible to delete a single rule without affecting the others—currently, all rules are deleted and then reapplied (theoretically, all active connections are dropped—I haven’t tested this) -> maybe someone uses this to manage the rules on their web server and then this would affect users.

Sure, that is an improvement, which I considered as a later addition. (see above)

* And i think we won't need a local history or the option to change the syntax... All other tools use `;` to seperate ports - we can just "translate" them to the correct synatx we need for PowerShell... (this could also introduce other bugs -> a profile is saved with `;` and then the syntax is changed... the `PortRangeToPortSpecificationConverter` will fail?)

I tested that one also quite thoroughly. The history separator is always replaced, when that option is changed. There were indeed various bugs around that option, but I’m quite sure I’ve caught all of them in the meantime. This is not for the PowerShell, but for the user. If users are used to the WF.msc interface, they might prefer ',' as a separator, if they are used to NETworkManager’s separator, they prefer ';'. So I considered, that there is no valid default for all users and gave them the option. The default is NETworkManager’s syntax anyway. If you find a bug, leave me a note. Nevertheless I would also keep fixing bugs within this application, if they are reported. (So, don’t worry, be happy;)

* EDIT 1:
  I think all rules can start with "NETworkManager - *" - if i look at the names in the Windows firewall settings, this should be fine. And you know directly who created it (without looking into the docs to see we use the `NwM_` syntax...)

The disadvantage of this is that the Name column will never contain visible helpful information unless you expand it. The auto-generated names currently contain the whole rule configuration in a compressed way, which is also helpful for the view in WF.msc. So that was the idea of keeping it short, but if the long variant looks better for you, do not mind me.

I’m not sure how to design a view for the profiles... but I would do that after reworking the main view / adding a dialog.

You have the FirewallRuleGrid UserControl. The child window would be handled in the FirewallViewModel, so the profile view is exactly the same, no matter what you change. This PR is maintenance friendly.

The idea in Paint -.-

I will start to work on this - You're welcome to help out if you'd like.

I probably will. I really did not want to create a lot of work for you. I do not mind, if you see things differently. So, if you want to rework this, I do not object. I just wanted to point out, why the application is coded like this.

Have a nice evening. :)