Community
Participate
Working Groups
DataBinding is missing support for the Character type throughout. As illustrated by this method in UpdateStrategy: private static Class autoboxed(Class clazz) { if (clazz == Float.TYPE) return Float.class; else if (clazz == Double.TYPE) return Double.class; else if (clazz == Short.TYPE) return Short.class; else if (clazz == Integer.TYPE) return Integer.class; else if (clazz == Long.TYPE) return Long.class; else if (clazz == Byte.TYPE) return Byte.class; else if (clazz == Boolean.TYPE) return Boolean.class; else if (clazz == Character.TYPE) // <- Missing return char.class; // <- Missing return clazz; } I have had to add these two lines whilst working with char and Character type properties. Unless I am simply mistaken, char and Character are a pair of autobox types too.. If nobody has any objections I'd like to finish off the currently partial support for the types char and Character in DataBinding. (I can do this as part of a complete patch for Bug 180392 - [DataBinding] Add support for shorts, bytes, and BigDecimal to StringToNumberConverter.)
(In reply to comment #0) > If nobody has any objections I'd like to finish off the currently partial > support for the types char and Character in DataBinding. It will be hard to find a situation that we object to a patch. ;)
Is it possible for me to get commit rights to DataBinding? (I have no idea how this is issued and to whom). My org.eclipse.*.databinding.* packages are way out of sync with HEAD with patches and fixes. I have lots of private 'contrib' code outside of databinding that should be in it. I have a team using DataBinding who are having to update from HEAD then apply patches. Regards Matt
(In reply to comment #2) > Is it possible for me to get commit rights to DataBinding? (I have no idea how > this is issued and to whom). You'd have to be elected as a committer. When I went thru the process I had to contribute to the project for at least 3 months (this is a standard Platform UI practice). Once a "body of work" existed the members of the team voted on granting me committer status. I don't think anything has changed in regards to these rules. We can probably provide more details if you're interested. > My org.eclipse.*.databinding.* packages are way out of sync with HEAD with > patches and fixes. I have lots of private 'contrib' code outside of databinding > that should be in it. I have a team using DataBinding who are having to update > from HEAD then apply patches. I admit that I haven't spent much time recently applying patches. I've been working on bug 182059 when I can find the time. Once I can get an initial version out there, and if Boris doesn't beat me to it, I plan on going through the patches and applying what I can. Until then if I was in your position I'd probably take matters into my own hands. A couple of options that I have employed in the past have been... 1) Take the code, apply your patches, and build your own endstates. As long as you follow the rules of the EPL this is possible. 2) When possible provide implementations to your team that contain your fixes. For example this patch could be an extension of UpdateStrategy that incorporates the fix. My experience has been that when dealing with open source, and really software in general, you have to sometimes get creative when it comes to bug fixes and enhancements. Not everyone can consume HEAD. For those case there will be a period of time that you have to workaround the issues. But hopefully we'll get to your patches soon. I'm not trying to make excuses here, just trying to help you get by until the patches can be applied.
Thanks for the insight Brad.
Created attachment 74569 [details] This tiny patch adds char<=>Character boxing to the autoboxed() method of UpdateStrategy. I have needed this patch when working with char and Character types in the same fashion as with the other primitive types. Of course there is more Character/char-related work to do after this. :)
(In reply to comment #5) > Created an attachment (id=74569) [details] > This tiny patch adds char<=>Character boxing to the autoboxed() method of > UpdateStrategy. > > > I have needed this patch when working with char and Character types in the same > fashion as with the other primitive types. > > Of course there is more Character/char-related work to do after this. :) Are you wanting me to evaluate this patch now? (In reply to comment #0) > If nobody has any objections I'd like to finish off the currently partial > support for the types char and Character in DataBinding. (I can do this as part > of a complete patch for Bug 180392 - [DataBinding] Add support for shorts, > bytes, and BigDecimal to StringToNumberConverter.) > If possible please keep these enhancements separate since they're separate bugs. It will make it easier to apply the patches and for tracking purposes.
> Are you wanting me to evaluate this patch now? Yes you could evaluate it now. It's quite harmless, and part of the work to close this bug. > If possible please keep these enhancements separate since they're separate > bugs. It will make it easier to apply the patches and for tracking purposes. OK.
I can't apply the patch in attachment 74569 [details] as it would introduce one bug and one inconsistency. It says the autoboxed type is char.class and it should be Character.class. It appears to still work with char.class because the default converter will just pass the value thru. But it should be using IdentityConverter which has some checks to ensure that if converting to a primitive null is not allowed. The mentioned inconsistency is that IdentityConverter will need to be updated as well. If it's not updated it will skip a few checks that it performs. To avoid these sort of issues we're pretty heavy on testing. You should be able to write a couple simple tests to assert the expected behavior. One in UpdateStrategyTest to assert that it uses IdentityConverter and then add two asserts to IdentityConverterTest.testIsPrimitiveTypeMatchedWithBoxed().
OK. Can we apply the autobox char to Character patch if we also add support to IdentityConverter? I should have spotted this, I do however have patches on top of patches on Data binding, it is difficult sometimes to isolate changes and create a single bug patch against HEAD. I'd write tests, assuming the testing framework has been improved, but I don't know where the test code is found these days. Maybe you can point me in the write direction.
(In reply to comment #9) > OK. Can we apply the autobox char to Character patch if we also add support to > IdentityConverter? If it works correctly I will apply the patch. > I should have spotted this, I do however have patches on top of patches on Data > binding, it is difficult sometimes to isolate changes and create a single bug > patch against HEAD. My advice is you find a system to solve that issue. One way is to create a new workspace with the code from HEAD and make your changes there and make the patch off it. Everyone makes mistakes but in the end we're ultimately responsible for our commits. > I'd write tests, assuming the testing framework has been improved, I'm not sure what you're referring to. Regardless if you had an issue it would be best to ask rather than ignore it. > but I don't > know where the test code is found these days. Maybe you can point me in the > write direction. This should get you started. http://wiki.eclipse.org/JFace_Data_Binding/FAQ#How_do_I_run_the_tests.3F
Matt, where do we stand with this? Patch 74569 doesn't get us what we need.
(In reply to comment #11) > Matt, where do we stand with this? Patch 74569 doesn't get us what we need. > My code here is more complete. I will isolate it and attach a replacement patch for Character<->char autoboxing support that includes IdentityConverter (correcting the bug and inconsistency Brad identified).
Created attachment 76199 [details] char<=>Character autobox support.
Thanks Matt. Can you also provide tests? I can provide some guidance on what and where. There should be quite a few similar tests already out there.
(In reply to comment #14) > Thanks Matt. Can you also provide tests? I can provide some guidance on what > and where. There should be quite a few similar tests already out there. > I need the guidance! Unsure of what to assert, and what test data is sufficient (my use case is complex and not directly applicable to a test). If there is a similar test that would help, I can clone and modify, but still somewhat unsure as to how to proceed. :(
For changes to IdentityConverter see IdentityConverterTest.testIsPrimitiveTypeMatchedWithBoxed(). For the UpdateStrategy changes see UpdateStrategyTests. There are all sorts of tests for asserting the type of the converter dependent upon the types in use.
What I am looking for is a separate patch to the test project that fails if run without the code patch, but succeeds after applying the code patch. It doesn't have to be fancy. If you are still unsure, I can write the tests for you to learn what we are looking for.
Created attachment 76628 [details] Test for char<=>Character autobox (modifies IdentityConverterTest)
(In reply to comment #17) > What I am looking for is a separate patch to the test project that fails if run > without the code patch, but succeeds after applying the code patch. It doesn't > have to be fancy. If you are still unsure, I can write the tests for you to > learn what we are looking for. My test case that fails without the patch but works with it, involves DescriptionToValueConverter for a (boxed) model observable of type char.class. DescriptionToValueConverter is not yet in the API. I can't think of another test to replace this one. Need help here.
Take a look at UpdateStrategyTest. You should be able to assert the type of converter being returned for char <=> Character conversions using assertDefaultConverter(...). Today it returns DefaultConverter but should be IdentityConverter after the patch.
Created attachment 81045 [details] JFace Data Binding Character support completed.
Created attachment 81046 [details] JFace Data Binding Character support tests.
Brad, I've attached patches to complete Character/char support for evaluation, including tests.
Great. Thanks Matt. I'm going to be out of town the next couple of days without my laptop. I'll try to get to this next week.
Matt, can you format the UpdateStrategy changes with the "Eclipse [built-in]" formatter? UpdateStrategy was poorly formatted to begin with so I've checked in the existing UpdateStrategy fully formatted. Your patch had a significant number of format changes and it was going to take too long to diff the files. If you can reformat your patch it will make it a lot easier on me to see what changed and to get this applied.
(In reply to comment #25) > Matt, can you format the UpdateStrategy changes with the "Eclipse [built-in]" > formatter? Make that "can you format all changes".
Created attachment 82080 [details] JFace Data Binding Character support completed. Formatted.
Created attachment 82081 [details] JFace Data Binding Character support tests. Formatted.
Wb Brad. Formatted patches applied. Still not great for Diffs though. Ironically I decided to remove line-breaks in UpdateStrategy.java by hand to make it more readable, so I could check that all the correct types were in there. Ever since it was formatted it has become imho difficult to view. Still, formatting is A Good Thing(TM).
Brad, do you know about the 'Ignore white space' option in the compare editor?
(In reply to comment #30) > Brad, do you know about the 'Ignore white space' option in the compare editor? > Yeah, that helped a bit but it was still pretty difficult if memory serves me right. (In reply to comment #29) > Ironically I decided to remove line-breaks in UpdateStrategy.java by hand to > make it more readable, so I could check that all the correct types were in > there. Ever since it was formatted it has become imho difficult to view. Still, > formatting is A Good Thing(TM). I agree it's hard to view. But we only need to format with this formatter when diffing. You can format it how ever you want when viewing and writing.
Created attachment 82150 [details] Rej file after trying to apply the latest patch Matt, here's what I get when I try to apply the patch to UpdateStrategy. Is this patch made against today's HEAD? The only changes that get applied are a couple of imports and the addition to the change to the autoboxed method.
(In reply to comment #32) > this patch made against today's HEAD? I thought so.. I've done a cvs update, the only thing that has changed in CVS is UpdateStrategy. You appear to have checked a reformatted version in, so now there is a merge conflict with UpdateStrategy.java. I've regenerated the patches in case this makes a difference, and integrated some of your extra line breaks, but I've had to do 'Mark as Merged' for some of the changes. The only changes that get applied are a > couple of imports and the addition to the change to the autoboxed method. There should be changes and additions to the pair map put() list too. Try again pls? :)
Created attachment 82173 [details] JFace Data Binding Character support completed. Regenerated.
Created attachment 82174 [details] JFace Data Binding Character support tests. Regenerated.
The last one applies fine. Thanks for your patience. I'll look this over tonight and hopefully commit it then. Sorry for all the run around on this one. :) Apparently I have a little to learn in regards to applying patches.
(In reply to comment #36) > The last one applies fine. Great. :) > Apparently I have a little to learn in regards to applying patches. Well, I had never written a JUnit test until last week :D You were very helpful on that front, thanks.
FIXED > 20071106. Patches from attachment 82173 [details] and attachment 82174 [details] applied. Thanks! The only change I had to make was to add the new test classes to org.eclipse.jface.tests.databinding.BindingTestSuite so that they're ran in the automated tests.
This may need be IP reviewed because the actual code patch and the tests, when taken together, are more than 250 lines of code.
Added the IPZilla link
This contribution is over 250 lines. > Could you please make a statement on the bug confirming that you authored > the code and that you have the right to donate the code to Eclipse under > the EPL for inclusion in future Eclipse releases? IP Statement for bug #197679 (and other contributions) by Matt Carter as requested by Boris Bokowski for acceptance of contributed changes to Data Binding. I authored all the code I have contributed. I hereby donate the code to Eclipse (but of course retaining my implicit copyright) under the latest version of the Eclipse Public License at the time of writing (2007-12-15) for inclusion in future Eclipse releases. I am UK-based. Rights of ownership/right to donate is as follows, as per all my contracts where it is explicitly written. this is also the default implied UK law: - Work done at my clients and employers belongs to them by default. - Work done in my own time belongs to me. In the event that any part of the work have been produced in I make the following statement. I am currently working on a project for a Company (Philips Collection Services Ltd.) by whom I am currently employed and where the above distinction between work ownership is explicitly stated in my contract (for the avoidance of doubt) (and to my knowledge is the default under UK law if left unstated). (I had this clause put in some time before I or anyone else in the Company made any contribution other than bug posts to bugs.eclipse.org). Let 'Work' mean work chosen by the copyright owners herein for contribution. Work done in my own time, I choose to donate under the terms of the EPL. Any Work I choose to contribute which may be owned by the Company is contributed under the following terms: I, as an agent of the Company for which I work and in the capacity of the current Head of IT, permit IT developers employed by the company to license and consume open source code and technology for it's software projects where the license to this code is compatible with the Company's policies and requirements. As a consumer of said open source code and technology, including but not limited to code licensed from Eclipse contributors under the Eclipse Public License, the company may choose to reciprocally contribute work back to said open source projects in the spirit of open source collaboration under the condition that said code is not in conflict with the company's IT and software development policies and in particular that the contribution released under the EPL does not grant a competitive advantage to any direct competitor of the company. None of the work I have committed so far on bugs.eclipse.org is in conflict with the Company's IT and software development policies, nor does it grant any competitive advantage to a direct competitor. (The Company has access to and already utilizes, or can utilize, said work to it's own ends thus any is competitive advantage given as a direct result of this contribution is equaled and thus negated.)
(Amendment to comment #41): Replace: > In the event that any part of the work have been produced in I make the > following statement. with: > For in the event that any part of the work that I have produced > was written in 'work time', I make the following statement.
Hi Matt: Thank you for your response. For clarify purposes, could you please have your employer(Philips Collection Services Ltd.) sign an Eclipse Employer Consent Form on your behalf. Here is a link to the form - http://www.eclipse.org/legal/committer_process/employer_consent.pdf When completed, the form may be faxed to my attention to 613-224-5172. Regards, Sharon Corbett Eclipse Foundation Inc. Tel: 613-224-9461 ext. 232
Hi Sharon, Nobody above me (Head of IT) has any idea what 'open source' or 'Eclipse' are but I'll give it a go! Regards Matt
Thanks Matt. ECF was received by Fax here at the Foundation today. I'll continue with the review of the relevant IPzilla CQ today. Regards, Sharon
The contribution has been approved in IPZilla.
