search

JFXtras / jfxtras

A supporting library for JavaFX, containing helper classes, extended layouts, controls and other interesting widgets.
http://jfxtras.org
Other
599 stars 123 forks source link

Remove extra day from LocalDateTimeRange when localDateTimeRangeCallb… #41

Closed daviddbal closed 8 years ago

daviddbal commented 8 years ago

…ack is executed

The extra day caused some unnecessary extra appointments to be created when making appointments from a repeat rule.

tbee commented 8 years ago

So, these are LocalDateTime values. Suppose we display a month, Jan 2015, then the current code will hold: 2015-01-01 00:00:00 to 2015-02-01 00:00:00

AFAIK see, in your PR range will hold: 2015-01-01 00:00:00 and 2015-01-31 00:00:00

This of course is incorrect. Given that the end datetime is exclusive, I still believe the original range is correct.

Am I correct or mistaken?

daviddbal commented 8 years ago

I am using the dates inclusive of the end date. That seemed to make more sense that excluding the end date. I believe that Internally agenda uses inclusive dates. Why should agenda force a different convention by the implementor?

tbee commented 8 years ago

Because, if you use inclusive enddates, you'd have to generate 2015-01-31 23:59:59.999999999999999999999999999999999999999999999

daviddbal commented 8 years ago

I am not sure why you want to use LocalDateTime instead of LocalDate. I only use LocalDate so the time portion doesn't matter to me. I assume every day displayed is complete. Are you planning on an intraday skin?

What is wrong with the inclusive end date you displayed? It is easily generated as follows:

        lEndLocalDate.plusDays(1).atStartOfDay().minusNanos(1)
tbee commented 8 years ago

The equally simple question would be: what is wrong with date < enddate instead of date <= enddate?

The reason for date time is that Agenda does not know what range is rendered. For the skin you are using, date is sufficiently accurate, but suppose you have a very narrow time range in a skin, say from 13:00 to 15:00 on the same day. Then time is a factor.

Tom

daviddbal commented 8 years ago

The problem is I am using LocalDate for the range, not LocalDateTime. When I convert I get an extra day. I didn't consider repeating intervals less than one day.

daviddbal commented 8 years ago

I discovered that the Date and Time between methods all have the first temporal argument inclusive and the second one exclusive. Your output is consistent with that convention so I suppose it is best to keep it that way. I'll change my code to accommodate it.

You can reject this pull request.

tbee commented 8 years ago

I did think about that :-) Thanks for putting it under scrutiny.