Save startup object correctly; show correct dropdown entries. #9300
Conversation
- Don't show an entry for 'My.MyApplication' value. That is only set in the project file but should not be surfaced to the UI. Value should come from the myapp file. - Show correct StartupObject property. Seems like we had 2 properties, one in the base Application.xaml rule and another in the vb-specific Application.xaml rule. Ensured that they have different names and have a visibility condition that would show/hide the correct one, depending on the project language. The only thing missing is to ensure the VB WinForms template contains the correct value in the StartupObject: right now it has 'Sub Main', but should be '[rootnamespace].My.MyApplication'.
ghost
added
Feature-Project-Properties-Designer
|
@KlausLoeffelmann for the last point, to change the project file on the VB WinForms template, is that set up here: https://github.com/dotnet/winforms/blob/3d2eea7ac142d4d459f3672c32a7e74bdde45612/pkg/Microsoft.Dotnet.WinForms.ProjectTemplates/content/WinFormsApplication-VisualBasic/Company.WinFormsApplication1.vbproj#L8? |
|
just noticed Klaus is OOF; @merriemcgaw, do you know if that's the case? 🙂 |
Yes, it is. |
| @@ -94,6 +94,9 @@ public async Task<ICollection<IEnumValue>> GetListedValuesAsync() | |||
| })); | |||
| } | |||
|
|
|||
| // Remove My.MyApplication entry if any. | |||
| enumValues = enumValues.Where(ep => !ep.Name.Contains("My.MyApplication")).ToList(); | |||
There was a problem hiding this comment.
Technically this code runs for C# too. It seems unlikely that would ever be an issue.
This can also be slightly more efficient if you use List.RemoveAll:
| enumValues = enumValues.Where(ep => !ep.Name.Contains("My.MyApplication")).ToList(); | |
| enumValues.RemoveAll(ep => !ep.Name.Contains("My.MyApplication")); |
It's more efficient because it doesn't allocate a new list, and if nothing matches the predicate then no memory is modified at all, whereas the previous code would always create a full copy.
There was a problem hiding this comment.
Technically this code runs for C# too. It seems unlikely that would ever be an issue.
That's what I thought too, this value only makes sense in VB projects. These entries come from IEntryPointFinderService from Roslyn; it might be the case that we need to make the change there instead of here, but I'm not sure.
There was a problem hiding this comment.
It's definitely worth investigating whether the filtering should live on the Roslyn side. Ideally we don't get involved in these kinds of language level policies.
...ectSystem.Managed/ProjectSystem/Rules/PropertyPages/ApplicationPropertyPage.VisualBasic.xaml
Outdated
Show resolved
Hide resolved
...lStudio.ProjectSystem.Managed/ProjectSystem/Rules/PropertyPages/ApplicationPropertyPage.xaml
Outdated
Show resolved
Hide resolved
...ectSystem.Managed/ProjectSystem/Rules/PropertyPages/ApplicationPropertyPage.VisualBasic.xaml
Outdated
Show resolved
Hide resolved
...ectSystem.Managed/ProjectSystem/Rules/PropertyPages/ApplicationPropertyPage.VisualBasic.xaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good to me also, so far.
Just one thing to keep in mind in the context of the templates:
Most of projects out there do have "Sub Main" currently as the start object setting. So, while I of course agree that we should change the template, if there is a Sub Main, but the Application Framework is enabled, we need to "migrate" the setting to something meaningfull, and work with the assumption, that if ApplicationFramework is enabled, then application.myapp is the source of truth, and we update the start object in vbproj on the next opportunity.
Avoid allocating a new list by using List.RemoveAll. There's no need to have different properties: the VB-specific file replaces the base one and it is only included for VB projects, so we can also remove the visibility condition. Unfortunately, this reverts the changes that caused the localization files to change but doesn't reset their status to translated.
That's a good point. I have tested this scenario and we will handle this correctly: Here's a new VB WinForms: The selected startup object is Adding a second form and selecting that in the UI would cause these changes: That would ensure the project file gets the correct value if the startup object is changed, as well as ensure the value is saved in the myapp file since the application framework property is enabled. 🙂 |
Fixes #9262
Seems like we had 2 properties, one in the base Application.xaml rule and another in the vb-specific Application.xaml rule. Ensured that they have different names and have a visibility condition that would show/hide the correct one, depending on the project language.Interceptor had the wrong property nameInterceptedStartupObjectProperty; it now uses the correct property name.The only thing missing is to ensure the VB WinForms template contains the correct value in the StartupObject: right now it has 'Sub Main', but should be '[rootnamespace].My.MyApplication'.
Microsoft Reviewers: Open in CodeFlow