325459 - monthview's relayout fails

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

monthview's relayout does some math with durations, but that fails, due to a bug with adding negative durations.
And if it succeeds, it gets the wrong duration (34 days, that's not a multiple of 7)
And fixing that uncovers other bugs...

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: partial patch, just fixes calDuration.cpp

This patch fixes the duration part, but applying it will raise errors in the month view.

Comment 2

[Michael Büttner (no reviews TFN)]

Attachment: patch for GetInSeconds

GetInSeconds is now aware of the is_neg-flag.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 210357 [details] [diff] [review]
patch for GetInSeconds

I can't review this yet, because those patches will break the monthview. It would be best to create one big patch out of those small patches that fixes everything.
(also, please don't use tabs)

Comment 4

[Joey Minta]

Attachment: full patch for calendar-month-view

dmose reviewed the initial checkin of this code.  This cleans up my mess (How did I write such bad code?).  The problems were mostly reference errors from moving reuseExistingGrid into a separate function.  Combined with the other patches on this bug, there needed to be a restructuring of the logic for when we can and cannot reuse the grid.  Before, we would get a 7 or -7 in the duration, now we need to check isNegative.  I also broke out some of the easier if() cases with returns, to avoid excessive indentation.  A patch -w will follow to make it clear what code actually changed.

Comment 5

[Joey Minta]

Attachment: patch -w

patch -w for review.

Comment 6

[Joey Minta]

Comment on attachment 210427 [details] [diff] [review]
patch -w

bah... this still isn't right :-(  It doesn't reuse the grid when rows don't need to be added or subtracted.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

I think that calculating the duration is wrong: it doesn't take the last day into account, because the last day is inclusive, not exclusive.

Comment 8

[Joey Minta]

Attachment: full patch for calendar-month-view v2

Patch fixes the previous issue of not re-using every time we actually could.  With this new version, we should reuse the grid every month (expect, of course, when the view is first loaded).

Comment 9

[Joey Minta]

Attachment: patch -w

updated patch -w for review

Comment 10

[Joey Minta]

(In reply to comment #7)
> I think that calculating the duration is wrong: it doesn't take the last day
> into account, because the last day is inclusive, not exclusive.
> 

The duration does come out at X weeks+6 days, but it comes out that way for both the old grid and the new start/end date, so it's ok.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 210353 [details] [diff] [review]
partial patch, just fixes calDuration.cpp

r=dmose, but with a modified comment.  Two negatives cannot ever add together to become a positive.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 210357 [details] [diff] [review]
patch for GetInSeconds

r=dmose

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 210478 [details] [diff] [review]
patch -w

Looks good; just a couple of nits:

> 
>           // If we've already drawn a view once, then in almost all cases we
>           // can resuse most of the grid.  We may need to add or subtract a row

Fix the "reuse" spelling as long as you're here?

>           // but this is still much faster than recreating all rows.
>           var canReuse = false;

This var can go away now, I think.

>+
>+          // OK, so we're off by one month.  We either need to add or subtract

s/month/week/, right?

>+          var gridrows = this.monthgridrows;
>+

Since this only gets referenced once, how about just accessing it directly rather than creating a temporary?

r=dmose with those addressed

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 210353 [details] [diff] [review]
partial patch, just fixes calDuration.cpp

>+    // Check if we need to add or substract. Remeber, two negatives
>+    // can add to something positive.
>+    if (idt.is_neg != mDuration.is_neg) {

A better comment:
    // Calculate the new absolute value of the duration
    // For two negative durations, the abs. value will increase,
    // so use + in that case.
    // Ofcourse, also use + when both durations are positive.

Comment 15

[Joey Minta]

All patches checked in

Comment 16

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!