Make all internal types in Common internal OR public

[pgrawehr]

This cleans up a few dependency issues between the bindings and the common project. The classes marked internal (including the Interop classes) are now public when building CommonHelpers.csproj but still internal for the final assembly. This removes the need to link to individual source files outside their binding folders, which causes build problems when they're used from multiple places.

Microsoft Reviewers: Open in CodeFlow

[krwq]

consider putting that directly in the cs files which do that, we want to get this warning for any case where we don't have good justification (#pragma warning disable SA1205 // Partial elements should declare access modifier: We use different visibility for interop individual projects and for published package, see XYZ.cs)

[pgrawehr]

I would need to disable it in many files in the common project, which would somewhat jeopardize the nice solution with not specifying a visibility for the internal classes.

As the documentation says: Because all files are also compiled using a separate project, the warning does come up for classes not in the common project.

[krwq]

consider following class structure:

partial class Interop
{
  public class Videodev2 { /* ... */ }
}

[pgrawehr]

That does not work here, because the class Interop is public while building the binding and resides in a different assembly. Partial classes cannot be merged from two assemblies.

[joperezr]

Perhaps then consider moving this file over to the Common project? I do feel like having a central place for all of the Interop code makes sense.

[pgrawehr]

The problem here is that these classes depend on public(!) enums in the Media project. I could move them, too, but then they would be a bit on the odd side.

[krwq]

is enum internal? if so it's also interop related so should be moved, if it's public let's leave as is

[pgrawehr]

No, that's what I'm saying: Those enums are public. (I did not test whether they are rightfully so, though)

[krwq]

LGTM after addressing comments

[joperezr]

NIT: Indenting on this comment.

[joperezr]

Usually when we we use infrastructure to automatically Opt-in all of the child projects into, it is good practice to provide an Opt-out switch, for example, a property like Condition="'$(OmitCommonProjectReference)' != 'true'" or something like that. If you do that, then instead of having to use the Condition you have above in the ItemGroup, you can just set those properties onto the Common and System.Device.Model projects themselves which would remove the ProjectReference from there. You don't have to make that change now, and I'm fine to keeping this as is, just wanted to share that other point of view.

[joperezr]

Honestly I don't love the fact of separating access modifiers from their implementation using partials. Given you would only have one instace per file, why not instead just add the ifdef on the class implementation itself to select if the class is either public or itnernal? I think having it separate like this will be harder to maintain and prone to errors.

[joperezr]

FWIW, we are already using that approach with at least three helper classes in common, I think we should use the same with the rest.

[pgrawehr]

This was @krwq's suggestion, and I think it's nice. It reduces the boilerplate on many files, and I think it reduces the possibility of errors. One would clearly not by accident change this file, but when one changes one of the other files (and i.e. adds an access specifier there) a build error would result. We could make this a bit more waterproof by anyway adding the explicit internal case.

[joperezr]

Thanks so much for all of this great cleanup @pgrawehr! I left some minor comments, and the only major one is on how to execute the visibility of the common classes, but other than that this looks great!

[krwq]

@joperezr I think for this specific case we should vote on the design - I personally prefer cleaner code (less #ifs).

Please vote:

• 🚀 for less #ifs
• 🎉 for explicit visibility (with #ifs)

Time until 1/31 EOD or all triage members voting whichever comes sooner.

[krwq]

Either way I agree with offline comment - if we decide to keep Visibility file we should be always explicit to make sure there is compile error on accidental change (also create Visibiltity.Interal.cs)