Port the majority of the System.ComponentModel APIs into the open by AlexGhiondea · Pull Request #12081 · dotnet/corefx

AlexGhiondea · 2016-09-26T21:26:19Z

This is the initial PR for this work.

What does this change contain:

The reference source for the updated APIs. We have decided to put the new APIs in the System.ComponentModel.TypeConverter type for now. Further refactoring will move them around if necessary.
The source code corresponding to the introduced APIs.
I have removed the stubs introduced by the System.Threading.Timers works as those APIs are now fully exposed and implemented.

There are a couple of things missing:

There are APIs that the implementation uses that have not yet been exposed. For those I have created stubs in order to have the code compile. I will work on either filling those APIs or filing issues to track the addition of the API.
There are no tests yet as part of this change. Once all the tracking issues have been filed I will start on getting the tests open sourced

/cc @weshaggard @danmosemsft

danmoseley · 2016-09-26T21:47:35Z

+    "netstandard1.7": {
      "imports": [
-        "dotnet5.1"
+        "dotnet5.4"


"dotnet5.4 [](start = 7, length = 11)

I believe you can remove "imports" sections

danmoseley · 2016-09-26T21:48:48Z

+// <copyright file="Component.cs" company="Microsoft">
+//     Copyright (c) Microsoft Corporation.  All rights reserved.
+// </copyright>                                                                
+//------------------------------------------------------------------------------


I believe you should replace all comments of this sort with

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

I rand the codeformatter on the TypeConverter project, but I missed the files I added to the Primitives folder.

Argh -- the code formatter did not remove the existing copyright headers....

danmoseley · 2016-09-26T21:49:38Z

+//------------------------------------------------------------------------------
+//     Copyright (c) Microsoft Corporation.  All rights reserved.
+// </copyright>                                                                
+//------------------------------------------------------------------------------


all these need to go...

danmoseley · 2016-09-26T21:51:15Z

+{
+    using System;
+
+    /// <devdoc>


probably search and replace devdoc for summary and remove para ?

danmoseley · 2016-09-26T21:52:30Z

+    ///       Specifies a set of technologies designer hosts should support.
+    ///    </para>
+    /// </devdoc>
+    [System.Runtime.InteropServices.ComVisible(true)]


check with Wes, but I think we are stripping all ComVisible attributes.

danmoseley · 2016-09-26T21:58:23Z

-        protected virtual object GetService(System.Type service) { throw null; }
-        public override string ToString() { throw null; }
-    }
+    // public partial class Component : System.IDisposable


// [](start = 4, length = 2)

?

This has been pushed down into the Primitives.

danmoseley · 2016-09-26T22:00:26Z

+ * An object that can rollback edits.
+ * 
+ * Copyright (c) 1999 Microsoft Corporation
+ */


more to remove :)

danmoseley · 2016-09-27T00:13:43Z

Are you planning to include tests in this commit, or a subsequent one? I have a preference for the same commit unless there is a good reason (eg unblock others).

AlexGhiondea · 2016-09-27T03:37:03Z

I am planning to have them as part of the same PR, but probably in a separate commit to keep things nicer.

justinvp · 2016-09-27T15:36:57Z

+
+
+
+


Nit: Leading whitespace. Applies to many files in this PR.

This is a by-product of removing the old style copyright headers. I will see if the CodeFormatter tool can fix this. If it cannot, I am going to leave it as-is.

👎 we should fix them even if the codeformatter doesn't.

Fixed these.

@justinvp sorry if my previous comment came off wrong -- really appreciate your code reviews! :)

justinvp · 2016-09-27T15:38:16Z

+        ///     Initializes a new instance of the <see cref='System.ComponentModel.AddingNewEventArgs'/> class,
+        ///     with no new object defined.
+        /// </summary>
+        public AddingNewEventArgs() : base()


Nit: the : base() is unnecessary here and in the ctor below.

Agreed. This and the other commend you added below are good candidates for cleaning up after this large chunk of code is ported into the open.

justinvp · 2016-09-27T15:41:35Z

+    ///     one itself.
+    /// </summary>
+    [HostProtection(SharedState = true)]
+    public class AddingNewEventArgs : EventArgs


Nit: This class could be simplified...

public class AddingNewEventArgs : EventArgs { public AddingNewEventArgs() { } public AddingNewEventArgs(object newObject) { NewObject = newObject; } public object NewObject { get; set; } }

I am going to capture these and the related comments about improving the code into a separate issue that we can tackle after the code has been ported into the open.

justinvp · 2016-09-27T15:42:48Z

+
+
+/*
+ */


Nit: In addition to the leading whitespace, some files have these empty comments.

justinvp · 2016-09-27T15:44:25Z

+            return _bindable.GetHashCode();
+        }
+
+#if !SILVERLIGHT


Should the SILVERLIGHT ifdef be removed?

justinvp · 2016-09-27T15:46:06Z

+    using System.Security.Permissions;
+    using CodeAccessPermission = System.Security.CodeAccessPermission;
+
+    /// <include file='doc\BindingList.uex' path='docs/doc[@for="BindingList"]/*' />


Should the <includes> be removed?

Yup. Removed!

justinvp · 2016-09-27T16:20:54Z

+        private bool _raiseListChangedEvents = true;
+        private bool _raiseItemChangedEvents = false;
+
+        [NonSerialized()]


Nit: Could be [NonSerialized] (parens aren't needed).

Will leave as-is for now. We can clean up all the code (including the other suggestions) once the port is complete!

This was in a single file. Simple enough to change now.

justinvp · 2016-09-27T18:54:53Z

+            }
+        }
+
+        private static class CultureInfoMapper


This could be improved...

Benefits:

Avoids intermediate allocations as items are added to the dictionary by specifying the capacity

Avoids looking up the key twice by using TryGetValue

Avoids manual lazy initialization

Allows s_cultureInfoNameMap to be readonly

private static class CultureInfoMapper { // Dictionary of CultureInfo.DisplayName, CultureInfo.Name for cultures that have changed DisplayName over releases. // This is to workaround an issue with CultureInfoConverter that serializes DisplayName (fixing it would introduce breaking changes). private static readonly Dictionary<string, string> s_cultureInfoNameMap = CreateMap(); private static Dictionary<string, string> CreateMap() { const int Count = 274; var result = new Dictionary<string, string>(Count) { { "Afrikaans", "af" }, ... }; Debug.Assert(result.Count == Count); return result; } public static string GetCultureInfoName(string cultureInfoDisplayName) { string name; return s_cultureInfoNameMap.TryGetValue(cultureInfoDisplayName, out name) ? name : cultureInfoDisplayName; } }

AlexGhiondea · 2016-09-27T18:59:37Z

@justinvp Thanks for taking a look at this PR!

stephentoub

I skimmed through and left a bunch of comments. Many are nits and can be addressed as a cleanup if you like. Some are a bit more concerning, e.g .why BackgroundWorker is in here. Overall, good job.

stephentoub · 2016-09-27T21:48:20Z

+            {
+                return true;
+            }
+        }


Nit: you could make members like this more concise if you wanted to with expression-bodied members, e.g.

protected virtual bool CanRaiseEvents => true;

stephentoub · 2016-09-27T21:48:59Z

+    public class Component : /*TODO NETSTANDARD2.0 MarshalByRefObject, */IComponent 
+    {
+        /// <summary>
+        ///    <para>Static hask key for the Disposed event. This field is read-only.</para>


Nit: hask => hash

Could the <para> tags be removed?

They could -- it looks like we were not consistent across this repo ... so we have other places with these tags.

I will capture these in a code-clean-up issue.

stephentoub · 2016-09-27T21:50:17Z

+        {
+            get { return _site; }
+            set { _site = value; }
+        }


Nit: could just be:

public virtual ISite Site { get; set; }

I'll only comment on each such pattern once; I'm sure there are other such cases later on.

stephentoub · 2016-09-27T21:51:24Z

+                    if (_events != null)
+                    {
+                        EventHandler handler = (EventHandler)_events[s_eventDisposed];
+                        if (handler != null) handler(this, EventArgs.Empty);


Nit: With C# 6, this pattern can now be simplified to just:

((EventHandler)_events[s_eventDisposed])?.Invoke(this, EventArgs.Empty);

stephentoub · 2016-09-27T21:52:18Z

+            {
+                ISite s = _site;
+                return s == null ? null : s.Container;
+            }


Nit: Could be simplified with ?.

public IContainer Container => _site?.Container;

stephentoub · 2016-09-27T22:36:53Z

+                    {
+                        string[] segments = licenseFile.Segments;
+                        string licFileName = segments[segments.Length - 1];
+                        string key = licFileName.Substring(0, licFileName.LastIndexOf("."));


What if there's no period in licFileName?

stephentoub · 2016-09-27T22:37:49Z

+            try
+            {
+                WebClient webClient = new WebClient();
+                webClient.Credentials = CredentialCache.DefaultCredentials;


We should test this and make sure it behaves as expected (e.g. the credentials are a nop) on Unix.

stephentoub · 2016-09-27T22:41:50Z

+    [System.Security.Permissions.PermissionSetAttribute(System.Security.Permissions.SecurityAction.LinkDemand, Name = "FullTrust")]
+    public sealed class ContextStack
+    {
+        private ArrayList _contextStack;


Same general question as earlier about replacing the non-generic collections with generic ones wherever possible.

stephentoub · 2016-09-27T22:56:59Z

@@ -0,0 +1,70 @@
+D:\code\gitHub\alexghiondea\corefx\src\System.ComponentModel.TypeConverter\src\System\ComponentModel\ArrayConverter.cs


What is this file that has your username in it?

Accidentally checked that in. Removed.

stephentoub · 2016-09-27T23:00:52Z

+{
+    internal const string SystemDrawing = "System.Drawing, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a";
+    internal const string SystemDesign = "System.Design, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a";
+}


What are these used for? These assemblies don't exist in core.

Removed them from here. they are used inside DesignerAttribute and EditorAttributes. I moved the string literals directly there. We might want to remove them completely.

weshaggard · 2016-09-28T06:21:46Z

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="utf-8"?>


We no longer need depprojs'.

weshaggard · 2016-09-28T06:23:07Z

+        "dotnet5.8"
      ]
+    },
+    "netstandard1.0":


netstandard1.0 section is no longer needed.

weshaggard

Still lots of clean-up to do here.

weshaggard · 2016-09-28T06:23:42Z

+            "System.Resources.ResourceManager": "4.0.0"
+        },
      "imports": [
-        "dotnet5.1"


imports can be removed.

weshaggard · 2016-09-28T06:24:41Z

    <Project Include="System.ComponentModel.Primitives.csproj">
      <TargetGroup>net45</TargetGroup>
    </Project>
+    <Project Include="System.ComponentModel.Primitives.csproj">


YOu can remove all the builds from here except for the default and the net463 build.

weshaggard · 2016-09-28T06:25:06Z

    <AssemblyName>System.ComponentModel.Primitives</AssemblyName>
-    <IsPartialFacadeAssembly Condition="'$(TargetGroup)'=='net45'">true</IsPartialFacadeAssembly>
-    <NuGetTargetMoniker Condition="'$(TargetGroup)' == ''">.NETStandard,Version=v1.0</NuGetTargetMoniker>
+    <AssemblyVersion Condition="'$(TargetGroup)'=='net45' or '$(TargetGroup)'=='netstandard1.0'">4.1.1.0</AssemblyVersion>


net45 can be removed from this project.

weshaggard · 2016-09-28T06:26:03Z

+
+#if !netstandard17
+    private ComponentCollection() { }
+#else


Do we even need the #ifdefs? I would just eliminate the netstandard1.0 builds.

weshaggard · 2016-09-28T06:26:48Z

+        ///     Creates a new event handler list.  The parent component is used to check the component's
+        ///     CanRaiseEvents property.
+        /// </summary>
+        [SuppressMessage("Microsoft.Security", "CA2122:DoNotIndirectlyExposeMethodsWithLinkDemands")]


looks like no because it is related to security demands which we don't have.

weshaggard · 2016-09-28T06:27:40Z

+        "dotnet5.8"
+      ]
+    },
    "netstandard1.0": {


remove net45 and netstandard1.0

please do remove these dead configurations from here otherwise we do more package restoring for no reason.

weshaggard · 2016-09-28T06:28:18Z

        public override System.ComponentModel.PropertyDescriptorCollection GetProperties(System.ComponentModel.ITypeDescriptorContext context, object value, System.Attribute[] attributes) { throw null; }
        public override bool GetPropertiesSupported(System.ComponentModel.ITypeDescriptorContext context) { throw null; }
    }
-    public partial class Component : System.IDisposable


You need to leave a type-forward behind.

I will add it as a manual type-forward to S.CM.P

weshaggard · 2016-09-28T06:29:56Z

+
+
+
+


👎 we should fix them even if the codeformatter doesn't.

weshaggard · 2016-09-28T06:32:21Z

 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <ItemGroup>
+    <ProjectReference Include="..\ref\4.1.0\System.ComponentModel.Primitives.depproj">


Can you please have a read over my document that talks about how to version these libraries and make the necessary updates and/or leave comments on my PR (#12036) about followup questions.

weshaggard · 2016-09-30T05:19:27Z

@dotnet-bot test this please

weshaggard · 2016-09-30T05:20:20Z

    }
-    public partial class ComponentCollection
+    [System.ComponentModel.DesignerCategoryAttribute("Component")]
+    public partial class Component : /*TODO NETSTANDARD2.0 System.MarshalByRefObject,*/ System.ComponentModel.IComponent, System.IDisposable


Please make sure we have an issue tracking these MarshalByRefObject TODOs.

There are a few follow-ups and they are all captured in issue #12212

weshaggard · 2016-09-30T05:22:01Z

+    "netstandard1.7": {
+        "dependencies":
+        {
+            "System.Runtime": "4.3.0-beta-24522-03",


I really hope System.ComponentModel.Primitives doesn't actually depend on all these packages. Certainly the ref doesn't. Please trim this to the exact set of references that show up in the assembly reference table of the built binary.

weshaggard · 2016-09-30T05:23:01Z

+  <ItemGroup Condition="'$(TargetGroup)'==''">
+    <Compile Include="System\ComponentModel\Component.cs" />
+  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)'=='net45' or '$(TargetGroup)'=='net463'">


net45 condition isn't needed.

weshaggard · 2016-09-30T05:24:50Z

 namespace System.ComponentModel
 {
-    public class ComponentCollection
+    using System.Collections;


using statements should be outside of namespaces.

weshaggard · 2016-09-30T05:26:30Z

+        "System.Diagnostics.Tools": "4.3.0-beta-24522-03",
+        "System.Resources.ResourceManager": "4.3.0-beta-24522-03",
+        "System.Runtime": "4.3.0-beta-24522-03",
+        "System.Threading": "4.0.0"


Bump System.Threading package version up to the latest 4.3.0-beta-* version as well, otherwise you will be seeing downgrade warnings.

weshaggard · 2016-09-30T05:26:56Z

+        "System.Threading": "4.0.0"
+      },
+      "imports": [
+        "dotnet5.8"


imports should be deleted.

weshaggard · 2016-09-30T05:30:07Z

+// ------------------------------------------------------------------------------
+// Changes to this file must follow the http://aka.ms/api-review process.
+// ------------------------------------------------------------------------------
+


most projects have just been putting these in the main .cs file instead of a separate .Forwards file.

Moved into the main file.

weshaggard · 2016-09-30T05:31:21Z

+        public override bool Equals(object obj) { throw null; }
+        public override int GetHashCode() { throw null; }
+    }
+    // [System.ComponentModel.DefaultEventAttribute("DoWork")]


We should remove these instead of commenting them out.

Thought I got all of them but missed this one. Gone now.

weshaggard · 2016-09-30T05:32:00Z

+        public System.ComponentModel.InheritanceLevel InheritanceLevel { get { throw null; } }
+        public override bool Equals(object value) { throw null; }
+        public override int GetHashCode() { throw null; }
+        //TODO NETSTANDARD2.0 - expose IsDefaultAttribute on Attribute public override bool IsDefaultAttribute() { throw null; }


Ensure we have a follow-up item for this.

weshaggard · 2016-09-30T05:33:31Z

 {
  "dependencies": {
    "System.Collections.NonGeneric": "4.3.0-beta-24522-03",
+    "System.Collections.Specialized": "4.3.0-beta-24522-03",


Please trim this to the exact set that is needed. I really hope we don't need things like System.Security.Permissions for example.

weshaggard · 2016-09-30T05:33:58Z

    <value>The object used to marshal the event handler calls issued when an interval has elapsed.</value>
  </data>
+
+  <!-- To be moved -->


to be moved where?

uhm.. here? Removed the comment

weshaggard · 2016-09-30T05:36:08Z

+using System.ComponentModel;
+using System.Reflection;
+
+namespace System


Ensure we have a follow-up item for removing all these stubs.

weshaggard · 2016-09-30T05:36:57Z

+
+namespace System.Resources
+{
+    public partial interface IResourceReader : System.Collections.IEnumerable, System.IDisposable


@alexperovich be aware this is going to cause conflicts with your resource work.

Why does this need to be here? Can it not just reference System.Resources.Reader?

If that is available, it should absolutely reference that!

weshaggard · 2016-09-30T05:38:33Z

+        "System.IO": "4.3.0-beta-24522-03",
+        "System.IO.FileSystem": "4.3.0-beta-24522-03",
        "System.Linq": "4.3.0-beta-24522-03",
+        "System.Net.WebClient": "4.3.0-beta-24522-03",


We should file a follow-up item about breaking this dependency on WebClient, especially if it is tied to the license stuff which we aren't likely to fully support anyway.

weshaggard

I've added a bunch of clean-up and follow-up comments but I don't see anything that is actually broken. Please be sure to follow-up on the clean-up.

This change allows types higher in the stack to use the type. The build is also adjusted to allow for building and testing against netstandard1.7

…tModel.TypeConverter.

Port the majority of the System.ComponentModel APIs into the open Commit migrated from dotnet/corefx@f94dd87

dnfclas added the cla-already-signed label Sep 26, 2016

danmoseley reviewed Sep 26, 2016

View reviewed changes

AlexGhiondea force-pushed the PortSysCompMod branch from 5340963 to 5b29529 Compare September 26, 2016 22:38

justinvp reviewed Sep 27, 2016

View reviewed changes

stephentoub approved these changes Sep 27, 2016

View reviewed changes

weshaggard reviewed Sep 28, 2016

View reviewed changes

weshaggard suggested changes Sep 28, 2016

View reviewed changes

AlexGhiondea force-pushed the PortSysCompMod branch 2 times, most recently from ac8e806 to e2ffe37 Compare September 29, 2016 17:25

weshaggard reviewed Sep 30, 2016

View reviewed changes

weshaggard approved these changes Sep 30, 2016

View reviewed changes

AlexGhiondea force-pushed the PortSysCompMod branch from e2ffe37 to c86453e Compare September 30, 2016 13:52

AlexGhiondea added 2 commits September 30, 2016 07:20

Move the Component type into System.ComponentModel.Primitives.

52f8b5d

This change allows types higher in the stack to use the type. The build is also adjusted to allow for building and testing against netstandard1.7

Port the additional System.Component model types into System.Componen…

09d2d1b

…tModel.TypeConverter.

AlexGhiondea force-pushed the PortSysCompMod branch from c86453e to 09d2d1b Compare September 30, 2016 14:20

AlexGhiondea merged commit f94dd87 into dotnet:master Sep 30, 2016

karelz modified the milestone: 1.2.0 Dec 3, 2016

justinvp mentioned this pull request Jun 5, 2017

Add query notification support to SqlClient #20708

Merged

AlexGhiondea mentioned this pull request Jan 31, 2020

Code cleanup for System.ComponentModel.TypeConverter dotnet/runtime#18754

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#12081 from AlexGhiondea/PortSysCompMod

361a5aa

Port the majority of the System.ComponentModel APIs into the open Commit migrated from dotnet/corefx@f94dd87

		@@ -0,0 +1,70 @@
		D:\code\gitHub\alexghiondea\corefx\src\System.ComponentModel.TypeConverter\src\System\ComponentModel\ArrayConverter.cs

Conversation

AlexGhiondea commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Sep 27, 2016

Uh oh!

AlexGhiondea commented Sep 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexGhiondea commented Sep 27, 2016

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley Sep 26, 2016 •

edited

Loading

justinvp Sep 27, 2016 •

edited

Loading

justinvp Sep 27, 2016 •

edited

Loading