[Mono-dev] Win Patches for Datagrid (first here) then idle

[Rob Wilkens]

In an automatable way, yes.

By hand, not that hard.

I can try to describe how to test it if you need.

-Rob
On 06/18/2012 05:27 PM, Stifu wrote:
> If it took me 3 tries to get such a simple patch in, I think I'd rather
> resign. :)
>
> Anyway, I'll check out the other patches another day. Is patch 2 hard to
> test? I haven't looked at it yet.
>
>
> Rob Wilkens wrote
>> Thanks for not blindly trusting me :-)
>>
>>
>> On 06/18/2012 05:20 PM, Stifu wrote:
>>> No, it's fine.
>>>
>>>
>>> Rob Wilkens wrote
>>>> Whoops a second problem with the one you posted in the commit you
>>>> mentioned, I think if we're checking >= then we want the results
>>>> reversed...
>>>>
>>>> -Rob
>>>>
>>>> On 06/18/2012 05:06 PM, Rob Wilkens wrote:
>>>>> I couldn't see it before you pushed it, and may have done it wrong
>>>>> myself (I don't recall now)..  :-)
>>>>>
>>>>> I'm sure in the case where this version you wrote would result in a
>>>>> zero
>>>>> result, the zero result is probably OK.
>>>>>
>>>>> The version below is probably good if you're ok with it.
>>>>>
>>>>> -Rob
>>>>>
>>>>> On 06/18/2012 04:58 PM, Stifu wrote:
>>>>>> Hah, that thought did cross my mind.
>>>>>> Did you just wait for me to push it before saying that? :p
>>>>>>
>>>>>> We could go for:
>>>>>>
>>>>>> 			int next_pixel_offset = pixel_offset;
>>>>>>
>>>>>> 			if (CurrentColumn < CurrentTableStyle.GridColumnStyles.Count)	
>>>>>> 			{
>>>>>> 				next_pixel_offset +=
>>>>>> CurrentTableStyle.GridColumnStyles[CurrentColumn].Width;
>>>>>> 			}
>>>>>>
>>>>>>
>>>>>> Rob Wilkens wrote
>>>>>>> Slight issue which probably will never be a problem:
>>>>>>>
>>>>>>> I'd replace the "0" with "pixel_offset" since the normal condition
>>>>>>> for
>>>>>>> that is pixel_offset PLUS the current column (which may be invalid)
>>>>>>> width.. 
>>>>>>>
>>>>>>> I don't think it'll be a problem though and can probably stand as-is.
>>>>>>>
>>>>>>> -Rob
>>>>>>>
>>>>>>> On 06/18/2012 04:39 PM, Stifu wrote:
>>>>>>>> Alright, the first patch is in
>>>>>>>> (https://github.com/mono/mono/commit/42ebb31fc143a171a6a5930bc647627c557842ee).
>>>>>>>> I took the liberty to change the coding style.
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>>
>>>>>>>> Rob Wilkens wrote
>>>>>>>>> This is for Stifu:
>>>>>>>>>
>>>>>>>>> Please follow this sequence when applying or testing the patches
>>>>>>>>> listed
>>>>>>>>> below.  Doing in other order may break things.  If you want me to
>>>>>>>>> create
>>>>>>>>> a unit test for something you don't see a unit test for, let me
>>>>>>>>> know,
>>>>>>>>> but in some cases, clicks are required with a mouse and i'm not
>>>>>>>>> necessarily sure how to create a patch to do that.
>>>>>>>>>
>>>>>>>>> Ok, I've attached the patches i had queued as separate individual
>>>>>>>>> patches, i hope i did this right..  These are from ranges of git
>>>>>>>>> diffs..  Please let me know if there are issues, my feelings won't
>>>>>>>>> be
>>>>>>>>> hurt, i'd rather do this right than do it fast.
>>>>>>>>>
>>>>>>>>> The order to apply them in (then i'll get into what it fixes
>>>>>>>>> after):
>>>>>>>>>
>>>>>>>>> I'd suggest the DataGrid patches first because they are in the
>>>>>>>>> middle
>>>>>>>>> of
>>>>>>>>> everything and get in the way -- except don't apply the
>>>>>>>>> IdleAndDataGrid.Whitespace.Jun10.patch until you've applied ALL the
>>>>>>>>> patches prior to Jun 10 (including idle patches), those patches in
>>>>>>>>> the
>>>>>>>>> Whitespace patch don't fix anything other the prettying up the
>>>>>>>>> code,
>>>>>>>>> but
>>>>>>>>> they depend on both set of patches in sequence..
>>>>>>>>>
>>>>>>>>> So the sequence i'm suggesting they must be applied in if they are
>>>>>>>>> applied at all are:
>>>>>>>>> (1) DataGrid1.Jun3.patch first
>>>>>>>>> (2) DataGrid2.Jun4.patch second
>>>>>>>>> (3) DataGrid3.Novell322563.jun4.patch third
>>>>>>>>> (4) DataGrid4.Novell322154.jun6.patch
>>>>>>>>> -- but don't do the other one i said not to do at this point --
>>>>>>>>> now to the idle fixes, these next ones (5-9) are meant to all be
>>>>>>>>> applied
>>>>>>>>> as part of essentially one patch for it to work, but is broken up
>>>>>>>>> so
>>>>>>>>> you
>>>>>>>>> can see progression.
>>>>>>>>> (5) Idle1-3.Jun2 (sorry for forgetting patch extension), This
>>>>>>>>> contains 3
>>>>>>>>> commits in one but they were all related, and makes life easier by
>>>>>>>>> being
>>>>>>>>> summarized into one like this.
>>>>>>>>> (6) Now you should do IdleAndDataGrid.Whitespace.Jun10.patch
>>>>>>>>> (7) Next, do Idle-Win32IdleEnable.jun11.patch
>>>>>>>>> (8) Idle-RaceConditionFix-Jun12.patch is next
>>>>>>>>> (9) Idle-TestFixForIdle.jun12.patch is last
>>>>>>>>>
>>>>>>>>> There, I have 9 attachments, and above is the sequence to apply
>>>>>>>>> them
>>>>>>>>> in.
>>>>>>>>>
>>>>>>>>> The below numbering system matches the above patch order
>>>>>>>>>
>>>>>>>>> #1: from the commit message:
>>>>>>>>> The sample code in
>>>>>>>>> https://bugzilla.novell.com/show_bug.cgi?id=MONO79788 was crashing
>>>>>>>>> on
>>>>>>>>> me
>>>>>>>>> if I clicked on a row header (where the + was). I investigated and
>>>>>>>>> found
>>>>>>>>> that it was because, when the table had no data to display yet, and
>>>>>>>>> if
>>>>>>>>> you clicked on a row header (that's the area to the left of what
>>>>>>>>> would
>>>>>>>>> be the data), as part of assigning the current cell, it would call
>>>>>>>>> ensure cell visibility function, which would call
>>>>>>>>> ScrollToColumnInPixels
>>>>>>>>> which would try to get the next pixel offset by looking at
>>>>>>>>> CurrentTableStyle.GridColumnStyles[CurrentColumn].Width, but when
>>>>>>>>> no
>>>>>>>>> data was being displayed there were no columns. So while current
>>>>>>>>> column
>>>>>>>>> had a value of zero, there were no items in the GridColumnStyles
>>>>>>>>> Array/List, even at zero index. The fix for this was before
>>>>>>>>> indexing
>>>>>>>>> into GridColumnStyles to Check The Length to make sure we're not
>>>>>>>>> going
>>>>>>>>> beyond its bounds. It is probably perfectly acceptable if we're
>>>>>>>>> beyond
>>>>>>>>> the bounds to just leave this value at zero for the offset.
>>>>>>>>>
>>>>>>>>> #2 From the commit message:
>>>>>>>>> Xamaring bug 5511: This fixes this and some side issues..
>>>>>>>>> First, fixed the crash by creating two additional stacks for when
>>>>>>>>> navigating to and from other sub tables... Both were 'style' stacks
>>>>>>>>> which tracked per column styles. Those needed to be updated and
>>>>>>>>> reset
>>>>>>>>> on
>>>>>>>>> a per table basis. Second, When navigating forward or back, EndEdit
>>>>>>>>> needed to be called so that we don't leave an editing cell open
>>>>>>>>> when
>>>>>>>>> we
>>>>>>>>> navigate, otherwise there was the possibility and reality that the
>>>>>>>>> edited cell would still be visible and editing on the next table
>>>>>>>>> either
>>>>>>>>> forward or back. To recreate this on the sample code, presuming you
>>>>>>>>> can
>>>>>>>>> get past the initial crash which was fixed here, this could be
>>>>>>>>> illustrated without the endedits that were added as follows : Run:
>>>>>>>>> 1.
>>>>>>>>> click + 2. click pb 3. click + 4. click pb 3. click + 5. click pd
>>>>>>>>> 6.
>>>>>>>>> click back 6. click pc 7. See 0 highlighted in column header from
>>>>>>>>> previous table Finally, I modified some previous submissions on a
>>>>>>>>> related problem so that they had more "Love for Spaces"
>>>>>>>>> (whitespace)
>>>>>>>>> without changing the actual code other than that.
>>>>>>>>>
>>>>>>>>> The above may have fixed, i think it was 5510 too.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> #3: From the commit
>>>>>>>>> This solves PART of Novell Bugzilla Issue #322563
>>>>>>>>> https://bugzilla.novell.com/show_bug.cgi?id=322563 What this
>>>>>>>>> accomplishes is that it hides the non browsable columns (columns
>>>>>>>>> that
>>>>>>>>> were not part of the original dataset, in the test case) from view
>>>>>>>>> in
>>>>>>>>> the DataGrid. Unfortunately, i can't see an obvious way to hide
>>>>>>>>> the'parent rows' display of those non browsable columns value. That
>>>>>>>>> is
>>>>>>>>> if You viewed a subtable off hidden field pb_Id=0 it would show
>>>>>>>>> that
>>>>>>>>> value (pb_Id=0) on the top of the datagrid where it shows the
>>>>>>>>> previous
>>>>>>>>> rows. The remaining issue seems to be a non major issue since it is
>>>>>>>>> a
>>>>>>>>> non functional issue.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> #4: From the commit
>>>>>>>>> Novell #323154
>>>>>>>>> Decided to include this in same branch (same pull request) as
>>>>>>>>> earlier
>>>>>>>>> changes due to it affecting the same DataGrid.cs -- i didn't want
>>>>>>>>> any
>>>>>>>>> conflicts. Here's a copy of what i wrote up earlier about this: the
>>>>>>>>> problem report is here:
>>>>>>>>> https://bugzilla.novell.com/show_bug.cgi?id=323154 I found by
>>>>>>>>> deduction
>>>>>>>>> that the repaint wasn't cancelling the active edit box after the
>>>>>>>>> row
>>>>>>>>> was
>>>>>>>>> deleted .. So while the table updated, the edit box with the old
>>>>>>>>> value
>>>>>>>>> didn't go away... The repaint was initiated from an Invalidate call
>>>>>>>>> which stack trace looked something like this:
>>>>>>>>> System.Windows.Forms.DataGrid.CalcAreasAndInvalidate() at
>>>>>>>>> System.Windows.Forms.DataGrid.RecreateDataGridRows(Boolean recalc)
>>>>>>>>> at
>>>>>>>>> System.Windows.Forms.DataGrid.OnListManagerItemChanged(System.Object
>>>>>>>>> sender, System.Windows.Forms.ItemChangedEventArgs e) at
>>>>>>>>> System.Windows.Forms.CurrencyManager.OnItemChanged(System.Windows.Forms.ItemChangedEventArgs
>>>>>>>>> e) at System.Windows.Forms.CurrencyManager.UpdateIsBinding() at
>>>>>>>>> System.Windows.Forms.CurrencyManager.ListChangedHandler(System.Object
>>>>>>>>> sender, System.ComponentModel.ListChangedEventArgs e) at
>>>>>>>>> System.Data.DataView.OnListChanged(System.ComponentModel.ListChangedEventArgs
>>>>>>>>> e) at System.Data.DataView.OnRowDeleted(System.Object sender,
>>>>>>>>> System.Data.DataRowChangeEventArgs args) at
>>>>>>>>> System.Data.DataTable.OnRowDeleted(System.Data.DataRowChangeEventArgs
>>>>>>>>> e)
>>>>>>>>> at System.Data.DataTable.DeletedDataRow(System.Data.DataRow dr,
>>>>>>>>> DataRowAction action) at System.Data.DataRow.Delete() at
>>>>>>>>> System.Data.DataView.Delete(Int32 index) at
>>>>>>>>> System.Data.DataRowView.Delete() at grid.ProcessCmdKey(Message
>>>>>>>>> ByRef
>>>>>>>>> msg, Keys keyData) (etc) ProcessCmdKey is from user code in sample
>>>>>>>>> in
>>>>>>>>> bug report... After the delete (as seen above), the first thing the
>>>>>>>>> DataGrid gets back is an OnListManagerItemChanged... Before that
>>>>>>>>> would
>>>>>>>>> call RecreateDataGridRows(), if it was going to do that, i inserted
>>>>>>>>> a
>>>>>>>>> check to see if we're editing, and if so, i cancel the edit
>>>>>>>>> (because
>>>>>>>>> we're reloading dataset), here is a summary of what my patch will
>>>>>>>>> look
>>>>>>>>> like in ONListManagerItemChanged in DataGrid.cs in
>>>>>>>>> System.Windows.Forms
>>>>>>>>> directory: if (rows == null || RowsCount != rows.Length -
>>>>>>>>> (ShowEditRow ?
>>>>>>>>> 1 : 0)) + { + if (is_editing) + CancelEditing ();
>>>>>>>>> RecreateDataGridRows
>>>>>>>>> (true); + } This solved the problem reported. It is now identical
>>>>>>>>> to
>>>>>>>>> windows .net behavior from what i can see, as far as this problem
>>>>>>>>> report
>>>>>>>>> goes anyway. MS .NET Crashes as well as mono in the sample code if
>>>>>>>>> you
>>>>>>>>> press a key twice to delete two rows when there was only one row to
>>>>>>>>> delete (index out of range). This is not necessarily a good thing,
>>>>>>>>> but
>>>>>>>>> it's user code from the bug report which causes that, not anything
>>>>>>>>> inherently different in mono.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> #5-#9 are all about fixing bug From Novell #321541 which if i have
>>>>>>>>> the
>>>>>>>>> right number is adding the ability to have the idle event handler
>>>>>>>>> only
>>>>>>>>> send idle events to the thread the idle handler was assigned in. 
>>>>>>>>> So
>>>>>>>>> if
>>>>>>>>> you have 2 threads, and they each assigned an idle event handler,
>>>>>>>>> they
>>>>>>>>> would each get their own idle event handler called when that thread
>>>>>>>>> went
>>>>>>>>> idle.  Or if only one thread had an idle event handler assigned
>>>>>>>>> that
>>>>>>>>> same idle handler wouldn't be called for EVERY thread, it would
>>>>>>>>> only
>>>>>>>>> be
>>>>>>>>> called on the thread it was assigned on.  This is so it more
>>>>>>>>> closely
>>>>>>>>> matches the windows .net behavior.
>>>>>>>>>
>>>>>>>>> I'll include each of the individual commit messages though they
>>>>>>>>> were
>>>>>>>>> all
>>>>>>>>> towards the same goal
>>>>>>>>>
>>>>>>>>> #5 had three commits:
>>>>>>>>> This addresses a 6-year old Novell bugzilla issue: 321541...
>>>>>>>>> Created a hashtable of per-thread event handlers for idle..
>>>>>>>>> Assigned
>>>>>>>>> in
>>>>>>>>> to that hashtable when the regular Idle eventhandler was assigned
>>>>>>>>> by
>>>>>>>>> mapping it by ManagedThreadId. The hashtable had to use a separate
>>>>>>>>> object (different class) rather than an EventHandler directly,
>>>>>>>>> because
>>>>>>>>> an eventhandler apparently cannot be assigned to another object, it
>>>>>>>>> can
>>>>>>>>> only be a part of a class. It also couldn't be checked for null or
>>>>>>>>> called from outside the class so i handled that as appropriate (by
>>>>>>>>> secondary callers). This has been checked against the code in
>>>>>>>>> 321541
>>>>>>>>> and
>>>>>>>>> the code appears to work fine now. I have also confirmed no new
>>>>>>>>> unit
>>>>>>>>> test failures have been introduced by this change. There were 3
>>>>>>>>> failures
>>>>>>>>> before, and are three failures now. Also, This includes a unit
>>>>>>>>> test,
>>>>>>>>> which will fail without this patch. Here's a shortcut to the Novell
>>>>>>>>> bug:
>>>>>>>>> https://bugzilla.novell.com/show_bug.cgi?id=321541
>>>>>>>>> Per suggestion, Changed a Hashtable to a generic dictionary.
>>>>>>>>>
>>>>>>>>> This change is to properly use the GenericDictionary to use
>>>>>>>>> the EventHandler type directly rather than the temporary class that
>>>>>>>>> was
>>>>>>>>> being used in my first attempt at this.
>>>>>>>>> (so some of the changes from the first commit you don't see here
>>>>>>>>> because
>>>>>>>>> they were thrown away by the second and third in a rewrite)
>>>>>>>>>
>>>>>>>>> #6 is a bunch of whitespace fixes for Datagrid and idle which has
>>>>>>>>> to
>>>>>>>>> be
>>>>>>>>> applied at this point in sequence.
>>>>>>>>>
>>>>>>>>> #7 Enables the idle message to work on Win32.  When i tested on
>>>>>>>>> Win32
>>>>>>>>> i
>>>>>>>>> realized the Idle event handler was never called, so i fixed that
>>>>>>>>> so
>>>>>>>>> when it went idle it would call it.
>>>>>>>>>
>>>>>>>>> Here's the commit from #7:
>>>>>>>>> This patch will enable the idle event to be called when the
>>>>>>>>> applicati...
>>>>>>>>> ...on is idle on Win32. This was necessary to make an earlier unit
>>>>>>>>> test
>>>>>>>>> from the same set of patches work on win32. Plus, it's a good idea.
>>>>>>>>>
>>>>>>>>> #8 This is important, essentially this had a lock set to stop a
>>>>>>>>> race
>>>>>>>>> condition that was happening with the test. There was an 'if
>>>>>>>>> something
>>>>>>>>> == null assign something to something new... And two threads were
>>>>>>>>> hitting this code at the same time and this was causing one thread
>>>>>>>>> to
>>>>>>>>> assign it, and before it would start working with it, thread 2
>>>>>>>>> would
>>>>>>>>> reassign it, and this resulted in a stack dump (exception) in
>>>>>>>>> add_Idle,
>>>>>>>>> this fix seemed to stop that.
>>>>>>>>>
>>>>>>>>> Here's the commit message
>>>>>>>>> This code fixes a possible race condition which during my testing
>>>>>>>>> seemed
>>>>>>>>> to be hit about once every twenty runs or so. Sometimes if two
>>>>>>>>> threads
>>>>>>>>> were assigning the idle handler at the same time, and it's the
>>>>>>>>> first
>>>>>>>>> assignment, they will both try to assign a new dictionary. This
>>>>>>>>> resulted
>>>>>>>>> in funny behavior, such as immediately after an add, the item
>>>>>>>>> wouldn't
>>>>>>>>> be found. After applying this fix, a lock around that particular
>>>>>>>>> check/assignment, I reran the tests about 50-75 times and never ran
>>>>>>>>> into
>>>>>>>>> this race condition again.
>>>>>>>>>
>>>>>>>>> #9 the last one is just some test changes after what i was told
>>>>>>>>> about
>>>>>>>>> mono requiring that all threads create the forms in the same
>>>>>>>>> thread.
>>>>>>>>> This only seemed to be a problem on windows, from what i recall,
>>>>>>>>> and
>>>>>>>>> only occasionally.
>>>>>>>>>
>>>>>>>>> The commit message reads:
>>>>>>>>> Due to a WinForms requriement (which only seems to occasionally be
>>>>>>>>> a
>>>>>>>>> problem on Windows), where all Controls (including Forms) must be
>>>>>>>>> created on One Thread, and then to do work on them from other
>>>>>>>>> threads
>>>>>>>>> only by use of invoke ( According to :
>>>>>>>>> http://www.mono-project.com/FAQ:_Winforms ), I modified my unit
>>>>>>>>> test
>>>>>>>>> accordingly to be in compliance.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Mono-devel-list mailing list
>>>>>>>>> Mono-devel-list at .ximian
>>>>>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>>>>>>>
>>>>>>>> --
>>>>>>>> View this message in context:
>>>>>>>> http://mono.1490590.n4.nabble.com/Win-Patches-for-Datagrid-first-here-then-idle-tp4650027p4650048.html
>>>>>>>> Sent from the Mono - Dev mailing list archive at Nabble.com.
>>>>>>>> _______________________________________________
>>>>>>>> Mono-devel-list mailing list
>>>>>>>> Mono-devel-list at .ximian
>>>>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>>>>> _______________________________________________
>>>>>>> Mono-devel-list mailing list
>>>>>>> Mono-devel-list at .ximian
>>>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>>>>>
>>>>>> --
>>>>>> View this message in context:
>>>>>> http://mono.1490590.n4.nabble.com/Win-Patches-for-Datagrid-first-here-then-idle-tp4650027p4650052.html
>>>>>> Sent from the Mono - Dev mailing list archive at Nabble.com.
>>>>>> _______________________________________________
>>>>>> Mono-devel-list mailing list
>>>>>> Mono-devel-list at .ximian
>>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>> _______________________________________________
>>>> Mono-devel-list mailing list
>>>> Mono-devel-list at .ximian
>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>>
>>> --
>>> View this message in context:
>>> http://mono.1490590.n4.nabble.com/Win-Patches-for-Datagrid-first-here-then-idle-tp4650027p4650057.html
>>> Sent from the Mono - Dev mailing list archive at Nabble.com.
>>> _______________________________________________
>>> Mono-devel-list mailing list
>>> Mono-devel-list at .ximian
>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>> _______________________________________________
>> Mono-devel-list mailing list
>> Mono-devel-list at .ximian
>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>
>
> --
> View this message in context: http://mono.1490590.n4.nabble.com/Win-Patches-for-Datagrid-first-here-then-idle-tp4650027p4650059.html
> Sent from the Mono - Dev mailing list archive at Nabble.com.
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list