Bug 316878

Comment 1 Ivan Furnadjiev 2011-02-14 03:31:29 EST

As we have a custom (non native) scrollbars now, we can put a general saveguard in the scrollbar itself - to always fire events with int values. The other place, where we got such a case, is the mouse wheel events.

Comment 2 Ivan Furnadjiev 2011-02-17 05:50:03 EST

Introduced new NumberFormatUtil.parseInt method witch replaces Integer.parseInt in all LCA classes. It checks for number to be in the range ( Integer.MIN_VALUE, Integer.MAX_VALUE ) and throws IllegalArgumentException. The decimal numbers are rounded first and casted to int afterwards. NumberFormatException is thrown in case of invalid string "number". Fixed in CVS HEAD.

Comment 3 Rüdiger Herrmann 2011-02-17 07:20:35 EST

(Sorry for being late with this objection, I was offline the past days)
In my opinion, the server-side is the wrong place to cope with floating-point coordinates.
From a server-side point of view, there is no such thing as floating-point coordinates. All the APIs dealing with coordinates return and accept only integer values. Given that, it is in the clients responsibility to ensure that floating-point coordinates are truncated before sending the to the server. The server-side should in deed complain if it receives non-integer values.
Therefore I suggest to 'move' the code to the client side.

Comment 4 Ivan Furnadjiev 2011-02-17 07:46:31 EST

We decided to do it on the server-side with the new "protocol" in mind. Because I'm not in deep with the protocol stuff, maybe Ralf will give you more insides.

Comment 5 Ralf Sternberg 2011-02-17 10:05:36 EST

Hi Rüdiger, yes we discussed this too. Our reasoning was that with the protocol we'll have JSON as exchange format. In JSON, there is no distinction between integer and floating point numbers. Even if a number has no fractional digits, it can still be in floating point notation, e.g. "23.0". Therefore, I think JSON numbers have to be parsed as floating point.

I'm not so sure about rounding. I remembered that we once had a problem with fractional scroll values (which in itself are not a problem). I thought about it as a protocol endpoint, and thus along the lines of the http://en.wikipedia.org/wiki/Robustness_Principle . But when it comes to a selection index, rounding is of course plain wrong. Thanks for pointing to it. We should revise this again.

Comment 6 Ralf Sternberg 2011-02-19 01:34:35 EST

After looking at the callers of this new parseInt method again (reading selection indices, bounds, etc.), it's obvious that fractional numbers are not acceptable. Changed the method to throw an exception if the parsed number has a fractional part.

Comment 7 Ivan Furnadjiev 2011-02-19 03:23:50 EST

(In reply to comment #6)
> After looking at the callers of this new parseInt method again (reading
> selection indices, bounds, etc.), it's obvious that fractional numbers are not
> acceptable. Changed the method to throw an exception if the parsed number has a
> fractional part.
Maybe I miss something, but the idea was to prevent the NumberFormatExceptions that crashes the application in case of fractional numbers. With this solution, we replace the NumberFormatExceptions with IllegalArgumentException which again will crash the application. Now, we have an improvement only with values like "23.0", which I didn't see at all. Now I think that this is the better idea:
> As we have a custom (non native) scrollbars now, we can put a general saveguard
> in the scrollbar itself - to always fire events with int values. The other
> place, where we got such a case, is the mouse wheel events.

Comment 8 Ralf Sternberg 2011-02-19 06:50:16 EST

(In reply to comment #7)
> Maybe I miss something, but the idea was to prevent the NumberFormatExceptions
> that crashes the application in case of fractional numbers. With this solution,
> we replace the NumberFormatExceptions with IllegalArgumentException which again
> will crash the application. Now, we have an improvement only with values like
> "23.0", which I didn't see at all. Now I think that this is the better idea:

Accepting e.g. a "3.5" as a selection index would mean to silently swallow a serious problem. If the current solution does not fix the problem, then please reopen and let's add a safeguard on the client.

Comment 9 Ivan Furnadjiev 2011-02-19 07:00:11 EST

I will reopen it. The real solution will be a safeguard on the client as suggested in comment#3.

Comment 10 Rüdiger Herrmann 2011-04-15 09:53:30 EDT

Sorry for nagging again. Is there a chance to get this into 1.4? Though floating point number seem to occur rarely, in terms of overall robustness this would be huge plus.

Comment 11 Ivan Furnadjiev 2011-04-19 07:07:42 EDT

Added rounding in ScrollBar.js#getValue(). JS test added too. As mouse wheel data is used to set the ScrollBar position, I think that this fix is enough to close the bug. Changes are in CVS HEAD.