search

madskristensen / Tweakster

A Visual Studio extension
Apache License 2.0
238 stars 23 forks source link

Port to Community Toolkit - Regression #78

Closed StevenBonePgh closed 3 years ago

StevenBonePgh commented 3 years ago

Not sure where to raise this one, but the issue is really now in the Community Toolkit. In that, the BaseOptionModel<T> class blindly assumes the underlying type is a string. The Visual Studio Extensions window reports the following in my system (after a small tweak to the exception logging to accept a formatted string):

BaseOptionModel<Tweakster.Options>.LoadAsync Scope:UserSettings CollectionName:Tweakster.Options Name:U2DCheckVerbosity PropertyType:System.Boolean Value:[NULL]
System.ArgumentException: Value does not fall within the expected range.
   at System.Runtime.InteropServices.Marshal.ThrowExceptionForHRInternal(Int32 errorCode, IntPtr errorInfo)
   at Microsoft.VisualStudio.Shell.Settings.ShellSettingsStore.GetString(String collectionPath, String propertyName)
   at Community.VisualStudio.Toolkit.BaseOptionModel`1.<LoadAsync>d__11.MoveNext()
BaseOptionModel<Tweakster.Options>.LoadAsync Scope:UserSettings CollectionName:Tweakster.Options Name:ShowBuildOrder PropertyType:System.Boolean Value:[NULL]
System.ArgumentException: Value does not fall within the expected range.
   at System.Runtime.InteropServices.Marshal.ThrowExceptionForHRInternal(Int32 errorCode, IntPtr errorInfo)
   at Microsoft.VisualStudio.Shell.Settings.ShellSettingsStore.GetString(String collectionPath, String propertyName)
   at Community.VisualStudio.Toolkit.BaseOptionModel`1.<LoadAsync>d__11.MoveNext()

My earlier PR added the attributes OverrideCollectionNameAttribute and OverrideDataTypeAttribute and these were checked for and honored by BaseOptionModel. While the attributes remain in Tweakster, the functionality of them is, of course, missing because these do not exist in the Community Toolkit.

The combination of these attributes and functionality honoring them in BaseOptionModel enables an extension author to tweak a setting elsewhere in the hive. Do you think it would be acceptable to create a PR moving these attributes and implementation to the Community Toolkit, followed by a fix for this here once that PR is accepted?

StevenBonePgh commented 3 years ago

I put in a PR that moves these to the Community Toolkit. Seems like a good fit there and solves a problem that lets me upgrade my homebrewed extension to use the toolkit. Once accepted, I'll fix the regression with a PR here.