Split L10NSharp to remove winforms dependency

[aror92]

Split L10NSharp into two parts to separate out the portion that does not require use of Windows Forms from the portion that does.

Additionally, remove System.Drawing dependency from the Windows Forms independent portion, since as of .NET 6 System.Drawing.Common is only supported on Windows.

Remove code related to runtime localization.

---

This change is 

[github-actions]

Test Results

    6 files  +3    84 suites  ±0   20s ⏱️ -9s
165 tests ±0  160 ✔️ ±0    5 💤 ±0  0 ❌ ±0 
495 runs  ±0  480 ✔️ ±0  15 💤 ±0  0 ❌ ±0 

Results for commit a96ea73. ± Comparison against base commit bd579ae.

♻️ This comment has been updated with latest results.

[andrew-polk]

Is there an advantage to being more consistent with libpalaso for naming?
That would be L10NSharp.Windows.Forms instead of L10NSharpWinForms for the package and namespace.
If you say it isn't worth it, I won't complain.

And then I also wonder if the ".UI" at the end of the namespace is helpful anymore.

Was there a reason to move away from ".Tests" (to just "Tests") in the namespace? I guess that makes it more consistent with the folder. (FWIW, libpalaso has .Tests for both folders and namespaces. But I do agree that having the namespaces and folder aligned is better regardless of which way you go.)

Reviewed 80 of 93 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 81 of 94 files reviewed, 6 unresolved discussions

---

src/L10NSharp/Utils.cs line 26 at r2 (raw file):

		[DllImport("user32.dll", CharSet = CharSet.Auto, EntryPoint = "SendMessage")]
		private static extern void SendMessageWindows(IntPtr hWnd, int msg, int wParam, int lParam);

I think these two SendMessage methods want to go into the Windows Forms package.

---

src/L10NSharp/Properties/AssemblyInfo.cs line 13 at r2 (raw file):

[assembly: InternalsVisibleTo("ExtractXliff, PublicKey=002400000480000094000000060200000024000052534131000400000100010019cae48c6e5703818db31fcca17b87012eef38df95e3961c59b035190eace2c4ee5cfa1a258b84867c7549f60eec00ec1e0004c2ed224457128e841311cea0f5a05c3d69b3dfcb7422b214febce6e06c83ce4d29c62f36a7fd5564e922338c800372a5ec09638671b4db1fb33ccbb1dc48d8122ffe0d30dadbfbf325f65437b2")]
[assembly: InternalsVisibleTo("CheckOrFixXliff, PublicKey=002400000480000094000000060200000024000052534131000400000100010019cae48c6e5703818db31fcca17b87012eef38df95e3961c59b035190eace2c4ee5cfa1a258b84867c7549f60eec00ec1e0004c2ed224457128e841311cea0f5a05c3d69b3dfcb7422b214febce6e06c83ce4d29c62f36a7fd5564e922338c800372a5ec09638671b4db1fb33ccbb1dc48d8122ffe0d30dadbfbf325f65437b2")]
[assembly: InternalsVisibleTo("L10NSharpWinforms, PublicKey=002400000480000094000000060200000024000052534131000400000100010019cae48c6e5703818db31fcca17b87012eef38df95e3961c59b035190eace2c4ee5cfa1a258b84867c7549f60eec00ec1e0004c2ed224457128e841311cea0f5a05c3d69b3dfcb7422b214febce6e06c83ce4d29c62f36a7fd5564e922338c800372a5ec09638671b4db1fb33ccbb1dc48d8122ffe0d30dadbfbf325f65437b2")]

I suppose it probably doesn't hurt anything. And probably makes this refactoring easier...
But I wonder if it is really the best design to have the Winforms package see the internals of L10NSharp.

---

src/L10NSharp/LocalizationManagerInternal.cs line 47 at r2 (raw file):

		internal static string ChooseFallbackLanguage()
		{
			return LocalizationManager.kDefaultLang;

I don't know if it is worth it, but I wonder if this should do more.
For example, should there be an option for the client to provide a fallback implementation such that the client could still use the existing dialog as it work now if desired?

Also, I didn't go researching myself, but I think maybe it used to be a little smarter? Like, if the desired lang was es-ES but we only have es, it would fall back to es? Now it seems like it just always falls back to English.
Hm. Well, maybe I'm wrong and this is still handled in MapToExistingLanguageIfPossible.

---

src/L10NSharp/LocalizationManagerInternal.cs line 779 at r2 (raw file):

		}

		/*/// ------------------------------------------------------------------------------------

Any reason to keep this commented code?

---

src/L10NSharpWinforms/LocalizationManagerWinforms.cs line 58 at r2 (raw file):

			string appId, string appName, string appVersion, string directoryOfInstalledFiles,
			string relativeSettingPathForLocalizationFolder,
			Icon applicationIcon, string emailForSubmissions,

As far as I can tell, applicationIcon is never used now. (Looks like it was just for LanguageChoosingDialog?)

[But see other comment about possibly reinstating use of LanguageChoosingDialog elsewhere.]

If that's true, that might mean a lot of clean-up can happen, starting with the removal of this Create method.
(I haven't fully evaluated that.)

---

src/L10NSharpWinforms/LocalizationManagerWinforms.cs line 82 at r2 (raw file):

		/// ------------------------------------------------------------------------------------
		internal new static Dictionary<string, ILocalizationManagerInternalWinforms> LoadedManagers

I think the only two usages of this should be using LocalizationManager.LoadedManagers.

[andrew-polk]

I haven't finished looking through things yet, but I have enough that it is worth publishing what I have.

Reviewable status: 81 of 94 files reviewed, 6 unresolved discussions (waiting on @aror92)

[aror92]

Happy to use L10NSharp.Windows.Forms instead, and I can remove the UI designation in the namespace. I believe L10NSharp was already using just "Tests" in the namespace. I didn't change anything about the existing L10NSharp & L10NSharpTests namespaces and tried to copy the style of those in the Winforms portion. Would you prefer I change this for consistency with Palaso, or leave it as it?

[aror92]

src/L10NSharp/Properties/AssemblyInfo.cs line 13 at r2 (raw file):

Previously, andrew-polk wrote…

I suppose it probably doesn't hurt anything. And probably makes this refactoring easier...
But I wonder if it is really the best design to have the Winforms package see the internals of L10NSharp.

Happy for alternate suggestions for how to handle this. The issue is that the Winforms package needs to extend the internal interface and class ILocalizationManagerInternal and LocalizationManagerInternal. The solutions that occur to me are either these need to become public, or we need to see L10NSharp internals.

[aror92]

src/L10NSharp/LocalizationManagerInternal.cs line 47 at r2 (raw file):

Previously, andrew-polk wrote…

I don't know if it is worth it, but I wonder if this should do more.
For example, should there be an option for the client to provide a fallback implementation such that the client could still use the existing dialog as it work now if desired?

Also, I didn't go researching myself, but I think maybe it used to be a little smarter? Like, if the desired lang was es-ES but we only have es, it would fall back to es? Now it seems like it just always falls back to English.
Hm. Well, maybe I'm wrong and this is still handled in MapToExistingLanguageIfPossible.

Good point. It used to use a winforms LanguageChoosingDialog to select a fallback language. I should have added that handling to LocalizationManagerInternalWinforms to retain the previous behavior--I'll add it now. Do you think there's a need for something more in the winforms-free side of things?

The fallback you're describing is handled in MapToExistingLanguageIfPossible.

[aror92]

Made the Winforms > .Windows.Forms namespace change. I just checked again and realized the RootNamespace for L10NSharp tests is .Tests. Updated the folder the match.

Also, I'm realizing that I find it quite helpful to have all the dialogues, buttons, helpers, etc in a separate namespace. I'd vote for changing ".UI" to ".UIComponents" and moving L10NExtender out into the Windows.Forms namespace.

[aror92]

src/L10NSharp/LocalizationManagerInternal.cs line 779 at r2 (raw file):

Previously, andrew-polk wrote…

Any reason to keep this commented code?

Deleted, thanks for catching this.

[andrew-polk]

Also, I'm realizing that I find it quite helpful to have all the dialogues, buttons, helpers, etc in a separate namespace. I'd vote for changing ".UI" to ".UIComponents" and moving L10NExtender out into the Windows.Forms namespace.

Yeah, I had actually started thinking there probably was a benefit to still maintaining a different (sub) namespace for actual UI stuff. Sounds good.

Reviewed 1 of 93 files at r1, 90 of 101 files at r3, all commit messages.
Reviewable status: 100 of 114 files reviewed, 6 unresolved discussions (waiting on @aror92)

---

src/L10NSharp.Tests/LocalizationManagerTestsBase.cs line 1010 at r3 (raw file):

		}

		/*[Test]

I'm guessing this got moved to the WinForms side and can be deleted here.

[andrew-polk]

Reviewable status: 100 of 114 files reviewed, 6 unresolved discussions (waiting on @aror92)

---

src/L10NSharp/LocalizationManagerInternal.cs line 47 at r2 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

Good point. It used to use a winforms LanguageChoosingDialog to select a fallback language. I should have added that handling to LocalizationManagerInternalWinforms to retain the previous behavior--I'll add it now. Do you think there's a need for something more in the winforms-free side of things?

The fallback you're describing is handled in MapToExistingLanguageIfPossible.

So is the plan that clients who still need the WinForms stuff will call LocalizationManagerWinForms.Create instead of LocalizationManager.Create?

[andrew-polk]

Along with adding to the ChangeLog, you'll want to add "+semver:major" to one of the commit messages.

Reviewable status: 100 of 114 files reviewed, 6 unresolved discussions (waiting on @aror92)

[andrew-polk]

Reviewable status: 100 of 114 files reviewed, 5 unresolved discussions (waiting on @aror92)

---

src/L10NSharp/Properties/AssemblyInfo.cs line 13 at r2 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

Happy for alternate suggestions for how to handle this. The issue is that the Winforms package needs to extend the internal interface and class ILocalizationManagerInternal and LocalizationManagerInternal. The solutions that occur to me are either these need to become public, or we need to see L10NSharp internals.

Yeah... if we were really going to do things the cleanest, I think we could go back to not having the "internal" version at all. But

1. That's biting off more than is worth it for this project.
2. As soon as did that, someone would want to implement another TranslationMemory type, and we would have shot ourselves in the foot.

I'm sure this is probably the best scenario given what we're trying to do.

[andrew-polk]

Reviewed 4 of 101 files at r3.
Reviewable status: 104 of 114 files reviewed, 3 unresolved discussions (waiting on @aror92)

---

src/L10NSharpWinforms/LocalizationManagerWinforms.cs line 58 at r2 (raw file):

Previously, andrew-polk wrote…

As far as I can tell, applicationIcon is never used now. (Looks like it was just for LanguageChoosingDialog?)

[But see other comment about possibly reinstating use of LanguageChoosingDialog elsewhere.]

If that's true, that might mean a lot of clean-up can happen, starting with the removal of this Create method.
(I haven't fully evaluated that.)

Ok, now that you've reinstated LanguageChoosingDialog, I guess that does use it.

---

src/L10NSharpWinforms/LocalizationManagerWinforms.cs line 82 at r2 (raw file):

Previously, andrew-polk wrote…

I think the only two usages of this should be using LocalizationManager.LoadedManagers.

Never mind. I see you switched it the other way and that was probably more correct.

[andrew-polk]

Also the README will need updating.

Reviewable status: 104 of 114 files reviewed, 4 unresolved discussions (waiting on @aror92)

---

src/L10NSharp.Windows.Forms/XLiffUtils/XliffLocalizationManagerWinforms.cs line 209 at r3 (raw file):

		public static void ShowLocalizationDialogBox(string id, IWin32Window owner = null)
		{
			TipDialog.ShowAltShiftClickTip(owner);

Looks like there is some stuff to clean up with the Alt-Shift-Click handler which used to bring up the localization dialog.
(This is also referenced in a label in the sample app and README.)

[aror92]

Reviewable status: 38 of 135 files reviewed, 4 unresolved discussions (waiting on @andrew-polk)

---

src/L10NSharp/LocalizationManagerInternal.cs line 47 at r2 (raw file):

Previously, andrew-polk wrote…

So is the plan that clients who still need the WinForms stuff will call LocalizationManagerWinForms.Create instead of LocalizationManager.Create?

Yes, that's the plan

---

src/L10NSharp/Utils.cs line 26 at r2 (raw file):

Previously, andrew-polk wrote…

I think these two SendMessage methods want to go into the Windows Forms package.

Done.

---

src/L10NSharp.Tests/LocalizationManagerTestsBase.cs line 1010 at r3 (raw file):

Previously, andrew-polk wrote…

I'm guessing this got moved to the WinForms side and can be deleted here.

Thanks for catching this. I hadn't moved it yet; just did that and deleted it here.

[aror92]

Reviewable status: 125 of 135 files reviewed, 4 unresolved discussions (waiting on @andrew-polk)

---

src/L10NSharp.Windows.Forms.Tests/TempFile.cs line 21 at r6 (raw file):

Previously, andrew-polk wrote…

Is there a reason you needed to copy this file to Winforms rather than use the existing one?

Not really. I hadn't added a project reference to L10NSharp.Tests in L10NSharp.Windows.Forms.Tests and wasn't sure whether it made more sense to do that or to copy this file. I initially thought I might only need a small piece of what was in this file but ended up needing all of it for one of the tests. I've referenced the existing one instead and remove the duplication.

---

src/L10NSharp.Windows.Forms.Tests/TestXliff/xliff-core-1.2-strict.xsd line 1 at r6 (raw file):

Previously, andrew-polk wrote…

I don't understand why the .xsd files were copied.

Thanks for catching that. Needed to copy the test xlfs, accidentally copied the xsds as well.

---

src/L10NSharp.Windows.Forms/XLiffUtils/XliffLocalizationManagerWinforms.cs line 209 at r3 (raw file):

Previously, andrew-polk wrote…

Looks like there is some stuff to clean up with the Alt-Shift-Click handler which used to bring up the localization dialog.
(This is also referenced in a label in the sample app and README.)

Thanks! Still need to revise the README, but I think this cs file is cleaned up now.

[aror92]

Reviewable status: 103 of 146 files reviewed, 6 unresolved discussions (waiting on @andrew-polk)

---

src/L10NSharp.Windows.Forms/XLiffUtils/XliffLocalizationManagerWinforms.cs line 103 at r7 (raw file):

					if (component is ILocalizableComponent)
						ComponentCache.Add(component, id);
					else

@andrew-polk Am I correct in thinking this entire else case isn't needed anymore if we're doing away with runtime localization?

---

src/L10NSharp.Windows.Forms/XLiffUtils/XliffLocalizationManagerWinforms.cs line 210 at r7 (raw file):

				return;

			if (ApplyLocalizationsToToolStripItem(component as ToolStripItem, id))

I think the ToolStripItem, ColumnHeader, and DataGridViewColumn cases are also related to the LocalizeItemDlg & could be done away with

[andrew-polk]

Reviewed 33 of 35 files at r7, all commit messages.
Reviewable status: 136 of 146 files reviewed, 2 unresolved discussions (waiting on @aror92)

---

src/L10NSharp.Windows.Forms/XLiffUtils/XliffLocalizationManagerWinforms.cs line 103 at r7 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

@andrew-polk Am I correct in thinking this entire else case isn't needed anymore if we're doing away with runtime localization?

That seems right. But it is difficult to tell what is going on exactly.
I'm not even sure what "the config dialog" is.

---

src/L10NSharp.Windows.Forms/XLiffUtils/XliffLocalizationManagerWinforms.cs line 210 at r7 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

I think the ToolStripItem, ColumnHeader, and DataGridViewColumn cases are also related to the LocalizeItemDlg & could be done away with

No, unless I'm mistaken, this is simply tying in to be able to localize the corresponding WinForms controls.

[andrew-polk]

Something isn't working right. When I load the SampleApp with the current code, it doesn't seem to localize the components.
If I add back in the Resources.Icon parameters in the LocalizationManagerWinforms.Create calls in the SampleApp, it does work.
I think the difference is that without the icon parameter, it is actually calling the non-winforms Create method and things don't get set up correctly.

Reviewable status: 136 of 146 files reviewed, 2 unresolved discussions (waiting on @aror92)

[aror92]

Oh, yes, LocalizationManagerWinforms is inheriting the Creates from LocalizationManager. I added a new create to LocalizationManagerWinforms that doesn't expect an icon parameter. There are also two obsolete create methods in LocalizationManager that include a TranslationMemory parameter. Since they're obsolete, I haven't added new create methods to hide those in LocalizationManagerWinforms. Should I? (Or is this a good time to remove those obsolete methods?)

The above should fix this issue if create is called without the icon parameter. I've also added that parameter back to the SampleApp.

Reviewable status: 136 of 146 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

[aror92]

Updated Changelog and README.

Reviewable status: 132 of 148 files reviewed, all discussions resolved (waiting on @andrew-polk)

[jasonleenaylor]

src/L10NSharp.Windows.Forms/LocalizationManagerWinforms.cs line 159 at r9 (raw file):

		/*/// <summary>
		/// True (default) to throw if we try to get a string from a particular manager

Did these get implemented in a different place? It makes sense that these should be in a non-winforms location. Also it would be better if they weren't statics if we ever consider using this outside of a desktop app situation...but that should probably be saved for a future PR.

[aror92]

src/L10NSharp.Windows.Forms/LocalizationManagerWinforms.cs line 159 at r9 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Did these get implemented in a different place? It makes sense that these should be in a non-winforms location. Also it would be better if they weren't statics if we ever consider using this outside of a desktop app situation...but that should probably be saved for a future PR.

Yes, they're in LocalizationManager instead. I'll delete them from here.

[tombogle]

src/L10NSharp/Properties/AssemblyInfo.cs line 13 at r2 (raw file):

Previously, andrew-polk wrote…
Happy for alternate suggestions for how to handle this. The issue is that the Winforms package needs to extend the internal interface and class ILocalizationManagerInternal and LocalizationManagerInternal. The solutions that occur to me are either these need to become public, or we need to see L10NSharp internals.

I think I would favor making them public unless there is some clear and present danger (which seems pretty unlikely for an interface).

[tombogle]

Oh, yes, LocalizationManagerWinforms is inheriting the Creates from LocalizationManager. I added a new create to LocalizationManagerWinforms that doesn't expect an icon parameter. There are also two obsolete create methods in LocalizationManager that include a TranslationMemory parameter. Since they're obsolete, I haven't added new create methods to hide those in LocalizationManagerWinforms. Should I? (Or is this a good time to remove those obsolete methods?)

The above should fix this issue if create is called without the icon parameter. I've also added that parameter back to the SampleApp.

Reviewable status: 136 of 146 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

Since we're breaking everything, this is probably the ideal time to jettison anything marked as obsolete.

[andrew-polk]

agree

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

[andrew-polk]

@andrew-polk reviewed 1 of 93 files at r1, 1 of 82 files at r4, 1 of 3 files at r8, 5 of 7 files at r9, all commit messages.
Reviewable status: 140 of 148 files reviewed, 7 unresolved discussions (waiting on @jasonleenaylor and @tombogle)

---

src/L10NSharp/LocalizingInfo.cs line 107 at r10 (raw file):

_shortcutKeys

Can/should this move to winforms?

---

src/L10NSharp/LocalizingInfo.cs line 77 at r10 (raw file):

		/// <summary></summary>
		Unspecified,
	}

Can/should this move to Winforms?

---

src/L10NSharp/LocalizingInfo.cs line 89 at r10 (raw file):

		ShortcutKeys = 8,
		All = Text | Comment | ToolTip | ShortcutKeys
	}

Can/should this move to winforms?

---

src/L10NSharp/LocalizingInfo.cs line 99 at r10 (raw file):

	/// LocalizationExtender. The type of object is either a Control or ToolStripItem and
	/// the information kept track of is the text, tooltip, shortcut keys, localization
	/// priority, comment and localization category.

I think this comment needs updating.

---

src/L10NSharp/LocalizingInfo.cs line 102 at r10 (raw file):

	/// </summary>
	/// ----------------------------------------------------------------------------------------
	public class LocalizingInfo

Much of this class seems pretty specific to winforms stuff.
Maybe it was too hard to tease out its usage from other places in the non-winforms code?

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 22 at r10 (raw file):

		//private string _shortcutKeys;
		//private string _comment;
		//private LocalizationCategory _category = LocalizationCategory.Unspecified;

Any reason to keep these here as comments?

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 166 at r10 (raw file):

					//return (i < 0 ? string.Empty : text.Substring(0, i));
			return (i < 0 ? text : text.Substring(0, i));
		}*/

Any reason to keep this?

[aror92]

Okay, great. I actually went ahead and made that change when I was working on the Changelog.

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

---

src/L10NSharp/LocalizingInfo.cs line 77 at r10 (raw file):

Previously, andrew-polk wrote…

Can/should this move to Winforms?

LocalizationCategory is currently used in XliffLocalizedStringCache, but I could look into whether it would be possible to move those usages to XliffLocalizedStringCacheWinforms

---

src/L10NSharp/LocalizingInfo.cs line 89 at r10 (raw file):

Previously, andrew-polk wrote…

Can/should this move to winforms?

UpdateFields is used in the StringExtractor, some XliffUtils, and LocalizationManagerInternal. I don't think we'd want to move it entirely, because we do at least need to use and update Text and Comment in the non-winforms code.

---

src/L10NSharp/LocalizingInfo.cs line 102 at r10 (raw file):

Previously, andrew-polk wrote…

Much of this class seems pretty specific to winforms stuff.
Maybe it was too hard to tease out its usage from other places in the non-winforms code?

Yeah, it's tricky because LocalizingInfo is used a fair amount in the non-winforms code, especially StringExtractor (this is probably the biggest challenge to moving more of LocalizingInfo to the winforms side; it references all the properties at least once), and to a lesser extent some classes in XliffUtils.

---

src/L10NSharp/LocalizingInfo.cs line 107 at r10 (raw file):

Previously, andrew-polk wrote…

_shortcutKeys

Can/should this move to winforms?

Nothing is really done with it here in LocalizingInfo. But its value is set in the StringExtractor's "GetInfoForCallToGetStringMethod" method in the non-winforms side (depending on how many parameters are given). If it seems best to move it, I could make some changes in StringExtractor. We may need a Winforms version of StringExtractor too, then?

---

src/L10NSharp/Properties/AssemblyInfo.cs line 13 at r2 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I think I would favor making them public unless there is some clear and present danger (which seems pretty unlikely for an interface).

The LocalizationManagerInternal class handles the active manager list and changes to it, which probably could be public, though I think there may be benefit to keeping it internal. If we do want to make these public, I think it would be better as follow-up work in another PR (along the lines of Andrew's item number 1 above). It would involve combining the current public class LocalizationManager and the internal class LocalizationManagerInternal, as well as the interfaces ILocalizationManager and ILocalizationManagerInternal.

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 22 at r10 (raw file):

Previously, andrew-polk wrote…

Any reason to keep these here as comments?

Thanks for catching it; deleted

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 166 at r10 (raw file):

Previously, andrew-polk wrote…

Any reason to keep this?

deleted

[andrew-polk]

@andrew-polk reviewed 2 of 93 files at r1, 1 of 9 files at r5, 1 of 7 files at r9, 4 of 4 files at r10.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aror92)

---

src/L10NSharp/LocalizingInfo.cs line 107 at r10 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

Nothing is really done with it here in LocalizingInfo. But its value is set in the StringExtractor's "GetInfoForCallToGetStringMethod" method in the non-winforms side (depending on how many parameters are given). If it seems best to move it, I could make some changes in StringExtractor. We may need a Winforms version of StringExtractor too, then?

I don't want to expand the scope of this project.
But I also don't want to leave anything in an in-between state which can be resolved fairly easily.

I mostly wanted to raise the questions to make sure they are things you've thought about. You have, so I'm satisfied.

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 22 at r10 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

Thanks for catching it; deleted

I'm not seeing the deletion.

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 166 at r10 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

deleted

Not seeing the deletion.

---

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs line 622 at r10 (raw file):

		}

		/*/// ------------------------------------------------------------------------------------

Any reason to keep this?

---

src/L10NSharp/XLiffUtils/XliffLocalizedStringCache.cs line 52 at r10 (raw file):

				{
					//MessageBox.Show("Error occurred reading localization file:" + Environment.NewLine + e.Message,
					//	Application.ProductName, MessageBoxButtons.OK, MessageBoxIcon.Warning);

Seems like it would be better to at least give a Console.WriteLn or something.
And then there's probably no reason to keep the comment.

[andrew-polk]

I've finally made it through my initial pass of all files. Sorry it took so long.

[aror92]

Reviewable status: 131 of 148 files reviewed, 5 unresolved discussions (waiting on @andrew-polk)

---

src/L10NSharp/LocalizingInfo.cs line 99 at r10 (raw file):

Previously, andrew-polk wrote…

I think this comment needs updating.

Updated

---

src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs line 622 at r10 (raw file):

Previously, andrew-polk wrote…

Any reason to keep this?

Deleted it.

Also, I realized that ApplyLocalization/ReapplyLocalizations/RefreshTooltips were all being called only on the non-Winforms side, so these null-ops were being called. I removed these null-ops here and from the non-Winforms interface and moved calls of these methods from LocalizationManager to LocalizationManagerWinforms so they access the XliffLocalizationManagerWinforms versions of these methods instead.

---

src/L10NSharp/XLiffUtils/XliffLocalizedStringCache.cs line 52 at r10 (raw file):

Previously, andrew-polk wrote…

Seems like it would be better to at least give a Console.WriteLn or something.
And then there's probably no reason to keep the comment.

Done.

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 22 at r10 (raw file):

Previously, andrew-polk wrote…

I'm not seeing the deletion.

Sorry, forgot to push the change!

---

src/L10NSharp.Windows.Forms/LocalizingInfoWinforms.cs line 166 at r10 (raw file):

Previously, andrew-polk wrote…

Not seeing the deletion.

Done.

[andrew-polk]

@andrew-polk reviewed 17 of 17 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aror92)

---

CHANGELOG.md line 76 at r11 (raw file):

    - The properties ApplicationIcon, ToolTipCtrls, LocalizableComponents and StringCache.
    - The methods RegisterComponentForLocalizing, GetShortcutKeyFromStringCache, ApplyLocalizationToIlocalizableComponent, ReapplyLocalizationsToAllComponents, RefreshToolips, ApplyLocalization, ApplyLocalizationsToILocalizableComponent, ApplyLocalizationsToControl, ApplyLocalizedToolTipToControl, HandleToolTipRefChanged, HandleToolTipRefDestroyed, ApplyLocalizationsToToolStripItem, ApplyLocalizationToListViewColumnHeader, and ApplyLocalizationToDataGridViewColumn.

RefreshToolips sounds like a lovely sort of method. Very spring-like.

(you accidentally deleted a t)

[andrew-polk]

@andrew-polk reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aror92)