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

[Rob Wilkens]

Now that you modified patch 1, I had to modify patch 2 so it would apply
correctly, I've attached it (I'm testing just like you're testing, i'm
starting with a clean mono tree and applying the patches one by one as
you apply them or just before you would in this case)

I just deleted a section that fixed up whitespace in something you
already modified yourself.

My next e-mail should be with the unit tests as a patch, which i will
submit after i finish my own testing first as a standalone unit test,
then i'll build it into the unit tests.

-Rob

On 06/17/2012 09:05 PM, 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120618/c5d0d021/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DataGrid2.Jun4.patch
Type: text/x-patch
Size: 2098 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120618/c5d0d021/attachment-0001.bin>