Unhandled Exception when rapidly switching between pens when drawing

[Joeppie]

Reproduction steps:

When the hotkeys for pens and strokes are being made in rapid succession, this leads to an uncaught exception, which probably could safely be caught, as an alternative to a window which if you click 2 out of 3 buttons exits the application and might scare non-technical users.

`UIThreadException

Oops, gInk crashed! Please include the following information if you plan to contact the developers (a copy of the following information is stored in crash.txt in the application folder):

The operation cannot be performed while the object or control collects or recognizes ink.

Stack Trace: at Microsoft.Ink.InkErrors.ThrowExceptionForInkError(Int32 error) at Microsoft.Ink.InkOverlay.SetWindowInputRectangle(Rectangle windowInputRectangle) at gInk.FormCollection.SelectPen(Int32 pen) in C:\Geovens\gInk\src\FormCollection.cs:line 681 at gInk.FormCollection.tiSlide_Tick(Object sender, EventArgs e) in C:\Geovens\gInk\src\FormCollection.cs:line 1078 at System.Windows.Forms.Timer.OnTick(EventArgs e) at System.Windows.Forms.Timer.TimerNativeWindow.WndProc(Message& m) at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)`

[geovens]

Thank you for the report. As I can't reproduce this crash and it may be complicated to properly fix this, I will simply catch the exception now in hope to make the crash even more rare.

[Joeppie]

I am a C# developer myself, so I could probably actually do a proper fix of this; I can after all also reproduce it! Also, I noticed that if gink.exe is run with a working directory that does not contain its localization files it crashes. Since its a very useful tool, perhaps I should see if I can contribute to it, if time permits!; it's probably better to make a pull request than a bug report :)

[Joeppie]

I am working on this issue and can reproduce it.

@geovens I also see many opportunities for improving the code quality, I will restrain myself, and focus on this one single issue, I will try to provide a pull request when I have a proper and properly tested solution.

Update: the problem is caused by InkOverlay not accepting modifications while ink is being drawn. This means the proper remedy is to include a check; if (!IC.CollectingInk) before calling IC.SetWindowInputRectangle(...)

P.S. The config.ini I cloned master branch with contains a value of 6 for toolbarheight; this should be either 0.06 or 0,06 depending on the users' locale. For such files to be properly transferable I highly recommend to use CultureInfo.InvariantCulture which fixes all decimal and regional settings to the US settings. This would mean that input and output of files is consistent across installations and settings.

[geovens]

Strange, I don't understand how it could be 6 for you instead of 0.06. When I look at current config.ini file in master branch (https://github.com/geovens/gInk/blob/master/bin/config.ini) it shows 0.06.

[Joeppie]

@geovens As for the parsing

This has to do with the CultureInfo setting of the Thread or the one that is implicitly used by the Parse and ToString overloads used in ReadOptions and SaveOptions

Consider this code:

        string value = "0.06";
        Thread.CurrentThread.CurrentCulture = CultureInfo.GetCultureInfo("NL-nl");
        Console.WriteLine($"Dutch interpretation: {double.Parse(value)}");

        Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;
        Console.WriteLine($"US interpretation: {double.Parse(value)}");

And its result:

The best way to avoid such ambiguity is to be explicit in the parse itself (i.e. where the values are read and written from context, as per Console.WriteLine($"US interpretation explicit parse call: {double.Parse(value, CultureInfo.InvariantCulture)}");

Although an easy program-wide fix is to do Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture;

As for the error with switching pens, I have a good fix in mind, this will make unnecessary setting of the SetWindowInputRectangle obsolete; simply use this property:

    private bool _interactWithInk;
    /// <summary>Whether or not the canvas can collect ink or not.</summary>
    public bool InteractWithInk
    {
        get { return _interactWithInk; }
        set
        {
            if (_interactWithInk)
            {
                if (_interactWithInk != value) //turn off ink, but only do so if it's not already off.
                {
                    IC.SetWindowInputRectangle(new Rectangle(0, 0, 1, 1));
                }
            }
            else
            {
                if (_interactWithInk != value) //turn on ink, but do so only if it's not already on.
                {
                    IC.SetWindowInputRectangle(new Rectangle(0, 0, this.Width, this.Height));
                }
            }
            _interactWithInk = value; //Apply value.
        }
    }

in this type of code:

     try
                {
                    InteractWithInk = true;
                }
                catch
                {
                    Thread.Sleep(1);
                    InteractWithInk = true;
                }

It may be that it no longer is possible at all to produce the exception, working with this technique I cannot reproduce the issue no matter how hard I try, without requiring ugly locks or complicated means. Switching between pen 1 and pen 2 for example; there is no need to call IC.SetWindowInputRectangle(new Rectangle(0, 0, this.Width, this.Height)); a second time :)

@geovens: Should I do a pull request for handling the Exception? And/or for a proposed improvement in file handling? I also discovered another bug there, the pan_enabled setting is wrongly saved or consumed I believe, looks to be a simple copy paste error :)

[geovens]

Thank you for the detailed and clear explanation. That culture dependent Parse behavior was beyond my mind. I could not figure this out myself. I believe that all the fixed you mentioned are great. Please make a pull request if you want.

[geovens]

I have added Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture; as the adjustable toolbar height feature was released in v1.1.0

[geovens]

As of the crash when rapidly switching pens, I wonder whether this is related to version of Microsoft.Ink.dll too as discussed in #73

[ccollicutt]

First, thanks for the software! This is the best screen drawing program out there. :)

I can get this to crash reliably. Happens almost every time I use it while doing a demo.

UIThreadException

Oops, gInk crashed! Please include the following information if you plan to contact the developers (a copy of the following information is stored in crash.txt in the application folder):

The operation cannot be performed while the object or control collects or recognizes ink.

Stack Trace:
   at Microsoft.Ink.InkErrors.ThrowExceptionForInkError(Int32 error)
   at Microsoft.Ink.InkOverlay.SetWindowInputRectangle(Rectangle windowInputRectangle)
   at gInk.FormCollection.SelectPen(Int32 pen) in H:\Codes\gInk\src\FormCollection.cs:line 734
   at gInk.FormCollection.tiSlide_Tick(Object sender, EventArgs e) in H:\Codes\gInk\src\FormCollection.cs:line 1145
   at System.Windows.Forms.Timer.OnTick(EventArgs e)
   at System.Windows.Forms.Timer.TimerNativeWindow.WndProc(Message& m)
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)