Community
Participate
Working Groups
We need to add the appropriate API to StringToNumberConverter and NumberToStringConverter for short and bytes as their boxed types are Numbers as well.
This won't be able to occur until 3.4 as we're in an API freeze.
While we're at it can we add BigDecimal support too? They're java.lang.Numbers, too.
Yeah, missed them too. Thanks for pointing it out. For BigDecimal we might also need to support the ICU4J BigDecimal (it doesn't extend from java.math.BigDecimal). I remember seeing this returned from the ICU4J DecimalFormat. Are you needing support for BigDecimal now? I'm not sure if I have the time but it could be made internal. The short and byte implementations are internal today.
We do use BigDecimals extensively in our application, so it would be great to have. Is there some way I could help?
(In reply to comment #4) > Is there some way I could help? Yes, if you have time and want to create a patch we'd be open to it and very grateful. I'd say the first step is to log a separate bug. The ability to default the validators and converters for strings and bytes is internal API. We support it, it's just not public. It shouldn't take much to add the support for BigDecimal. You can take a look at how the others have been implemented in the internal code. There's also a StringToNumberParser that handles creating the appropriate error messages for when validation fails. You'd need to create a validator and a converter and add the ability for defaulting these into UpdateStrategy and UpdateValueStrategy. There are also some pretty extensive tests in the test project to help with what the requirements are of these converters/validators.
*** Bug 196977 has been marked as a duplicate of this bug. ***
Created attachment 74319 [details] Attachment from bug 196977 by Matt Carter
I'm implementing java.math.BigDecimal support so that we have a complete patch to resolve: Bug 180392 [DataBinding] Add support for shorts, bytes, and BigDecimal to StringToNumberConverter. I want to convert the BigDecimal to String using ICU's NumberFormat class (in line with the other Number types in UpdateStrategy/NumberToStringConverter). ICU doesn't directly support java.math.BigDecimal, so for now I plan to create the corresponding com.ibm.icu.math.BigDecimal and format that (to provide proper international support). However I hit this error: Access restriction: The type BigDecimal is not accessible due to restriction on required library /home/dir/eclipse/plugins/com.ibm.icu_3.4.5.jar (non-modifiable) Access rules: 2 rule(s) defined - non modifiable Accessible: com/ibm/icu/text/* Forbidden: **/* Why is there such an access restriction on this plugin dependency? More to the point can it be removed so I can add BigDecimal support? If so how? Regards Matt
(Re. comment #8) I am planning to also implement both java.mathBigDecimal and ICU4J BigDecimal, but cannot access the ICU4J BigDecimal class due to the aforementioned access restriction /home/dir/eclipse/plugins/com.ibm.icu_3.4.5.jar (non-modifiable). I need help to get past this and use ICU4J's BigDecimal-related classes.
I've had this on my to do list but haven't had the time to look at it. I'll try to look into it tonight.
(In reply to comment #8) > I want to convert the BigDecimal to String using ICU's NumberFormat class (in > line with the other Number types in UpdateStrategy/NumberToStringConverter). > ICU doesn't directly support java.math.BigDecimal What problem did you run into? It will convert a java.math.BigDecimal. It ends up invoking doubleValue() on the BigDecimal to get the value. > However I hit this error: > > Access restriction: The type BigDecimal is not accessible due to restriction on > required library /home/dir/eclipse/plugins/com.ibm.icu_3.4.5.jar > (non-modifiable) > > Access rules: 2 rule(s) defined - non modifiable > Accessible: com/ibm/icu/text/* > Forbidden: **/* > > Why is there such an access restriction on this plugin dependency? The recommendation for depending upon ICU4J, in order to use the replacement plugin[1], is to import the packages rather than creating a direct dependency upon the ICU4J plugin. By importing the package we have the flexibility to swap out the implementation with the replacement plugin. The only packages offered by the replacement plugin are com.ibm.icu.text and com.ibm.icu.util. Since com.ibm.icu.math isn't in that list we can't add direct dependencies to the ICU4J BigDecimal implementation. This is why the access rule exists, because we're importing the packages explicitly in the manifest. > More to the point can it be removed so I can add BigDecimal support? If so how? Not if we want to be able to use the replacement plugin and we do, at least so far. In order to determine the next steps we probably need to discuss the issue you had with using the ICU4J NumberFormat. [1] http://wiki.eclipse.org/index.php/ICU4J#Replacement_Plug-in
> What problem did you run into? It will convert a java.math.BigDecimal. It > ends up invoking doubleValue() on the BigDecimal to get the value. There is code inside ICU NumberFormat.format() for Java.math.BigDecimal but it is commented out and falls through to the general Number support which calls .doubleValue(). In my opinion using a floating point number for conversion of a String to a BigDecimal or vice-versa is wrong. BigDecimal and BigInteger are arbitrary-precision numbers. They can have, by definition, a very high bit width. Parsing the String to a double will in some cases do one of: fail to parse it as a double, truncate it, or lose accuracy. Parsing as a double is a workaround which will work only for numbers in common range at best. BigDecimal and BigInteger have constructors that take a String and parse it properly for any precision. However they are in the math.* package.. > <snip> This is why the access rule exists, Now I see. Somebody said they want to support ICU BigDecimal. If math.* is outside of the replacement, I wonder whether we should create a dependency from Data Binding on the rest of ICU or not? Is this good or bad for the library? I presume people want as much support for as many types as possible... Personally I use java.math.BigDecimal, when implementing I am simply trying to use the ICU parsing and formatting features for proper ICU locale support, and trying to avoid a call to doubleValue()!
Forgot to mention.. Because I found that direct java.math.BigDecimal has been commented out of ICU4J's formatting and parsing functions, leaving only support for ICU BigDecimal, in order to support java.math.BigDecimal but still using ICU for formatting, I was going to take the java.math.BigDecimal fromValue as a String, construct an ICU BigDecimal from it, then format the ICU BigDecimal. It is at that point that I found that ICU BigDecimal was inaccessible. Regards Matt
(In reply to comment #12) > > What problem did you run into? It will convert a java.math.BigDecimal. It > > ends up invoking doubleValue() on the BigDecimal to get the value. > > > There is code inside ICU NumberFormat.format() for Java.math.BigDecimal but it > is commented out and falls through to the general Number support which calls > .doubleValue(). > > In my opinion using a floating point number for conversion of a String to a > BigDecimal or vice-versa is wrong. I didn't say that this was the right thing to do, just that it was there. I was concerned that you were receiving an error that I wasn't seeing. Now that we know what the issue is we can hopefully move forward. > I presume people want as much support for as many types as possible... You have to add context and evaluate each use case. Supporting as much as possible adds dependencies that not everyone wants or has. I think we have options to get around this limitation but I'll have to do some digging. Adding the dependency on the full ICU4J just because of BigDecimal doesn't seem like a good choice at this point in time as most won't use BigDecimal at all. ICU4J is a configuration decision that we'd like to leave up to the consumer as this effects the rest of their application and not just our little library.
Thanks Brad for your input. I agree. No ICU BigDecimal support. This does leave the problem of trying to use ICU's format/parse for java.math.BigDecimal <-> String conversion somehow other than with the doubleValue() invocation. The only solution I could think of is not viable unless we could add the ICU4J math.* dependency. I'm out of ideas on how to do this cleanly..
(In reply to comment #15) > This does leave the problem of trying to use ICU's format/parse for > java.math.BigDecimal <-> String conversion somehow other than with the > doubleValue() invocation. The only solution I could think of is not viable > unless we could add the ICU4J math.* dependency. > > I'm out of ideas on how to do this cleanly.. This is just a brain dump so it's nothing concrete but here's what I'd start with... The issue of NumberFormat not supporting java.math.BigDecimal is with the ICU4J full implementation, correct? With the replacement plugin my guess is that the issue doesn't exist as it should call thru to the JDK's NumberFormat. This would need to be verified. If true that should narrow the scope of the issue. Not to belittle anyone using BigDecimal but it's usage in a model is probably an edge case. If we determine that we have to support it one option that we have is adding an optional dependency to the ICU4J full implementation and then working some reflection magic. So we'd load the class dynamically and interact with it in a way that won't impact other consumers. Others options to consider and investigate: * Investigate into the library to determine why this support doesn't exist and what the workaround is that others employ. * Don't support it when using the ICU4J full implementation. * Perform some checking to ensure that the String can be represented as a double and fail when it can't. My guess is that for most use cases a double is sufficient.
> Not to belittle anyone using BigDecimal but it's usage in a model is probably > an edge case. I'm not so sure. We use BigDecimal to represent all financial figures and for all monetary calculations. It is used extensively here. It is the ideal type, the only other safe alternative being a scaled integer. Float and double are unsuitable for these sorts of calculation as they suffer from cumulative rounding and inaccuracy issues (especially when aggregating a long series of numeric values). The SQL NUMBER type, e.g. NUMBER(12,2) maps best to BigDecimal.
A couple of things: I would wholeheartedly disagree that the double representation of a number is "good enough" even for numbers between Double.MAX_VALUE and Double.MIN_VALUE. There are many (even small precision) numbers in that range that cannot be represented exactly. In systems that must guarantee the exact values of these numbers, converting to a double is a no-no. Also, I just noticed this code in the StringToNumberConverter that shipped with Eclipse 3.3: } else if (BigInteger.class.equals(boxedType)) { return new BigDecimal(result.getNumber().doubleValue()) .toBigInteger(); } Arguably this snippet produces an incorrect result because of the clipping that can occur due to the call to doubleValue(). At very least, this violates the principle of least surprise because of that clipping, in addition to returning a BigDecimal when a BigInteger is requested. So just that I understand, the objection to using the String constructor for BigDecimal or BigInteger is that it won't internationalize correctly?
(In reply to comment #18) > A couple of things: I would wholeheartedly disagree that the double > representation of a number is "good enough" even for numbers between > Double.MAX_VALUE and Double.MIN_VALUE. What statement led you to believe that this was being proposed? > Also, I just noticed this code in the StringToNumberConverter that shipped with > Eclipse 3.3: > > } else if (BigInteger.class.equals(boxedType)) { > return new BigDecimal(result.getNumber().doubleValue()) > .toBigInteger(); > } > > Arguably this snippet produces an incorrect result because of the clipping that > can occur due to the call to doubleValue(). At very least, this violates the > principle of least surprise because of that clipping, in addition to returning > a BigDecimal when a BigInteger is requested. Please log a separate bug about invoking doubleValue(). There is a call to toBigInteger() at the end of that line so it's returning the correct type. > So just that I understand, the objection to using the String constructor for > BigDecimal or BigInteger is that it won't internationalize correctly? There hasn't been an objection to this. We're trying to figure out how to interact with an ICU4J BigDecimal when we don't want to depend upon the full ICU4J.
(In reply to comment #19) > > A couple of things: I would wholeheartedly disagree that the double > > representation of a number is "good enough" even for numbers between > > Double.MAX_VALUE and Double.MIN_VALUE. > > What statement led you to believe that this was being proposed? > Sorry, I had meant to refer to any intermediate representation as a double for arbitrary-precision numbers, including calls to Number.doubleValue() regardless of what is done with the result. I thought that that was one of the proposed "investigation" topics in comment #16. Apologies if I misunderstood.
No need to apologize. I wasn't clear. That option was in regards to if we couldn't figure out an acceptable way to get around this issue AND if the value could be represented as a double without losing precision. I agree that a solution that loses precision is not acceptable.
Note to self: NumberToBigIntegerConverter: Fix to convert via a string instead of a double. NumberToBigDecimalConverter: Fix to convert via a string instead of double.
I have investigated the ICU code again to look for any workaround for using ICU to format and parse java.math.BigDecimal. In ICU NumberFormat (class DecimalFormat), virtually every type (e.g. long, BigInteger) is turned internally into an ICU BigDecimal and it is this that is formatted. They now support only one flavour of BigDecimal, the ICU one. This means in effect that we cannot format java.math.BigDecimal using ICU unless we include ICU's BigDecimal class or the math.* package in the allowed access restrictions. Therefore to do this right we really have to try and get Eclipse's ICU scope and Replacement Plugin extended to include ICU BigDecimal. The sane alternatives are: 1) Have internationalization for BigDecimal and BigInteger but convert via a double, losing both precision and range (which we agree are both unacceptable). 2) Have no internationalization for now, and support arbitrary precision correctly on both BigDecimal and BigInteger. I suggest we do the following: - Start off the process to extend the Replacement Plugin - For now, switch from 1) to 2) until ICU BigDecimal is available to use for proper internationalized conversion of java.math.BigDecimal. This is, after all, internal code, and not many will notice the change anyway. Votes?
+1
(In reply to comment #23) > I have investigated the ICU code again to look for any workaround for using ICU > to format and parse java.math.BigDecimal. > > In ICU NumberFormat (class DecimalFormat), virtually every type (e.g. long, > BigInteger) is turned internally into an ICU BigDecimal and it is this that is > formatted. They now support only one flavour of BigDecimal, the ICU one. > > This means in effect that we cannot format java.math.BigDecimal using ICU > unless we include ICU's BigDecimal class or the math.* package in the allowed > access restrictions. > > Therefore to do this right we really have to try and get Eclipse's ICU scope > and Replacement Plugin extended to include ICU BigDecimal. We don't have to have this. Feel free to request it but we can do this via reflection and by declaring an optional dependency upon the full ICU4J. We can lookup the class and if it does not exist we assume that the full ICU4J isn't included. We can then fall back on Number.doubleValue() as this is what the JDK does today in NumberFormat, at least in JDK 1.5.0_11. > The sane alternatives are: > 1) Have internationalization for BigDecimal and BigInteger but convert via a > double, losing both precision and range (which we agree are both unacceptable). I'm changing my position that I feel that this is unacceptable to that this is not ideal. Whether the consumer knows it or not it happens in the JDK today in NumberFormat. If the consumer includes the full ICU4J then we should behave like ICU4J. But if they use the replacement plug-in we'll be behaving like the rest of the platform. I haven't checked JDK 6 or 7 to see what they do there. > 2) Have no internationalization for now, and support arbitrary precision > correctly on both BigDecimal and BigInteger. I'm confused. If we have no internationalization how do we support arbitrary precision correctly? > I suggest we do the following: > - Start off the process to extend the Replacement Plugin > - For now, switch from 1) to 2) until ICU BigDecimal is available to use for > proper internationalized conversion of java.math.BigDecimal. > > This is, after all, internal code, and not many will notice the change anyway. > > Votes? +1/2 -1/2. We have a way around this today via reflection even though it's not as clean as we would like. Go ahead and request it but we don't know that this will be possible at this point. I'm not a fan of waiting around especially when we have a solution even though it's a little less sane. By having a solution today we can get it into the hands of early adopters sooner to assert that our logic is sound and to work out any bugs. We could then swap out our workaround if the replacement plug-in is updated in the future. Basically I'd rather do it today, find it is not going to do what we thought and pull it rather than pushing it in towards the end and possibly getting locked into it at release time without enough time to assert its correctness.
Also I didn't see in comment 23 how you were planning to convert from JDK BigDecimal to ICU4J BigDecimal. Are you planning on using Strings as was previously mentioned? I'd feel better about this if we asked the ICU4J community how to do this.
I think prior to 1.5, java.text.DecimalFormat was not able to format BigDecimals without losing precision. It looks like this was fixed for 1.5. Is the problem that the ICU implementation of NumberFormat.parse does not return a java.math.BigDecimal?
(In reply to comment #27) > I think prior to 1.5, java.text.DecimalFormat was not able to format > BigDecimals without losing precision. It looks like this was fixed for 1.5. You're right. I was looking in NumberFormat and not DecimalFormat. DecimalFormat handles it.
(In reply to comment #27) > Is the problem that the ICU implementation of NumberFormat.parse does not > return a java.math.BigDecimal? The issue, correct if I'm wrong Matt, is that the ICU4J NumberFormat/DecimalFormat doesn't support java.math.BigDecimal for formatting or parsing.
(In reply to comment #25) > > The sane alternatives are: > > 1) Have internationalization for BigDecimal and BigInteger but convert via a > > double, losing both precision and range (which we agree are both unacceptable). > > I'm changing my position that I feel that this is unacceptable to that this is > not ideal. Whether the consumer knows it or not it happens in the JDK today in > NumberFormat. I still maintain that converting BigDecimal values, or any arbitrary-precision number into a double and back is unacceptable. It is a clear bug in data binding's BigDecimal support and must be resolved; regardless of what else we do in addition to this to achieve/retain ICU internationalization. Currently data binding will fail when handling perfectly valid BigDecimal values. Conversion via doubleValue() happens in NumberFormat as only a 'Best Effort' conversion of an unrecognised implementation of "Number". The Number interface supports a series of methods to convert the 'Number' whatever kind it is, into a few primitive types. Double is the widest of these available methods, so this is what NumberFormat uses for best effort. For known types, there is specialization. There has to be specialization for BigDecimal because it simply does not fit into doubleValue() (unlike most other Number types with fixed precision, most of which do fit, so this is not an issue there). > If the consumer includes the full ICU4J then we should behave > like ICU4J. But if they use the replacement plug-in we'll be behaving like the rest of the platform. I like your idea of checking for full ICU4J presence and using it if we can. +1 We know using ICU BigDecimal during the conversion process, if it is available, is the right solution, as we have discussed this. This leaves just what we do when full ICU4J is *not* present. Back to my 1) or 2) post, Comment #23, for this. > > 2) Have no internationalization for now, and support arbitrary precision > > correctly on both BigDecimal and BigInteger. > > I'm confused. If we have no internationalization how do we support arbitrary > precision correctly? There is a standard java.math.BigDecimal constructor that takes a String and parses the String value. We are simply trying here to use ICU for internationalization instead. > I'm not a fan of waiting around especially when we have a solution even though it's a little less sane. By having a solution today we can get it into the hands of early adopters sooner to assert that our logic is sound and to work out any bugs. We could then swap out our workaround if the replacement plug-in is updated in the future. Basically I'd rather do it today, find it is not going to do what we thought and pull it rather than pushing it in towards the end and possibly getting locked into it at release time without enough time to assert its correctness. Me too. Our fallback implementation must not use doubleValue(), though. I will summarize in the next post.
(In reply to comment #26) > Also I didn't see in comment 23 how you were planning to convert from JDK > BigDecimal to ICU4J BigDecimal. Are you planning on using Strings as was > previously mentioned? I'd feel better about this if we asked the ICU4J > community how to do this. We could use Strings yes. I was planning to use the more direct bigInteger and scale() properties from one BigDecimal type to construct the other. This is the same method used in both the JDK and ICU internals. We can then format/parse the ICU BigDecimal and thus the java.math.BigDecimal using ICU. Feel free to ask the community but I've copied a solution they use internally.
OK, I will summarize the proposed solution which after vote can be implemented immediately. 1. Using reflection we check for the presence of icu.math.BigDecimal (i.e. the full ICU4J). If this class is present, we use it, and do a perfect conversion with proper ICU internationalization. 2. We start off the process to make the above solution the only one by requesting that Eclipse's ICU scope & Replacement Plugin is extended to include ICU BigDecimal. 3. When full ICU4J is not available in the application, we have to make a choice. We have the same choice as in Comment #23: 1) Have internationalization for BigDecimal and BigInteger but convert via a double, losing both precision and range. or 2) Have no internationalization for now, and support arbitrary precision correctly on both BigDecimal and BigInteger. I see 1 as a serious bug, 2 as an unwelcome but unavoidable inability to support ICU internationalization for BigDecimal, *yet*. As such I vote: Matt +1 I presume Dave's vote for 2) still stands, and I assume he has no objection to Brad's idea to check to see if ICU4J is available at runtime and use it if present. (Please correct me if I'm wrong Dave). Dave +1 Brad can you cope with losing doubleValue() and ICU parsing of BigDecimal temporarily? Any further votes?
(In reply to comment #31) > (In reply to comment #26) > > Also I didn't see in comment 23 how you were planning to convert from JDK > > BigDecimal to ICU4J BigDecimal. Are you planning on using Strings as was > > previously mentioned? I'd feel better about this if we asked the ICU4J > > community how to do this. > > We could use Strings yes. I was planning to use the more direct bigInteger and > scale() properties from one BigDecimal type to construct the other. This is the > same method used in both the JDK and ICU internals. > > We can then format/parse the ICU BigDecimal and thus the java.math.BigDecimal > using ICU. > > Feel free to ask the community but I've copied a solution they use internally. There's a constructor on com.ibm.icu.math.BigDecimal that accepts a java.math.BigDecimal[1]. It's not included in the version of ICU4J distributed by Eclipse. It seems to be removed when built. I'm not sure why Eclipse decided to not included this but it states that all it does is invoke the BigDecimal(String)[2] constructor. I'd prefer to use the available API as long as it promises to accomplish our goals. Also this would only go one way java.math.BigDecimal -> com.ibm.icu.math.BigDecimal. We'd need to have a routine to go the other way (simple googlin' didn't reveal anything for this but I didn't spend much time on it). [1] http://www.icu-project.org/apiref/icu4j/com/ibm/icu/math/BigDecimal.html#BigDecimal(java.math.BigDecimal) [2] http://www.icu-project.org/apiref/icu4j/com/ibm/icu/math/BigDecimal.html#BigDecimal(java.lang.String)
(In reply to comment #32) > 3. When full ICU4J is not available in the application, we have to make a > choice. We have the same choice as in Comment #23: I'm obviously missing a part of the problem. I thought the problem was what to do when using the ICU4J NumberFormatter because it didn't support java.math.BigDecimal directly. If using the replacement plug-in all we need to do is pass it a java.math.BigDecimal as the platform will include support for it. What am I missing?
> There's a constructor on com.ibm.icu.math.BigDecimal that accepts a > java.math.BigDecimal[1]. It's not included in the version of ICU4J distributed > by Eclipse. It seems to be removed when built. I'm not sure why Eclipse > decided to not included this .. This requires us to be able to access com.ibm.icu.math.*. If we could access this class from data binding (with reference to the access restrictions mentioned earlier), then we could use this String constructor, or construct an ICU BigDecimal from the unscaledValue and scale, there are a few options. > Also this would only go one way java.math.BigDecimal -> > com.ibm.icu.math.BigDecimal. We'd need to have a routine to go > the other way (simple googlin' didn't reveal anything for this > but I didn't spend much time on it). Easy this :) To convert the other way we can use this constructor on java.math.BigDecimal: public BigDecimal(BigInteger unscaledVal, int scale) Like this: out = new java.math.BigDecimal(in.unscaledValue(), in.scale());
(In reply to comment #35) > > There's a constructor on com.ibm.icu.math.BigDecimal that accepts a > > java.math.BigDecimal[1]. It's not included in the version of ICU4J distributed > > by Eclipse. It seems to be removed when built. I'm not sure why Eclipse > > decided to not included this .. > > This requires us to be able to access com.ibm.icu.math.*. > If we could access this class from data binding (with reference to the access > restrictions mentioned earlier), then we could use this String constructor, or > construct an ICU BigDecimal from the unscaledValue and scale, there are a few > options. Nothing changes here from the previous plans. We just use reflection which is what we'd already have to do.
(In reply to comment #34) > (In reply to comment #32) > > 3. When full ICU4J is not available in the application, we have to make a > > choice. We have the same choice as in Comment #23: > > I'm obviously missing a part of the problem. I thought the problem was what to > do when using the ICU4J NumberFormatter because it didn't support > java.math.BigDecimal directly. If using the replacement plug-in all we need to > do is pass it a java.math.BigDecimal as the platform will include support for > it. What am I missing? > I assumed that the replacement plugin for ICU would behave in the same way as ICU. You are telling me that the replacement supports proper conversion of java.math.BigDecimal, when ICU [the one shipped with eclipse] does not. If this is the case, problem solved as you say.
(In reply to comment #37) > (In reply to comment #34) > > (In reply to comment #32) > > > 3. When full ICU4J is not available in the application, we have to make a > > > choice. We have the same choice as in Comment #23: > > > > I'm obviously missing a part of the problem. I thought the problem was what to > > do when using the ICU4J NumberFormatter because it didn't support > > java.math.BigDecimal directly. If using the replacement plug-in all we need to > > do is pass it a java.math.BigDecimal as the platform will include support for > > it. What am I missing? > > > > I assumed that the replacement plugin for ICU would behave in the same way as > ICU. You are telling me that the replacement supports proper conversion of > java.math.BigDecimal, when ICU [the one shipped with eclipse] does not. > > If this is the case, problem solved as you say. > The replacement plugin calls thru to the JDK which speaks in terms of java.math.BigDecimal and not com.ibm.icu.math.BigDecimal. Thus no conversion, or extra logic, should be necessary.
Created attachment 75146 [details] Fix to all issued discussed. Patch to prevent doubleValue() conversion of BigDecimal and BigInteger, as discussed above, as well as adding support for shorts and bytes. Replaces previous patch for short and byte support only.
Created attachment 75148 [details] Patch to prevent doubleValue() conversion of BigDecimal and BigInteger, and to add support for shorts and bytes. (This version removes an unused field from the previous patch).
Created attachment 75162 [details] Patch to prevent doubleValue() conversion of BigDecimal and BigInteger, and to add support for shorts and bytes. (This version comments out a debug statement left in the previous patch).
I will need some time to digest all of this. Feel free to ping me on this bug if you don't see any progress.
Created attachment 75340 [details] Patch to prevent doubleValue() conversion of BigDecimal and BigInteger, and to add support for shorts and bytes. (This version includes UpdateStrategy.java which I omitted from the previous patch)
The patch looks good overall, but there are four issues: 1. In UpdateStrategy.java. you added two lines like this: ((DecimalFormat) integerFormat).setGroupingUsed(false); I assume you did this for a reason, in which case I would like to see a comment explaining the reason. Also, I would like to see tests for this (see 4. below). 2. In NumberToStringConverter, "fromTypeIsLong" should probably be renamed to "fromTypeFitsLong". 3. In StringToNumberConverter, you throw an IllegalArgumentException under certain circumstances. Shouldn't this be thrown only if the double cannot be represented by an integral value, i.e. new BigDecimal(n.doubleValue()).scale > 0? 4. We need tests for this. Adding methods to NumberToStringConverterTest and StringToNumberConverterTest would be a good start. To write tests, think about what could go wrong in your implementation and try to cover all the cases. Also, have a look at http://jester.sourceforge.net/. I am *not* suggesting that you actually use the tool. I just like the idea that if someone were to change your code along the lines of what Jester does, the tests should detect that. At least for me, this helps to get into the mood for testing.
Created attachment 76634 [details] Patch to prevent doubleValue() conversion of BigDecimal and BigInteger, and to add support for shorts and bytes - Revised patch.
(In reply to comment #44 / Boris) > 1. In UpdateStrategy.java. you added two lines like this: > ((DecimalFormat) integerFormat).setGroupingUsed(false); Leaked in from another patch. Fixed. > 2. In NumberToStringConverter, "fromTypeIsLong" should probably be renamed to > "fromTypeFitsLong". Done. > 3. In StringToNumberConverter, you throw an IllegalArgumentException under > certain > circumstances. Shouldn't this be thrown only if the double cannot be > represented > by an integral value, i.e. new BigDecimal(n.doubleValue()).scale > 0? Agreed. Done. > 4. We need tests for this. Adding methods to NumberToStringConverterTest and > StringToNumberConverterTest would be a good start. Test patch coming soon.
Moving to M3.
(In reply to comment #46) > Test patch coming soon. > Matt, did you ever get these tests written?
No. I've been too busy to do this, I got pulled off onto a non-UI project and the rest of my life is too busy. It's still flagged in my Inbox.
OK. I've gone ahead and written the tests for this patch at last, and fixed remaining conversion problems handling BigDecimals which the tests helped to find, thus a new patch too. To generate the 'expected' strings for comparison (particularly for the very long decimals) I've had to introduce a dependency via reflection on ICU4J com.ibm.icu.math.BigDecimal (which is currently outside of the access restrictions) from the unit tests. There is an alternative to remove this dependency; quoting the formatted big numbers as constant strings, but this would not follow suit with the other tests, which generate their 'expected' strings from the input using IntegerFormat or NumberFormat. If the ICU BigDecimal dependency is undesirable in the tests this can be quickly changed. With Java 1.5 DecimalFormat, and with ICU4J present, the NumberFormatter should properly parse and return a BigInteger or BigDecimal so there will be no Double involved. In the absence of the ICU4J library or Java 1.5, the BigDecimal converter has been changed to only accept non-integral Double values, if the active NumberFormatter doesn't properly support BigDecimal and decides to convert the Number to a Double. This avoids any rounding, loss of precision or truncation occurring in the internals of a BigDecimal bounding and as such hopefully the issues discussed on this bug. We still need to approach the ICU Replacement Plugin team (or whoever is appropriate) about removing the access restriction on com.ibm.icu.math.BigDecimal and supporting it in the Replacement plugin, so that we can remove the reflection code. Matt
Created attachment 80877 [details] Revised patch to prevent doubleValue() conversion of BigDecimal and BigInteger, and to add support for shorts and bytes. Revised patch to prevent doubleValue() conversion of BigDecimal and BigInteger, and to add support for shorts and bytes.
Created attachment 80878 [details] Unit Tests
bounding -> binding :)
Thanks for the contribution. Sorry for not being able to put this into M3. Marking it for M4.
I am awfully sorry, but I only saw this bug today when I looked if I had older bugs with patches that were not yet applied. Unfortunately, the contribution needs to be IP reviewed, and there is no way we can get this into 3.4 because of the API freeze that will be in effect starting today. Is there a useful part of this that does not affect the API which we could release in 3.4 M7? If yes, I would start the IP review process now.
Ping [DataBinding] Add support for shorts, bytes, and BigDecimal to StringToNumberConverter Can we get this patch in yet? I've been running this patch for over a year now. BigDecimal<->String doesn't work properly until this patch is applied (except for the default toString conversion which is read-only). Regards Matt Carter
Created attachment 109069 [details] Patch updated for CVS HEAD Updated patch.
Thanks for the ping. I have tried to enter a CQ but hit a snag, see bug 243233.
I have been able to file a CQ successfully now. Matt, just to double-check: Are you still with Philips, so that we can use the employer consent form that you sent in for bug 197679?
Hi Boris, Yes the form is still valid.
We now have approval from the Eclipse IP team to check in the changes (but we need to ensure the default Eclipse copyright and license is attached to the patches prior to check in).
Released to HEAD. (Finally!) I had to insert the following in the manifest file for core.databinding: Import-Package: com.ibm.icu.math;resolution:=optional, and this in the manifest file for the test plugin: Import-Package: com.ibm.icu.math, Matt, it would be great if you could verify this, using one of the next I builds, or using the M3 build when it is available. Thanks for the patch, and sorry that it took us so long!
Boris, I'm getting two compiler errors in StringToNumberConverter.java (lines 203 & 205) after updating the changes introduced by this patch; it seems as if IllegalArgumentException has no constructor taking a Throwable as parameter before Java 1.5.
(In reply to comment #63) > Boris, I'm getting two compiler errors in StringToNumberConverter.java Good catch! I'm installing the proper JREs into my Eclipse now to make sure I didn't break anything else.
Released a fix for the compile errors to HEAD.
Verified by code inspection using I20081028-0100, and by checking the test results for I20081027-1300.
Can you tell me why this works: IObservableValue transactionValueObserveValue = BeansObservables.observeValue(transaction, "value"); bindingContext.bindValue(textTextObserveWidget, transactionValueObserveValue, null, null); with value beeing a BigDecimal and when I use a ComputedObservable IObservableValue balanceComputed = new ComputedValue(){ protected Object calculate() { BigDecimal balance = new BigDecimal("0.00"); for( Object o: accountTransactionsObserveSet){ Transaction t = (Transaction)o; balance = balance.add(t.getValue()); } return balance; } }; the value is not converted to String when bound to a text Field. Any hints would be appreciated !
(In reply to comment #67) Can you try: new ComputedValue(BigDecimal.class) { ...
Comment on attachment 109069 [details] Patch updated for CVS HEAD added missing iplog+ flag.
