search

plusonelabs / calendar-widget

An calender widget for your Android home screen.
Apache License 2.0
369 stars 127 forks source link

Fix Wonky All Day Event Visualization #185

Closed walles closed 9 years ago

walles commented 9 years ago

In some cases it's possible to get an all day event into the calendar that spans zero days (starts and ends at the same timestamp). Google Calendar's web UI renders such events as spanning one full day, so we should do that as well.

I have no idea how to put such events in the calendar, but I have one of them in mine so it is possible.

Without this change, the Calendar Widget says about my event that it starts on Sunday (correct) and ends on the day before, Saturday (wrong).

This does seem to be a corner case, because at least one other calendar app (DigiKal) also fails at this event. In their case they render the (all day Sunday) event as starting on Saturday (wrong) and ending on Friday (even more wrong).

walles commented 9 years ago

Note that this change builds on the unit testing support introduced by PR #180.

yvolk commented 9 years ago

Hi @walles, Thank you for your efforts and for your patience. These commits give me mixed feelings, and so I cannot except them. Let me explain:

  1. Regarding NPE fix I still feel that we are going the wrong way: trying to cure a consequence and not a cause of the problem. This is why we are testing a fix to an Exception, which doesn't occur in the application (it occurs in a mocked extract only. ..)
  2. Regarding this "Wonky All Day Event". Fixing it by changing definition of the spansOneFullDay function to something that doesn't align with its name looks dangerous. We may break something that works, now or later, using that function...
  3. Regarding test approach as a whole. As I already wrote, we need to test working application first. Tests, which I see so far, are testing only small parts, not the whole path from a Calendar to a Widget on a screen. This is why they cannot prove that our application will deal with e.g. these strange all-day-events as we expect it. Please look at a test pack of https://github.com/andstatus/andstatus and launch it on emulator to get a clue of what I am thinking of.
walles commented 9 years ago

The definition of spansOneFullDay() in this PR matches the behavior of Google Calendar's web UI.

yvolk commented 9 years ago

@walles but it doesn't match the name of this method. So it becomes misleading and may cause further bugs...

walles commented 9 years ago

Google's web UI actually displays the event as spanning one full day.

What would you call the method?

yvolk commented 9 years ago

I mean that the code, which you proposed for the method, looks like a hack, which works by coincidence. The proposed code means "spansNotMoreThanOneFullDay" ?!

walles commented 9 years ago

No it doesn't, I think you've been reading an earlier version of the patch.

The current version's logic is:

  1. If it's an all day event, and it starts and ends at the same time, consider it to span one single day.
  2. In all other cases, use the logic that was already there (start + 1d == end).
yvolk commented 9 years ago

"No it doesn't, I think you've been reading an earlier version of the patch." Now I see, your Pull request has commits, which override each other...

walles commented 9 years ago

@yvolk I rebased this on top of your fix for the NPE crash, could you have another look?

yvolk commented 9 years ago

@walles We implemented the com.plusonelabs.calendar.calendar.CalendarEventProvider#fixAllDayEvent procedure, which fixes such "wonky zero days all-day events". Please test!

So this pull request became obsolete and may be closed?!

walles commented 9 years ago

Just from reading the code, that should do it.

The event in my calendar was on the 30th of August (and it's in a calendar to which I only have read access), so I can't verify.

Thanks for fixing this, I'll re-open if I see this behavior again.