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

Stifu stifu at free.fr
Mon Jun 18 21:14:36 UTC 2012 

"I couldn't see it before you pushed it, and may have done it wrong
myself (I don't recall now)..  :-)"

Well yeah, I only reformatted your patch, but didn't add in the zero, it was
there in the first place. You said it yourself, actually: "It is probably
perfectly acceptable if we're beyond the bounds to just leave this value at
zero for the offset."

Anyway, I'm pushing the modified version, as I agree it makes more sense and
is more readable, even if it makes no difference.

I know you sometimes only realize things afterward, but I'd like to think
things through to avoid creating noise as much as possible in the future. I
just feel bad polluting the version history.


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-tp4650027p4650054.html
Sent from the Mono - Dev mailing list archive at Nabble.com.

More information about the Mono-devel-list mailing list