[Mono-dev] Win Patches for Datagrid (first here) then idle
Rob Wilkens
robwilkens at gmail.com
Mon Jun 18 01:05:53 UTC 2012
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/20120617/63e08a5a/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DataGrid1.Jun3.patch
Type: text/x-patch
Size: 916 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DataGrid2.Jun4.patch
Type: text/x-patch
Size: 2584 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DataGrid3-Novell322563.jun4.patch
Type: text/x-patch
Size: 1167 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DataGrid4-Novell323154.jun6.patch
Type: text/x-patch
Size: 640 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0011.bin>
-------------- next part --------------
diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUICarbon.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUICarbon.cs
index 18afe41..957664b 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUICarbon.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUICarbon.cs
@@ -94,7 +94,6 @@ namespace System.Windows.Forms {
static readonly object queuelock = new object ();
// Event Handlers
- internal override event EventHandler Idle;
#endregion
#region Constructors
@@ -769,8 +768,10 @@ namespace System.Windows.Forms {
internal override void RaiseIdle (EventArgs e)
{
- if (Idle != null)
- Idle (this, e);
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if (Idle_Threads!=null && Idle_Threads.ContainsKey(id) && Idle_Threads[id]!=null){
+ Idle_Threads[id](this,e);
+ }
}
internal override IntPtr InitializeDriver() {
@@ -1366,9 +1367,13 @@ namespace System.Windows.Forms {
lock (queuelock) {
if (MessageQueue.Count <= 0) {
- if (Idle != null)
- Idle (this, EventArgs.Empty);
- else if (TimerList.Count == 0) {
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if (Idle_Threads!=null &&
+ Idle_Threads.ContainsKey(id) &&
+ Idle_Threads[id]!=null)
+ {
+ Idle_Threads[id](this,EventArgs.Empty);
+ } else if (TimerList.Count == 0) {
ReceiveNextEvent (0, IntPtr.Zero, 0.15, true, ref evtRef);
if (evtRef != IntPtr.Zero && target != IntPtr.Zero) {
SendEventToEventTarget (evtRef, target);
diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIDriver.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIDriver.cs
index b79b967..752b16c 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIDriver.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIDriver.cs
@@ -29,6 +29,7 @@
using System.Drawing;
using System.Threading;
using System.Runtime.InteropServices;
+using System.Collections.Generic;
namespace System.Windows.Forms {
internal abstract class XplatUIDriver {
@@ -269,7 +270,32 @@ namespace System.Windows.Forms {
}
#endregion // XplatUI Driver Properties
- internal abstract event EventHandler Idle;
+ internal Dictionary<int,EventHandler> Idle_Threads;
+ internal virtual event EventHandler Idle {
+ add {
+ if (Idle_Threads==null){
+ Idle_Threads=new Dictionary<int,EventHandler>();
+ }
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if (!Idle_Threads.ContainsKey(id)){
+ EventHandler hnd=null;
+ Idle_Threads.Add(id,hnd);
+ }
+ Idle_Threads[id]+=value;
+ }
+ remove {
+ if (Idle_Threads!=null){
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if (Idle_Threads.ContainsKey(id)){
+ Idle_Threads[id]-=value;
+ if (Idle_Threads[id]==null)
+ {
+ Idle_Threads.Remove(id);
+ }
+ }
+ }
+ }
+ }
#region XplatUI Driver Methods
internal abstract void AudibleAlert(AlertType alert);
diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIWin32.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIWin32.cs
index efc464d..ff77b8a 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIWin32.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIWin32.cs
@@ -1239,9 +1239,12 @@ namespace System.Windows.Forms {
}
internal override void RaiseIdle (EventArgs e)
- {
- if (Idle != null)
- Idle (this, e);
+ {
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if (Idle_Threads!=null && Idle_Threads.ContainsKey(id) && Idle_Threads[id]!=null)
+ {
+ Idle_Threads[id](this,e);
+ }
}
internal override Keys ModifierKeys {
@@ -3323,7 +3326,6 @@ namespace System.Windows.Forms {
Win32SetForegroundWindow(handle);
}
- internal override event EventHandler Idle;
#endregion // Public Static Methods
#region Win32 Imports
diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11.cs
index 9b6c130..4b578a9 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11.cs
@@ -1697,8 +1697,11 @@ namespace System.Windows.Forms {
}
if (pending == 0 && allowIdle) {
- if ((queue == null || queue.DispatchIdle) && Idle != null) {
- Idle (this, EventArgs.Empty);
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if ((queue == null || queue.DispatchIdle) && Idle_Threads!=null && Idle_Threads.ContainsKey(id) && Idle_Threads[id]!=null)
+ {
+ Idle_Threads[id](this,EventArgs.Empty);
+
}
lock (XlibLock) {
@@ -2594,8 +2597,11 @@ namespace System.Windows.Forms {
#region Public Static Methods
internal override void RaiseIdle (EventArgs e)
{
- if (Idle != null)
- Idle (this, e);
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if (Idle_Threads!=null && Idle_Threads.ContainsKey(id) && Idle_Threads[id]!=null)
+ {
+ Idle_Threads[id](this,e);
+ }
}
internal override IntPtr InitializeDriver()
@@ -6350,7 +6356,6 @@ namespace System.Windows.Forms {
#endregion // Public Static Methods
#region Events
- internal override event EventHandler Idle;
#endregion // Events
diff --git a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11GTK.cs b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11GTK.cs
index 77e89c4..1103c85 100644
--- a/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11GTK.cs
+++ b/mcs/class/Managed.Windows.Forms/System.Windows.Forms/XplatUIX11GTK.cs
@@ -1199,8 +1199,10 @@ namespace System.Windows.Forms {
}
if (pending == 0) {
- if (Idle != null) {
- Idle (this, EventArgs.Empty);
+ int id=Thread.CurrentThread.ManagedThreadId;
+ if (Idle_Threads!=null && Idle_Threads.ContainsKey(id) && Idle_Threads[id]!=null){
+ {
+ Idle_Threads[id](this,EventArgs.Empty);
}
lock (XlibLock) {
@@ -4352,7 +4354,6 @@ namespace System.Windows.Forms {
#endregion // Public Static Methods
#region Events
- internal override event EventHandler Idle;
#endregion // Events
#region X11 Imports
diff --git a/mcs/class/Managed.Windows.Forms/Test/System.Windows.Forms/FormEventTest.cs b/mcs/class/Managed.Windows.Forms/Test/System.Windows.Forms/FormEventTest.cs
index c2f3c37..2753bcf 100644
--- a/mcs/class/Managed.Windows.Forms/Test/System.Windows.Forms/FormEventTest.cs
+++ b/mcs/class/Managed.Windows.Forms/Test/System.Windows.Forms/FormEventTest.cs
@@ -623,7 +623,66 @@ namespace MonoTests.System.Windows.Forms
eventhandled = false;
_form.MinimumSize = new Size(100, 100);
Assert.AreEqual (true, eventhandled, "#A10");
- }
+ }
+
+ /**
+ ** This next test is in response to a bug report
+ ** which pointed out that the idle events were being
+ ** sent to every thread rather than just the thread
+ ** they were assigned on.
+ **
+ ** Report: https://bugzilla.novell.com/show_bug.cgi?id=321541
+ **/
+ private static Form _form1_OneIdlePerThread=null;
+ private static Form _form2_OneIdlePerThread=null;
+ private static int count1_OIPT=0;
+ private static int count2_OIPT=0;
+ private static ThreadStart OIPT_ThreadStart2;
+ private static Thread OIPT_Thread2;
+ private static int oipt_t1=0;
+ private static int oipt_t2=0;
+ [Test]
+ public void OneIdlePerThread(){
+ Thread t=Thread.CurrentThread;
+ oipt_t1=t.ManagedThreadId;
+ count1_OIPT=0;count2_OIPT=0;
+ _form1_OneIdlePerThread=new Form();
+
+ OIPT_ThreadStart2=new ThreadStart(TIPT_Two);
+ OIPT_Thread2=new Thread(OIPT_ThreadStart2);
+ OIPT_Thread2.IsBackground=true;
+ OIPT_Thread2.ApartmentState=ApartmentState.STA;
+ OIPT_Thread2.Start();
+ Application.Idle+=new EventHandler(TestIdlePerThread);
+ Application.Run(_form1_OneIdlePerThread);
+ Thread.Sleep(1000);//allow other thread to finish
+ Assert.AreEqual (true, count1_OIPT==1, "#Idle: idle #1 hit too many times");
+ Assert.AreEqual (true, count2_OIPT==1, "#Idle: idle #2 hit too many times");
+ }
+ public static void TIPT_Two(){
+ Thread t=Thread.CurrentThread;
+ oipt_t2=t.ManagedThreadId;
+ _form2_OneIdlePerThread=new Form();
+ Application.Idle+=new EventHandler(TestIdlePerThread2);
+ Application.Run(_form2_OneIdlePerThread);
+ }
+ public static void TestIdlePerThread(object o, EventArgs e){
+ Thread t=Thread.CurrentThread;
+ count1_OIPT++;
+ Application.Idle-=new EventHandler(TestIdlePerThread);
+ if (_form1_OneIdlePerThread!=null)
+ _form1_OneIdlePerThread.Close();
+ Assert.AreEqual (true, oipt_t1==t.ManagedThreadId, "#Idle:Wrong Thread-t1");
+ }
+ public static void TestIdlePerThread2(object o, EventArgs e){
+ Thread t=Thread.CurrentThread;
+ count2_OIPT++;
+ Application.Idle-=new EventHandler(TestIdlePerThread2);
+ if (_form2_OneIdlePerThread!=null)
+ _form2_OneIdlePerThread.Close();
+ Assert.AreEqual (true, oipt_t2==t.ManagedThreadId, "#Idle:Wrong Thread-t2");
+ }
+
}
[TestFixture]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IdleAndDataGrid.Whitespace.Jun10.patch
Type: text/x-patch
Size: 12481 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Idle-RaceConditionFix-Jun12.patch
Type: text/x-patch
Size: 1170 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Idle-TestFixForIdle.jun12.patch
Type: text/x-patch
Size: 2349 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Idle-Win32IdleEnable.jun11.patch
Type: text/x-patch
Size: 1150 bytes
Desc: not available
URL: <http://lists.ximian.com/pipermail/mono-devel-list/attachments/20120617/63e08a5a/attachment-0015.bin>
More information about the Mono-devel-list
mailing list