[Mono-dev] Patches for mono-winforms

Stifu stifu at free.fr
Mon Jun 4 06:03:53 UTC 2012 

"Also, as far as I can tell, patches 2, 4, and 5 are really small, and defy
the creation of unit tests anyway -- it should be enough for someone
knowledgeable about WinForms to decide if these changes are correct."

No matter how small these patches are, and no matter if they're obviously
correct. If something can be tested, it should be tested. :) I've committed
several one-line patches, each of them coming with their unit test, so I've
spent more time writing tests than fixes.

This also ensures that no one breaks the tests / feature later on, by
working on the code, trying to fix another bug, moving code around,
refactoring... I guess you could easily come up with various stats showing
that fixes without tests are XX% more likely to get broken again later.
Tests also ensure we're on the same level across all OSes or machine type
(for example, we could imagine patch number X does not fix the bug on Mac
for some reason, and so this problem would be detected thanks to the test).
To sum things up, tests are what makes a framework mature and reliable.
Without them, you're just applying temporary patches.

I'll check all of this out when I have more time. And I don't mind coming up
with the tests myself, if I can figure out something. As for patch 1, I'll
just check if I can reproduce the bug on Linux.


Steven Boswell II wrote
> 
> Yes, the unit test fails for me without my patch -- I made darn sure of
> that after wasting your time. ;-)  The additions to my unit test ensure
> that the EditingControlShowing event is getting called.  Without my patch,
> Mono never calls that event.  So if it's succeeding for you...I'm totally
> lost.
> 
> Let me know if you need any more info, or any other changes, so that you
> can commit patch 3.
> 
> Also, as far as I can tell, patches 2, 4, and 5 are really small, and defy
> the creation of unit tests anyway -- it should be enough for someone
> knowledgeable about WinForms to decide if these changes are correct.  I
> know that it brings Mono's behavior in line with .NET.  Please let me know
> if I should do anything else with these patches.
> 
> For the time being, I seem to be out of bugs, and am working on
> annoyances...that's a good sign. :-)  Enclosed is my latest change;
> there's no unit test because it requires user interaction to even see the
> problem.  Before, getting a data-grid-view combo-box to drop down its menu
> took three mouse-clicks.  Now, it only takes one.  This brings the
> behavior in line with .NET.  Let me know what you think of this change.
> 
> Steven Boswell
> 
> 
> ________________________________
>  From: Stifu <stifu@>
> To: mono-devel-list at .ximian 
> Sent: Sunday, June 3, 2012 3:20 PM
> Subject: Re: [Mono-dev] Patches for mono-winforms
>  
> Nope, I still can't get the tests to fail. :\
> Does it fail for you (when unpatched)? It's possible the bug doesn't
> affect
> Mono on Windows, but I'd like to make sure of it.
> 
> Can't see anything wrong with patch 3 at first sight.
> 
> 
> Steven Boswell II wrote
>> 
>> Sorry about that.  I've added code to make sure the event-handler gets
>> called.  Enclosed is another attempt.
>> Also, do you have any further comments about patch 3?  For your
>> convenience, I've enclosed another copy.
>> 
>> 
>> ________________________________
>>  From: Stifu <stifu@>
>> To: mono-devel-list at .ximian 
>> Sent: Sunday, June 3, 2012 8:58 AM
>> Subject: Re: [Mono-dev] Patches for mono-winforms
>>  
>> I checked out your unit tests for patch 1.
>> On Mono on Windows, they already pass even without your patch. I cannot
>> check Linux right now.
>> 
>> Have you double checked that your tests fail without your patch?
>> Thanks for clearing that up.
>> 
>> 
>> Steven Boswell II wrote
>>> 
>>> Crap, I didn't notice that InitializeEditingControl() was public.
>>> 
>>> My whole motivation for splitting up that method was to make sure that
>>> the
>>> style modified by the EditingControlShowing event-handler would be
>>> applied
>>> to the cell.  But that's done a few lines later, by the call to
>>> "dgvEditingControl.ApplyCellStyleToEditingControl (style)".  Argh.
>>> 
>>> In any case, enclosed is a much simpler patch that doesn't change the
>>> public API.  Hopefully I didn't do anything else stupid -- I'm truly not
>>> trying to waste your time. ;-)
>>> 
>>> Steven Boswell
>>> 
>>> 
>>> ________________________________
>>>  From: Stifu <stifu@>
>>> To: mono-devel-list at .ximian 
>>> Sent: Sunday, June 3, 2012 1:33 AM
>>> Subject: Re: [Mono-dev] Patches for mono-winforms
>>>  
>>> This patch does not look good to me, because you're changing the public
>>> API.
>>> 
>>> In DataGridViewCell, the InitializeEditingControl method expects 2
>>> parameters rather than 3 after your patch, which means we're breaking
>>> the
>>> API and are no longer compatible with .NET.
>>> 
>>> 
>>> Steven Boswell II wrote
>>>> 
>>>> Argh...one more dumb oversight in my change.
>>>> Enclosed is ANOTHER version of the patch.
>>>> I wish I had the luxury of working on my hobbies when I was awake and
>>>> energetic. ;-)
>>>> 
>>>> Steven Boswell
>>>> 
>>>> 
>>>> ________________________________
>>>>  From: Steven Boswell II <ulatekh@>
>>>> To: "mono-devel-list at .ximian" <mono-devel-list at .ximian> 
>>>> Sent: Saturday, June 2, 2012 7:58 PM
>>>> Subject: Re: [Mono-dev] Patches for mono-winforms
>>>>  
>>>> 
>>>> Rob, you're my hero.  Very few tests in DataGridViewTest.cs create a
>>>> Form,
>>>> but most of the ones that do involve data binding.
>>>> I added a Form to my test, and it succeeded immediately.  Apparently,
>>>> Application.Run() isn't necessary, but that was a good idea.
>>>> Enclosed is a revised patch, for review by the Powers That Be.
>>>> 
>>>> Steven Boswell
>>>> 
>>>> 
>>>> ________________________________
>>>>  From: Rob Wilkens <robwilkens@>
>>>> To: mono-devel-list at .ximian 
>>>> Sent: Saturday, June 2, 2012 6:46 PM
>>>> Subject: Re: [Mono-dev] Patches for mono-winforms
>>>>  
>>>> 
>>>> On 06/02/2012 09:38 PM, Rob Wilkens wrote: 
>>>> On 06/02/2012 08:55 PM, Steven Boswell II wrote: 
>>>>>The EditingControlShowing event has to be called, and it has to be
> called
>>> after the control's contents have been initialized properly...that's not
>>> really two separate issues.
>>>>>>
>>>>>>
>>>>>>The enclosed patch is an updated version; in addition to having a unit
>>> test, it fixes one additional bug revealed by my testing. Before,
>>> DataGridViewComboBoxCell.InitializeEditingControl() was setting the
>>> initial
>>> value from the FormattedValue property, instead of
>>> the initialFormattedValue
>>> parameter.
>>>>>>
>>>>>>
>>>>>>I tried to write an additional unit test that worked with bound data,
>> but
>>> for the life of me I can't figure out why it doesn't work.  I've done
>>> data-binding with DataGridView before...it wasn't this mysterious.  In
>>> my
>>> unit test, after I set the DataGridView's DataSource property, the
>>> data-grid
>>> doesn't initialize properly; instead of four rows, it ends up with one
>>> row,
>>> and all its cell values are null.  After beating my head against the
>>> wall
>>> for several hours, I'm perfectly happy to be told what a moron I am, if
>>> someone will just tell me why the EditingControlShowingTest_Bound test
>>> doesn't work. :-)
>>>>>>
>>>>>>
>>>>>>Steven Boswell
>>>>>>
>>>>>Not writing to call you a moron, I'm a newbie myself and i could
>>>>       be wrong...  But i copied and pasted your test code for
>>>>       EditingControlShowingTest_Bound into Visual Studio 2010, but
>>>>       rather than creating the DataGridVIew in code i placed it on the
>>>>       form and modified your code to use the one on the form...  And i
>>>>       disabled the asserts...  And from what i can tell it runs fine in
>>>>       both .net and mono (that is, the data grid view populates).
>>>>>
>>>>>One thing that I noticed about your code, though, is it depends on
>>>>       a 'showing' event..
>>>>>
>>>>>And i wonder if that means your datagridview needs to be placed on
>>>>       a form which is displayed, so that it is actually shown...
>>>>>
>>>>>i.e. change your code something like this:
>>>>>
>>>>>using (Form Form1=new Form()){
>>>>>    Form1.Controls.Add(_dataGridView);//optionaly set sizeand
>>>>       location of both gridview and form
>>>>>    Form1.Show();
>>>>>
>>>>>    ... insert the rest of your code here .... 
>>>>>}
>>>>>
>>>> Oh, and if i'm right, you might need the equivalent of an
>>>> Application.Run(Form1) to process the events since you're listening for
>>>> events, just don't forget to close Form1 or the Run loop might never
>>>> end
>>>> and your test can hang up with a displayed window...
>>>> 
>>>> 
>>>> 
>>>> _______________________________________________
>>>> 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
>>>> _______________________________________________
>>>> 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/Patches-for-mono-winforms-tp4649620p4649670.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/Patches-for-mono-winforms-tp4649620p4649674.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/Patches-for-mono-winforms-tp4649620p4649683.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/Patches-for-mono-winforms-tp4649620p4649691.html
Sent from the Mono - Dev mailing list archive at Nabble.com.

More information about the Mono-devel-list mailing list