Move Metadata and LicenseInfo so they can be accessed independent of Windows Forms #1478

aror92 · 2025-11-12T17:54:23Z

Winforms-independent functionality of Metadata, CreativeCommonsLicense, and CustomLicense is moved to the new classes MetadataBare, CreativeCommonsLicenseInfo, and CustomLicenseInfo in SIL.Core.Clearshare.

Metadata, CreativeCommonsLicense, and CustomLicense inherit from the Winforms-free versions.

Make LicenseInfo class independent of Windows Forms and move it to SIL.Core.Clearshare.
- Move the FromXmp method to LicenseUtils and LicenseWithImageUtils to construct Winforms-independent and Winforms-dependent license types respectively.
- Move the FromToken method to LicenseWithLogo, so it can return the Winforms-dependent license types. FromToken is only used in libpalaso tests and examples, and a Winforms-independent version of this method is not needed.
- Move the GetImage method to the ILicenseWithImage interface, which is implemented by CreativeCommonsLicense and CustomLicense.

This change is

Move Metadata and LicenseInfo from SIL.Windows.Forms.ClearShare to SIL.Core.ClearShare. Create an interface "ILicenseWithImage" in Windows.Forms.ClearShare to handle the GetImage function. Create base classes "CreativeCommonsLicenseWithoutImage" and "CustomLicenseWithoutImage" in SIL.Core.ClearShare that LicenseInfo can use without Winforms dependency. And subclass them in SIL.Windows.Forms.ClearShare to add Image related methods. Subclasses keep the original names(CreativeCommonsLicense and CustomLicense) to minimize disruption to Bloom.

Update metadata test so it doesn't require System.Drawing Update version of L10NSharp

Autoformatting messed up a few comments because it mis-identified which lines of code the comments were associated with.

Use CreativeCommonsLicense and CustomLicense for the superclasses and add "WithImage" specification to the subclasses with images.

github-actions · 2025-11-12T18:05:52Z

Palaso Tests

4 files ± 0 4 suites ±0 11m 10s ⏱️ + 1m 24s
5 092 tests +22 4 858 ✅ +22 234 💤 ±0 0 ❌ ±0
16 591 runs +66 15 870 ✅ +66 721 💤 ±0 0 ❌ ±0

Results for commit f4e2040. ± Comparison against base commit ae035eb.

This pull request removes 32 and adds 54 tests. Note that renamed tests count towards both.

SIL.Windows.Forms.Tests.ClearShare.LicenseInfoTests ‑ FromToken_Ask_GiveCustomLicense
SIL.Windows.Forms.Tests.ClearShare.LicenseInfoTests ‑ FromToken_Ask_GiveNullLicense
SIL.Windows.Forms.Tests.ClearShare.LicenseInfoTests ‑ FromToken_CreativeCommons_GiveExpectedAttributes
SIL.Windows.Forms.Tests.ClearShare.LicenseInfoTests ‑ Token_GivenCreativeCommonsLicense
SIL.Windows.Forms.Tests.ClearShare.LicenseInfoTests ‑ Token_GivenCustomLicense_IsCustom
SIL.Windows.Forms.Tests.ClearShare.LicenseInfoTests ‑ Token_GivenNullLicense_IsAsk
SIL.Windows.Forms.Tests.ClearShare.MetadataTests ‑ ChangeLicenseDetails_HasChanges_True
SIL.Windows.Forms.Tests.ClearShare.MetadataTests ‑ ChangeLicenseObject_HasChanges_True
SIL.Windows.Forms.Tests.ClearShare.MetadataTests ‑ GetCopyrightBy_Empty_ReturnsEmpty
SIL.Windows.Forms.Tests.ClearShare.MetadataTests ‑ GetCopyrightBy_HandlesMultilineCopyrightHolder
…

SIL.Tests.ClearShare.MetadataCoreTests ‑ CanRemoveRightsStatment
SIL.Tests.ClearShare.MetadataCoreTests ‑ ChangeLicenseDetails_HasChanges_True
SIL.Tests.ClearShare.MetadataCoreTests ‑ ChangeLicenseObject_HasChanges_True
SIL.Tests.ClearShare.MetadataCoreTests ‑ ChangingFromCCLicense_WorksOkay
SIL.Tests.ClearShare.MetadataCoreTests ‑ DeepCopy
SIL.Tests.ClearShare.MetadataCoreTests ‑ GetCopyrightBy_Empty_ReturnsEmpty
SIL.Tests.ClearShare.MetadataCoreTests ‑ GetCopyrightBy_HandlesMultilineCopyrightHolder
SIL.Tests.ClearShare.MetadataCoreTests ‑ GetCopyrightBy_HasCOPYRIGHTAndSymbolNoYear_ReturnsCopyrightHolder
SIL.Tests.ClearShare.MetadataCoreTests ‑ GetCopyrightBy_HasCopyrightAndSymbolAndComma_ReturnsCopyrightHolder
SIL.Tests.ClearShare.MetadataCoreTests ‑ GetCopyrightBy_HasCopyrightAndSymbolNoYear_ReturnsCopyrightHolder
…

♻️ This comment has been updated with latest results.

SIL.Core.Tests/ClearShare/MetadataBareTests.cs

SIL.Windows.Forms.Tests/ClearShare/MetadataTests.cs

andrew-polk · 2025-11-12T18:27:48Z

Not worth a lot of head time, but worth a question:
Is it better to release 17.0 before this goes in so clients can decide to take the l10nsharp breaking change without this breaking change? Or is it better to include two breaking changes together in one major version release?

Bloom, getting ready to start a full regression test pass before releasing, would probably prefer to do so on a version without this change. (We already have the l10nsharp change.) But we are currently just using the beta version, so I'm not sure it matters much.
Where it could potentially matter is if we needed to hot-fix 17.0. (But we don't really have a build system set up to do that anyway, do we??)

SIL.Core.Tests/ClearShare/MetadataBareTests.cs

andrew-polk · 2025-11-12T18:39:04Z

SIL.Core/SIL.Core.csproj line 24 at r2 (raw file):

    <PackageReference Include="Mono.Unix" Version="7.1.0-final.1.21458.1" />
    <PackageReference Include="L10NSharp" Version="9.0.0-*" />
    <PackageReference Include="TagLibSharp" Version="2.3.0" />

I wonder if there are implications of adding these libraries we need to think through.

Increases size of SIL.Core's dependencies by about 0.6MB (probably ok...).
We worked hard at some point in the past to remove L10NSharp from SIL.Core. Probably that was just because of the WinForms dependency? But I don't remember if there were other reasons. It would be a bit unfortunate to add the dependency just for a couple translations of license messages. Have you considered using Localizer instead?

Another option which likely isn't worth it is to put all this clearshare stuff in a new package called SIL.Core.Clearshare or SIL.Clearshare

tombogle

True. It looks like the package publishing step itself would be able to grab the tag with the version number off whatever branch it was building on, but the build itself runs only on master. I imagine that in a pinch we could make the necessary changes to support a hotfix, but apparently the need has never yet arisen.
I'm ambivalent. Whenever I update, I pretty much always go to the latest release and just make whatever changes are needed. It looks like not a lot of downstream software uses these things, so I think the impact will be pretty low.

@tombogle reviewed 2 of 28 files at r1, all commit messages.
Reviewable status: 2 of 28 files reviewed, 7 unresolved discussions (waiting on @andrew-polk and @aror92)

SIL.Core/SIL.Core.csproj line 24 at r2 (raw file):

Previously, andrew-polk wrote…

I wonder if there are implications of adding these libraries we need to think through.

Increases size of SIL.Core's dependencies by about 0.6MB (probably ok...).

We worked hard at some point in the past to remove L10NSharp from SIL.Core. Probably that was just because of the WinForms dependency? But I don't remember if there were other reasons. It would be a bit unfortunate to add the dependency just for a couple translations of license messages. Have you considered using Localizer instead?

Another option which likely isn't worth it is to put all this clearshare stuff in a new package called SIL.Core.Clearshare or SIL.Clearshare

I agree. Probably the majority of SIL software that uses Core also uses L10nSharp, but even without the Winforms dependency in L10nSharp, I think the localization approach is a separate concern and should not be mixed in with Core. GlyssenEngine uses Core and is consumed downstream by applications that do not use L10nSharp for localization. This is very definitely what the Localizer class was created for. On a side note, now that L10nSharp is no longer Winforms-dependent, L10NSharpLocalizer could now be moved to a non-Winforms assembly (but sadly, SIL.Core is the only reasonable possibility as things stand).

andrew-polk

@andrew-polk reviewed 2 of 28 files at r1, 2 of 7 files at r3.
Reviewable status: 5 of 30 files reviewed, 8 unresolved discussions (waiting on @aror92 and @tombogle)

SIL.Core/ClearShare/LicenseInfo.cs line 3 at r3 (raw file):

using System.Collections.Generic;
using JetBrains.Annotations;
using L10NSharp;

You can now remove this and the package dependency in SIL.Core.csproj

andrew-polk

@andrew-polk reviewed 1 of 2 files at r4.
Reviewable status: 5 of 30 files reviewed, 6 unresolved discussions (waiting on @aror92 and @tombogle)

andrew-polk

I didn't do any kind of detailed review on the assumption that things were mostly just moved/split without changes except where necessary.
My overall comments have been addressed satisfactorily in my mind.

I appreciate that you tried to leave the class names in the WinForms part the same as they were.
I'm not sure I love the "Bare" names, but I wouldn't hold up the change for trying to figure out something better.

I know you all are trying to get this is as soon as you can. So if you want to merge, that's fine by me.

Reviewable status: 5 of 30 files reviewed, 6 unresolved discussions (waiting on @aror92 and @tombogle)

aror92

Reviewable status: 5 of 30 files reviewed, all discussions resolved (waiting on @andrew-polk and @tombogle)

SIL.Core/ClearShare/LicenseInfo.cs line 3 at r3 (raw file):

Previously, andrew-polk wrote…

You can now remove this and the package dependency in SIL.Core.csproj

Done.

tombogle

@tombogle reviewed 1 of 7 files at r3, all commit messages.
Reviewable status: 6 of 30 files reviewed, 7 unresolved discussions (waiting on @andrew-polk)

SIL.Core/ClearShare/LicenseInfo.cs line 6 at r4 (raw file):

namespace SIL.Core.ClearShare
{
	/// <summary/>

Please enter a real summary.

SIL.Core/ClearShare/CustomLicenseBare.cs line 7 at r4 (raw file):

{
	/// <summary>
	/// CustomLicenseBare is a WindowsForms-free base version of the CustomLicense class. In order to be WindowsForms independent,

Ditto: summary should answer "what is this?"

SIL.Core/ClearShare/CustomLicenseBare.cs line 25 at r4 (raw file):

		///<summary></summary>
		/// <remarks>
		/// Currently, we don't know the language of custom license strings, so we the ISO 639-2 code for undetermined, "und"

missing "use"

SIL.Core/ClearShare/CustomLicenseBare.cs line 54 at r4 (raw file):

		{
			if (!properties.ContainsKey("rights (en)"))
				throw new ApplicationException("A license property is required in order to make a  Custom License from metadata.");

extra space in "...make a Custom..."

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 8 at r4 (raw file):

{
	/// <summary>
	/// CreativeCommonsLicenseBare is a WindowsForms-free base version of the CustomLicense class. In order to be WindowsForms independent,

I think this is supposed to say CreativeCommonsLicense.
Also, this "Summary" feels more like "Remarks". It gives lots of useful information about the relationship of this class to CustomLicense and its WinForms independency, but it fails to answer the "what is this?" question that a would-be consumer would probably expect to see answered in the Summary.

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 12 at r4 (raw file):

	/// To include license images, use the CreativeCommonsLicense class in SIL.WindowsForms.Clearshare.
	/// </summary>
	public class CreativeCommonsLicenseBare : LicenseInfo

I also don't love "Bare" Since this is a breaking change anyway, could we not perhaps call this CreativeCommonsLicense and name the Winforms version WindowsCreativeCommonsLicense or `CreativeCommonsLicenseForWindows' or ``CreativeCommonsLicenseForWinForms', etc. (and similarly with the other "Bare versions). Or maybe better yet, since they bare in different namespaces, keep the class names the same. Consumers can just use the appropriate namespace.

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 193 at r4 (raw file):

		/// <remarks>
		/// REVIEW: (asked by Hatton Oct 2016) Serialization by whom? Why not just use the url, which is the canonical form?
		/// Note that this does not include any qualifier (of which at the moment the one one is "igo", but who knows

typo: one one

aror92

Reviewable status: 6 of 30 files reviewed, 10 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 12 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

CreativeCommonsLicenseInfo works

Done.

SIL.Core/ClearShare/CustomLicenseBare.cs line 11 at r5 (raw file):

Previously, tombogle (Tom Bogle) wrote…

CustomLicenseInfo

Done.

aror92

Reviewable status: 6 of 30 files reviewed, 10 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Core/ClearShare/LicenseUtils.cs line 11 at r5 (raw file):

Previously, tombogle (Tom Bogle) wrote…

No, but I think what I'm proposing would not be a significant change that could break anything. I actually explored several more disruptive ideas, but they involved reflection, etc., and I wasn't really comfortable with any of them. Moving the implementation details would just get those details closer to the class that should know and care.

This would work, and I can do the same in LicenseWithImageUtils for the Winforms side, as long as we are okay having "new static" TryCreateFromMetadata methods in CreativeCommonsLicense and CustomLicense in the Winforms side. For the most part, we were trying to avoid having static functions with the "new" keyword.

Otherwise we could avoid "new static" by using different names for TryCreateFromMetadata for Winforms vs Winforms-free. (We've already done this with FromMetadata.)

aror92

Reviewable status: 6 of 30 files reviewed, 10 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Core/ClearShare/LicenseInfo.cs line 6 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Yes. And don't thank me. Thank ChatGPT!

Done.

SIL.Core/ClearShare/LicenseUtils.cs line 9 at r5 (raw file):

Previously, tombogle (Tom Bogle) wrote…

A summary comment would be helpful here:

Done.

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 8 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

How about adding a summary that says clearly what this class is/does, as distinct from the remarks:

Done.

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 10 at r5 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I think a cref link would be nice:

Done.

SIL.Core/ClearShare/CustomLicenseBare.cs line 7 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Do the same as for the CC class, adding a summary like this:

Done.

SIL.Core/ClearShare/CustomLicenseBare.cs line 8 at r5 (raw file):

Previously, tombogle (Tom Bogle) wrote…

The winforms version just adds a generic Image property. I'm not sure it actually makes sense to refer to CC0 or CC BY. Those would seem to be related to the Creative Commons license, not some arbitrary custom license.

Done.

aror92

Reviewable status: 6 of 30 files reviewed, 10 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 12 at r4 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

Done.

We'll likely want a new name for MetadataBare as well, then: MetadataInfo, or something else?

tombogle

@tombogle reviewed 1 of 12 files at r7, all commit messages.
Reviewable status: 7 of 30 files reviewed, 6 unresolved discussions (waiting on @andrew-polk, @aror92, and @jasonleenaylor)

SIL.Core/ClearShare/LicenseUtils.cs line 11 at r5 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

This would work, and I can do the same in LicenseWithImageUtils for the Winforms side, as long as we are okay having "new static" TryCreateFromMetadata methods in CreativeCommonsLicense and CustomLicense in the Winforms side. For the most part, we were trying to avoid having static functions with the "new" keyword.

Otherwise we could avoid "new static" by using different names for TryCreateFromMetadata for Winforms vs Winforms-free. (We've already done this with FromMetadata.)

Ah. Yeah, "new static" is a bit of a code smell I think. Maybe just leave it as is...

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 12 at r4 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

We'll likely want a new name for MetadataBare as well, then: MetadataInfo, or something else?

Agreed. I have offered two possible suggestions.

SIL.Core/ClearShare/MetadataBare.cs line 49 at r7 (raw file):

	/// when it is simply asked for Copyright, even though the new value is stored in two other copyright fields.
	/// </summary>
	public class MetadataBare

I think MetadataBase or MetadataCore would be better names. Pretty decent precedent for Base, none for Core.

SIL.Core/ClearShare/CreativeCommonsLicenseInfo.cs line 82 at r7 (raw file):

		public static LicenseInfo CCLicenseInfoFromMetadata(Dictionary<string, string> metadataProperties)
		{
			if(!metadataProperties.ContainsKey("license"))

Preferred coding style in libpalaso is to put a space between if and the opening parenthesis. (Won't make this a blocker, since there are already a few hundred places where it's done this way, but consistency does make for easier searching and readability.) Same a few lines below.

SIL.Core/ClearShare/CreativeCommonsLicenseInfo.cs line 92 at r7 (raw file):

		}

		public static CreativeCommonsLicenseInfo BareLicenseFromUrl(string url)

A good naming pattern used elsewhere in libpalaso DLLs for this kind of thing is CreateFromUrl. Another place uses CreateRepeatClassNameHereFromBlah, but since this is already a static, I think that is not as good of a pattern to follow.

SIL.Windows.Forms/ClearShare/Metadata.cs line 76 at r7 (raw file):

		/// </summary>
		/// <example>LoadXmpFile("c:\dir\metadata.xmp")</example>
		public new void LoadXmpFile(string path)

If this "new override" is intended to be selected based on the compile-time type of the variable, it really needs a clear comment to that effect. It's extremely subtle behavior, especially since it is textually identical to the method it is replacing. If, on the other hand, that is not the intent, and we really want to choose the correct method based on the runtime type of the object, then the base class should be marked virtual and this should be a normal override. (Even then a comment would probably be in order.)
But if the latter, does LoadProperties actually have to be static? Seems like it would make more sense for it to be a member, since it always gets an instance as its second parameter. Then no override would be needed at all.

tombogle

Reviewable status: 7 of 30 files reviewed, 8 unresolved discussions (waiting on @andrew-polk, @aror92, and @jasonleenaylor)

SIL.Core/ClearShare/MetadataBare.cs line 23 at r7 (raw file):

{
	/// <summary>
	/// MetadataBare is a WindowsForms-free base version of the Metadata class. In order to be WindowsForms independent, it does not include information about any license images

As we've done other places, I think the summary should begin with what this class is/does, and maybe move the other info into a remarks element (with appropriate crefs).

SIL.Core/ClearShare/MetadataBare.cs line 32 at r7 (raw file):

	/// Microsoft Pro Photo Tools: http://www.microsoft.com/download/en/details.aspx?id=13518
	///
	/// A previous version of this class used exiftool.exe to read and write this data. The exact fields chosen to store each piece of ClearShare metadata

Since technically this class is not the same class as the one being referred to here, and since this a really long comment with a lot of technical details, maybe just keep it in the actual Metadata class and add a short one-sentence reference here so if anyone is interested they can go read all the details there.

aror92

Reviewable status: 7 of 31 files reviewed, 5 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Core/ClearShare/CreativeCommonsLicenseInfo.cs line 82 at r7 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Preferred coding style in libpalaso is to put a space between if and the opening parenthesis. (Won't make this a blocker, since there are already a few hundred places where it's done this way, but consistency does make for easier searching and readability.) Same a few lines below.

done

SIL.Core/ClearShare/CreativeCommonsLicenseInfo.cs line 92 at r7 (raw file):

Previously, tombogle (Tom Bogle) wrote…

A good naming pattern used elsewhere in libpalaso DLLs for this kind of thing is CreateFromUrl. Another place uses CreateRepeatClassNameHereFromBlah, but since this is already a static, I think that is not as good of a pattern to follow.

Most of what's in this class was moved unchanged from SIL.WindowsForms/ClearShare/CreativeCommonsLicense.cs. But this is one thing that was slightly altered.

The WinForms CCLicense class contains a static method named just "FromUrl" that returns a CCLicense object. The WinForms-free CCLicenseInfo class needs a similar method to return a CCLicenseInfo object, and we want the distinction to be clear from the method name. Given that, I would opt for "CreateCreativeCommonsLicenseInfoFromUrl" even though this is a static.

SIL.Core/ClearShare/MetadataBare.cs line 23 at r7 (raw file):

Previously, tombogle (Tom Bogle) wrote…

As we've done other places, I think the summary should begin with what this class is/does, and maybe move the other info into a remarks element (with appropriate crefs).

done

SIL.Core/ClearShare/MetadataBare.cs line 32 at r7 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Since technically this class is not the same class as the one being referred to here, and since this a really long comment with a lot of technical details, maybe just keep it in the actual Metadata class and add a short one-sentence reference here so if anyone is interested they can go read all the details there.

done

SIL.Core/ClearShare/MetadataBare.cs line 49 at r7 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I think MetadataBase or MetadataCore would be better names. Pretty decent precedent for Base, none for Core.

The one hesitation I have to using Base here is the implication that the class wouldn't be used on its own, especially since this class is the main way to get metadata info outside of Winforms.

aror92

Reviewable status: 7 of 31 files reviewed, 5 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Windows.Forms/ClearShare/Metadata.cs line 76 at r7 (raw file):

Previously, tombogle (Tom Bogle) wrote…

If this "new override" is intended to be selected based on the compile-time type of the variable, it really needs a clear comment to that effect. It's extremely subtle behavior, especially since it is textually identical to the method it is replacing. If, on the other hand, that is not the intent, and we really want to choose the correct method based on the runtime type of the object, then the base class should be marked virtual and this should be a normal override. (Even then a comment would probably be in order.)
But if the latter, does LoadProperties actually have to be static? Seems like it would make more sense for it to be a member, since it always gets an instance as its second parameter. Then no override would be needed at all.

Thanks for catching this! Yes, it should be virtual & override. I've fixed that. Will add a comment and look into removing static from LoadProperties.

SIL.Core/ClearShare/CreativeCommonsLicenseBare.cs line 12 at r4 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Agreed. I have offered two possible suggestions.

Done.

aror92

Reviewable status: 7 of 31 files reviewed, 5 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Windows.Forms/ClearShare/Metadata.cs line 76 at r7 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

Thanks for catching this! Yes, it should be virtual & override. I've fixed that. Will add a comment and look into removing static from LoadProperties.

Finished changes to LoadProperties & added comments. This issue should be complete now.

SIL.Core/ClearShare/MetadataBare.cs line 49 at r7 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

The one hesitation I have to using Base here is the implication that the class wouldn't be used on its own, especially since this class is the main way to get metadata info outside of Winforms.

I've gone with MetadataCore but can switch if you have a strong preference for Base over Core

tombogle · 2025-12-09T16:53:02Z

SIL.Windows.Forms/ClearShare/WinFormsUI/MetadataEditorControl.cs line 76 at r10 (raw file):

				_copyrightBy.Text = _metadata.GetCopyrightBy();
				if (_metadata.License!=null && (_metadata.License is ILicenseWithImage))

Can also be simplified

Code snippet:

if (_metadata.License is ILicenseWithImage licenseWithImage)
    _licenseImage.Image = licenseWithImage.GetImage();

tombogle

@tombogle reviewed 1 of 8 files at r8, all commit messages.
Reviewable status: 8 of 31 files reviewed, 3 unresolved discussions (waiting on @andrew-polk, @aror92, and @jasonleenaylor)

SIL.Windows.Forms/ClearShare/WinFormsUI/MetadataDisplayControl.cs line 71 at r10 (raw file):

					// Licenses constructed from SIL.Core.Clearshare will not have images,
					// and will not be of type ILicenseWithImage.
					if (metaData.License is ILicenseWithImage)

You can use "pattern matching" to simplify this code.

Code snippet:

if (metaData.License is ILicenseWithImage licenseWithImage)
    licenseImage = licenseWithImage.GetImage();

aror92

Reviewable status: 7 of 31 files reviewed, 3 unresolved discussions (waiting on @andrew-polk, @jasonleenaylor, and @tombogle)

SIL.Windows.Forms/ClearShare/WinFormsUI/MetadataDisplayControl.cs line 71 at r10 (raw file):

Previously, tombogle (Tom Bogle) wrote…

You can use "pattern matching" to simplify this code.

Done.

SIL.Windows.Forms/ClearShare/WinFormsUI/MetadataEditorControl.cs line 76 at r10 (raw file):

Previously, tombogle (Tom Bogle) wrote…

Can also be simplified

Done.

tombogle

@tombogle reviewed 2 of 28 files at r1, 1 of 7 files at r3, 1 of 7 files at r6, 2 of 12 files at r7, 2 of 8 files at r8, 2 of 4 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: 19 of 31 files reviewed, all discussions resolved (waiting on @andrew-polk and @jasonleenaylor)

andrew-polk

Reviewable status: 19 of 31 files reviewed, all discussions resolved (waiting on @jasonleenaylor and @tombogle)

aror92 added 14 commits November 12, 2025 09:49

Fix metadata test

121f8b1

Update metadata test so it doesn't require System.Drawing Update version of L10NSharp

Fix incorrect formatting

e08c0f5

Autoformatting messed up a few comments because it mis-identified which lines of code the comments were associated with.

Fix license class naming

867f1eb

Use CreativeCommonsLicense and CustomLicense for the superclasses and add "WithImage" specification to the subclasses with images.

Fix L10NSharp version

fc75ef9

Fix License Class naming

b3634e8

Update test app usings

73fca3a

Fix ILicenseWithImage cast

e95cacf

Working WindowsForms split

08e562d

Fix license class naming

0b84f03

Add Licenses with new names; rename static methods in bare classes

20faa75

Add Winforms metadata tests

9875706

Fix L10NSharp version number

af01423

Update CHANGELOG

0d78af6

github-advanced-security bot found potential problems Nov 12, 2025

View reviewed changes

Fix mediaFile in MetadataBareTests

cd7eb47

github-advanced-security bot found potential problems Nov 12, 2025

View reviewed changes

SIL.Core.Tests/ClearShare/MetadataBareTests.cs Fixed Show fixed Hide fixed

SIL.Core.Tests/ClearShare/MetadataBareTests.cs Fixed Show fixed Hide fixed

tombogle reviewed Nov 13, 2025

View reviewed changes

aror92 added 2 commits November 19, 2025 09:57

Use Localizer in LicenseInfo instead of L10NSharp.LocalizationManager

4d87a67

Dispose mediafiles in MetadataBareTests

53ed05e

andrew-polk requested changes Nov 19, 2025

View reviewed changes

Remove L10NSharp dependency in SIL.Core

5edc50c

andrew-polk reviewed Nov 19, 2025

View reviewed changes

aror92 commented Nov 19, 2025

View reviewed changes

tombogle requested changes Nov 20, 2025

View reviewed changes

Fix comments in CCLicense, CCLicenseBare, and CustomLicenseBare

7dea421

aror92 added 2 commits November 25, 2025 09:07

Rename CC/CustomLicenseBare to CC/CustomLicenseInfo

98374ee

Fix CCLicenseInfo naming

06841ec

aror92 commented Nov 25, 2025

View reviewed changes

Add summaries in Winforms-free classes

5ae51d0

aror92 commented Nov 25, 2025

View reviewed changes

aror92 added 2 commits November 25, 2025 10:06

Update refs to CCLicenseInfo and CustomLicenseInfo

f789a98

Add summary to CreativeCommonsLicense

7ad02b3

tombogle requested changes Nov 26, 2025

View reviewed changes

Rename MetadataBare to MetadataCore

8447409

aror92 commented Dec 3, 2025

View reviewed changes

aror92 added 2 commits December 3, 2025 09:56

Fix MetadataCore reference in Metadata

ea0fb8c

Fix LoadXmpFile in Metadata & MetadataCore to use virtual/override

84c4699

aror92 commented Dec 3, 2025

View reviewed changes

Remove static modifier from LoadProperties

d961c94

aror92 commented Dec 9, 2025

View reviewed changes

tombogle requested changes Dec 9, 2025

View reviewed changes

Simplify license checks in Metadata controls via pattern matching

756c248

aror92 commented Dec 9, 2025

View reviewed changes

tombogle approved these changes Dec 9, 2025

View reviewed changes

andrew-polk approved these changes Dec 9, 2025

View reviewed changes

andrew-polk and others added 4 commits December 9, 2025 15:24

Merge branch 'master' into core.metadata

d372675

Update CHANGELOG

a0ad541

Cleanup mediafile in MetadataTests

5aa26ad

Initialize mediaFile in metadata tests setup

f4e2040

aror92 merged commit 56d54b4 into master Dec 9, 2025
10 of 11 checks passed

aror92 deleted the core.metadata branch December 9, 2025 23:40

Uh oh!

Move Metadata and LicenseInfo so they can be accessed independent of Windows Forms #1478

Move Metadata and LicenseInfo so they can be accessed independent of Windows Forms #1478

Uh oh!

Conversation

aror92 commented Nov 12, 2025 • edited by tombogle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Palaso Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrew-polk commented Nov 12, 2025

Uh oh!

Uh oh!

Uh oh!

andrew-polk commented Nov 12, 2025

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle commented Dec 9, 2025

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

aror92 left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

aror92 commented Nov 12, 2025 •

edited by tombogle

Loading

github-actions bot commented Nov 12, 2025 •

edited

Loading