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 > [email protected] > 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 [email protected] http://lists.ximian.com/mailman/listinfo/mono-devel-list
