Terminal: iTerm color schemes once more are failing to parse colors

[SneWs]

Not sure if I am supposed to request to get this re-opened or if I should create another bug. But this is the same as in #2

• Your Windows build number: (Type ver at a Windows Command Prompt)
  Microsoft Windows [Version 10.0.17763.253]

• What you're doing and what's happening: (Copy & paste specific commands and their output, or include screen shots)
  Running:
  PS C:ToolsColorTool> .ColorTool.exe C:ToolsColorTool/schemes/molokai.itermcolors
  Error loading ini file "C:ToolsColorTool/schemes/molokai.itermcolors"
  for key "DARK_BLACK"
  the value "" is invalid

The actual file contents can be found in this gist:
https://gist.github.com/SneWs/4bbb50dce9a132a4dcbd07e3e2c85c44

• What's wrong / what should be happening instead:
  The colors should be successfully parsed and set without any error messages being rendered.

[zadjii-msft]

What colortool version are you running? (colortool -v)

[SneWs]

I built it from master to see if the bug still where present, and that it was. Before running my locally built version it was version:

ColorTool.exe -v
colortool v1.0.1810.02002

[cooperpellaton]

Can confirm that I also see this problem across all .itermcolors files. Running ColorTool version v1.0.1810.02002.

[SneWs]

Did some experimentation. Seems that if I change my regional formatting to US from Swedish it successfully parses the file I put in a gist above.

So to repro this issue, have Windows 10 running with US English as language and change the regional formatting to Swedish.

[cooperpellaton]

My system language is also US English.

[zadjii-msft]

Okay, knowing that setting the regional formatting to swedish will make this repro will help make this possible to actually investigate. I figured #1/#2 would have fixed this, but I'd need to walk through it in a debugger to find out what's broken this time.

I've filed msft:20266491 to make sure I get to this at some point, but unfortunately it's pretty low on my backlog at the time. If someone wants to help debug why this is broken, a PR would be much appreciated 😊

[j4james]

I think the regional formatting thing is a bit of red herring. The problem is that the program supports multiple file formats, and it achieves this by attempting to open the file in each of the supported formats until one of them succeeds. This relies on the parsers failing silently when given the wrong file type as input, but that's not the case for the Ini parser, and thus you get this spurious error message.

What is needed is some extra validation at the start of the Ini parser which will let it fail silently when given a file of the wrong type. Or possibly it could suppress its error messages until it has managed to parse at least one line successfully.

Note that you can also bypass the error by omitting the file extension when loading the scheme. For example calling it like this:

ColorTool schemes\molokai

That's because each parser tries to add their default file extension when matching the file name. So the Xml parser can find the file using the default itermcolors extension, but the Ini parser won't find anything with an ini extension, so won't even attempt to parse it.

This only works when you refer to the scheme via a relative path, though. And of course it's not really a satisfactory solution to the problem. I just thought it worth mentioning as a temporary workaround.

[j4james]

In case it's of any help, below is a trivial patch that simply suppresses the Ini error message if it hasn't managed to read at least one string successfully. I don't know if that's the best solution to the problem, but it's at least an easy fix.

diff --git a/tools/ColorTool/ColorTool/IniSchemeParser.cs b/tools/ColorTool/ColorTool/IniSchemeParser.cs
index dc4b8d2..4f77300 100644
--- a/tools/ColorTool/ColorTool/IniSchemeParser.cs
+++ b/tools/ColorTool/ColorTool/IniSchemeParser.cs
@@ -96,7 +96,7 @@ namespace ColorTool
                 if (tableStrings[i].Length <= 0)
                 {
                     success = false;
-                    if (reportErrors)
+                    if (reportErrors && i > 0)
                     {
                         Console.WriteLine(string.Format(Resources.IniParseError, filename, name, tableStrings[i]));
                     }

[cooperpellaton]

@j4james Based on your diagnosis, wouldn't it be easier/better to prevent the silent fail (and suppression) in the first place by checking the file format and encoding _before_ opening and matching it against the parser? Doing it this way would also prevent the scenario of opening a misformatted file, etc.

[j4james]

@cooperpellaton Possibly better, but it's hard to imagine anything easier than the five character change above. :) How exactly does one go about checking whether something is a valid ini file? The simplest solution I could think of was to call GetPrivateProfileSectionNames on the file and see whether that returned anything. I don't know if that's a good approach to take, but I've included another patch below showing how it could work.

diff --git a/tools/ColorTool/ColorTool/IniSchemeParser.cs b/tools/ColorTool/ColorTool/IniSchemeParser.cs
index dc4b8d2..45d4834 100644
--- a/tools/ColorTool/ColorTool/IniSchemeParser.cs
+++ b/tools/ColorTool/ColorTool/IniSchemeParser.cs
@@ -15,6 +15,8 @@ namespace ColorTool
     class IniSchemeParser : ISchemeParser
     {
         [DllImport("kernel32")]
+        private static extern int GetPrivateProfileSectionNames(StringBuilder retVal, int size, string filePath);
+        [DllImport("kernel32")]
         private static extern int GetPrivateProfileString(string section, string key, string def, StringBuilder retVal, int size, string filePath);

         // These are in Windows Color table order - BRG, not RGB.
@@ -83,6 +85,8 @@ namespace ColorTool
             string filename = FindIniScheme(schemeName);
             if (filename == null) return null;

+            if (GetPrivateProfileSectionNames(new StringBuilder(3), 3, filename) <= 0) return null;
+
             string[] tableStrings = new string[COLOR_TABLE_SIZE];
             uint[] colorTable = null;

[cooperpellaton]

@j4james Understood and agreed, I guess it was more of a conceptual question as to whether we should just do something hastily that solves the problem or which will be more comprehensive going forward.

Re: parsing INIs I'd be partial to implementing this if we decide to do it that way.