Fix Leica .lei timestamp formatting

[melissalinkert]

Millisecond parsing in Joda has different results than with Date/SimpleDateFormat. If the number of ms digits in the format string exceeds the number of ms digits in the date string, then zero padding happens on the wrong side (e.g. 93 becomes 930 instead of 093). If the number of ms digits in the date string exceeds the number of ms digits in the format string, then parsing fails.

See JodaOrg/joda-time#62.

Fixes http://trac.openmicroscopy.org/ome/ticket/12117.

[ghost]

Don't we want to continue to throw an IllegalArgumentException in this case as before, rather than returning -1, which is a valid-but-incorrect date? There's no way to distinguish between a valid date and an error condition. I should have clarified when writing this comment that this isn't a change in behaviour from this PR, but it should arguably not be returning an invalid date and just throw.

[ghost]

The change looks reasonable, but I'll have to do some testing to check how this behaves WRT timezones given that it's passing through Date. Ideally this should be resulting in a UTC timestamp, not offset to local time.

[ghost]

An alternative solution would be to correct the input in LeicaReader:

    for (int j=0; j<numStamps; j++) {
      String ts = getString(64);
      ts = ts.replaceAll(":(\\d)$", ":00$1");
      ts = ts.replaceAll(":(\\d\\d)$", ":0$1");
      timestamps[seriesIndex][j] = ts;
      addSeriesMetaList("Timestamp", timestamps[seriesIndex][j]);
    }

(just as an example; the regex compiling should really be outside the loop)

[ghost]

The change, bar the exception handling/error return comment above, looks OK. However, I'm not convinced this is necessarily the best solution. My main concern is that this removes the timezone support which joda provides, but which Date lacks, and which was the primary reason for replacing it with joda.

The root problem is that the parsing of SS (rather than SSS) is different between joda and Date. It's ambiguous and both implementations make valid choices; the joda strategy of treating them as the most significant digits is arguably more correct. We can either revert to using the SimpleDateFormt/Date across the board, which leaves us with a somewhat inconsistent mix of Date/Calendar and joda, or fix up the readers which have ambiguous input so they parse properly with joda, as illustrated above. I think the latter would be preferable since this will be limited to a small number of readers such as LeicaReader.

[mtbc]

My 2¢:

1. Joda's strategy matches what ISO 8601 requires regarding fractions on seconds components.
2. Until we have JSR-310 or whatever I'd certainly be in favor of trying to avoid Java's current date-and-time handling, it's horrible enough for complex use cases to be much of Joda's raison d'être, and mixing libraries in the same codebase might ask for extra confusion (e.g., multiple settings for default time zone).

@rleigh-dundee's suggestion of fixing up the strings before parsing them into timestamps is a good one, but unless we think that this idea that 01.44 is really 01.044 is fairly ubiquitous, I'd be leery of doing it in general code instead of lei-specific code.

[melissalinkert]

Pushed two commits to revert the DateTools change and fix this in the Leica reader. I am concerned that we will run into the same problem elsewhere, so it will be important to keep an eye out for similar problems going forward.

[ghost]

The change looks fine, thanks. The only minor detail is that with leica/martin/samples-SP2/samples-SP2.lei I see:

Timestamp #094: 2007:10:26,11:20:15:437
Timestamp #095: 2007:10:26,11:20:18:93
Timestamp #096: 2007:10:26,11:20:18:93
Timestamp #097: 2007:10:26,11:20:20:765

Would it be possible to pad the millisecond formatting appropriately here, to remove the ambiguity? Or alternatively, use the normalised Timestamp value so it's a proper ISO-8601 date string.

[melissalinkert]

Should be sorted out now.

[ghost]

The output is now fine, thanks. Do we need two assignments of timestamps[seriesIndex][j] though, or could it just be a single assignment using getString(64) as a parameter to getTime?

[melissalinkert]

One more commit pushed to address the last comment. Hopefully nothing further that needs to be fixed?

[ghost]

The change looks fine, thanks. Good to merge if green from me.

[melissalinkert]

--no-rebase