Closed Bug 335643 Opened 14 years ago Closed 13 years ago

Tasks in multiweek/month view are not displayed if start date is before first day in view

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Sunbird 0.3

People

(Reporter: mttl, Assigned: thomas.benisch)

Details

(Keywords: dataloss)

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060426 Mozilla Sunbird/0.3a1+

A task is not displayed in the multiweek or month view if the start date of the task is before the first day in the selected view. It is only shown if the start is visible in the view. An Event is always shown, regardless the visibilty of its start date. This also should be the case for tasks.

Reproducible: Always

Steps to Reproduce:
1. Create a task spanning two weeks (or months).
2. Select multiweek (month) view and "Tasks in view".
3. Configure the view such that the first week (month) of the task is not visible.

Actual Results:  
The task is not displayed in the view

Expected Results:  
The task should be displayed even if the first day of the task is before the first day of the actual view.
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060429 Mozilla Sunbird/0.3a2
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → Windows 2000
Version: unspecified → Trunk
reassigned.
Assignee: base → michael.buettner
Assignee: michael.buettner → thomas.benisch
taking over this task
Attached patch patch v1 (obsolete) — Splinter Review
This patch contains a fix for the SQL query for non-recurrent tasks
in the storage calendar.

A task has a date (todo_entry) and a due date (todo_due).
As the query looks quite complex, here's a short overview
(neglecting floating entries):

todo_entry  todo_due  condition
---------------------------------------------------------------------------
  -           -         -
  +           -         todo_entry >= range_start && todo_entry < range_end
  -           +         todo_due >= range_start && todo_due < range_end
  +           +         todo_due >= range_start && todo_entry < range_end
Attachment #223674 - Flags: first-review?(dmose)
In this context I think I also found a bug in the SQL query for
non-recurrent events (calStorageCalendar.js). The correct
query is

var floatingEventStart = "event_start_tz = 'floating' AND event_start"
var nonFloatingEventStart = "event_start_tz != 'floating' AND event_start"
var floatingEventEnd = "event_end_tz = 'floating' AND event_end"
var nonFloatingEventEnd = "event_end_tz != 'floating' AND event_end"
// The query needs to take both floating and non floating into account
this.mSelectEventsByRange = createStatement(
    this.mDB,
    "SELECT * FROM cal_events " +
    "WHERE " +
    " (("+floatingEventEnd+" >= :range_start + :start_offset) OR " +
    "  ("+nonFloatingEventEnd+" >= :range_start)) AND " +
    " (("+floatingEventStart+" < :range_end + :end_offset) OR " +
    "  ("+nonFloatingEventStart+" < :range_end)) " +
    "  AND cal_id = :cal_id AND recurrence_id IS NULL"
    );

For the case start_offset != end_offset, the original query is wrong.
If you confirm this, I will write a separate task.
(In reply to comment #4)
> Created an attachment (id=223674) [edit]
> patch v1
> 
> This patch contains a fix for the SQL query for non-recurrent tasks
> in the storage calendar.
> 
> A task has a date (todo_entry) and a due date (todo_due).
> As the query looks quite complex, here's a short overview
> (neglecting floating entries):
> 
> todo_entry  todo_due  condition
> ---------------------------------------------------------------------------
>   -           -         -
>   +           -         todo_entry >= range_start && todo_entry < range_end


If I'm interpreting things correctly, the above line is not correct.  A task with a start date before range_start wouldn't show up, even if it hasn't been completed yet.
Comment on attachment 223674 [details] [diff] [review]
patch v1

As you pointed out above, this code is pretty difficult to understand without a bunch extra documentation.  I think that's likely to hurt us the next time somebody needs to do maintenance on this code, because it's really tricky to verify that it does exactly what it ought to be doing.  I'd suggest using some more helper variables to further breakdown the code into more human-meaningful chunks.  In particular, I think it might be helpful to factor the code into "floating" and "non-floating" that can be ANDed together to form the WHERE clause.  There may be other additional ways to break this up as well.
Attachment #223674 - Flags: first-review?(dmose) → first-review-
(In reply to comment #5)

> For the case start_offset != end_offset, the original query is wrong.
> If you confirm this, I will write a separate task.

Agreed; that looks like a bug too.  Nice catch!
(In reply to comment #6)
I think before I create a new patch, we should get in agreement about
how the SQL query should look like. I added some item numbers to the
overview table, which makes refering to each row easier:

     todo_entry  todo_due  condition
--------------------------------------------------------------------------------
(1)    -           -         -
(2)    +           -         todo_entry >= range_start && todo_entry < range_end
(3)    -           +         todo_due >= range_start && todo_due < range_end
(4)    +           +         todo_due >= range_start && todo_entry < range_end

> > todo_entry  todo_due  condition
> > ---------------------------------------------------------------------------
> >   -           -         -
> >   +           -         todo_entry >= range_start && todo_entry < range_end
> 
> 
> If I'm interpreting things correctly, the above line is not correct.  A task
> with a start date before range_start wouldn't show up, even if it hasn't been
> completed yet.

Your interpretation is correct, but that's actually the same behaviour as
before. Nevertheless this might be wrong, the question is how tasks with
missing date or due date are displayed. The only alternatives I see in this 
case are

(2a)   +           -         todo_entry < range_end
(2b)   +           -         -

I favour (2a). I think (1) is correct, all tasks with missing date and due date
are returned. (4) is also correct, this condition fixes the bug.
(3) is also discussable, tasks with a due date after range_end are not
returned, therefore an alternative might be

(3a)   -           +         todo_due >= range_start

What do you think?
(In reply to comment #8)
> (In reply to comment #5)
> 
> > For the case start_offset != end_offset, the original query is wrong.
> > If you confirm this, I will write a separate task.
> 
> Agreed; that looks like a bug too.  Nice catch!

I filed Bug #339670.
After spending a couple of hours today thinking about this, I believe that the use cases here are more varied and complex than I realized.  As a result, I'd like to suggest splitting this into two pieces.  

Specifically, this bug would just be used to address the obvious dataloss (non-matching start & end offsets; start is before the beginning of the range) here, while more or less preserving existing semantics: (only look at todos which have start dates; ignore due dates) both in storage and ICS providers.

I do agree with you that we need to take due dates as well as various permutations of undated items into account.  However, in pondering exactly how to do this, I can imagine various different use cases that would imply different behavior on the part of the app.  I think figuring out the best strategy there is going to require us to spend some time in UI-land enumerating these cases and figuring out how we think they're likely to apply to our target users.  So I'd suggest spinning off that work into another bug.

If that sounds like a reasonable strategy, that reduces the table entries to (1) and (2).  As far as (1), it looks to me like the current semantics is not to include undated entries in the views, so I'd propose sticking with that.  For (2), I agree that (2a) is the way to go there.
(In reply to comment #11)
> After spending a couple of hours today thinking about this, I believe that the
> use cases here are more varied and complex than I realized.  As a result, I'd
> like to suggest splitting this into two pieces.  
> 
> Specifically, this bug would just be used to address the obvious dataloss
> (non-matching start & end offsets; start is before the beginning of the range)
> here, while more or less preserving existing semantics: (only look at todos
> which have start dates; ignore due dates) both in storage and ICS providers.
> 
> I do agree with you that we need to take due dates as well as various
> permutations of undated items into account.  However, in pondering exactly how
> to do this, I can imagine various different use cases that would imply
> different behavior on the part of the app.  I think figuring out the best
> strategy there is going to require us to spend some time in UI-land enumerating
> these cases and figuring out how we think they're likely to apply to our target
> users.  So I'd suggest spinning off that work into another bug.

I think this approach makes sense. There's not only the question, how tasks
with missing date or due date are displayed in the views, but also what
getItems() should return. This might be different, because getItems() is
also used by the calendar todo list, which calls getItems() with null,null
as parameters. In the storage calendar that means, that the start time
is set to the minimal possible value and the end time to the maximum
possible value. In this case, the SQL query should return all tasks.

> If that sounds like a reasonable strategy, that reduces the table entries to
> (1) and (2).  As far as (1), it looks to me like the current semantics is not
> to include undated entries in the views, so I'd propose sticking with that. 
> For (2), I agree that (2a) is the way to go there.

That means, the table will look like the following:

     todo_entry  condition
-----------------------------------------
(1)    -           -
(2)    +           todo_entry < range_end

I think your conclusion for (1) is wrong. Although the views don't display
tasks with missing date (todo_entry), getItems() should return ALL tasks with 
missing date (see my previous comment). Otherwise those tasks won't appear
in the calendar todo list. Therefore (todo_entry IS NULL) must be part of 
the SQL query.
This patch fixes the dataloss problem only.
Attachment #223674 - Attachment is obsolete: true
Attachment #223929 - Flags: first-review?(dmose)
(In reply to comment #12)
> I think this approach makes sense. There's not only the question, how tasks
> with missing date or due date are displayed in the views, but also what
> getItems() should return.

Agreed.  getItems is not the best API in the world and may well need some changes to accommodate todos in a reasonable way.

> I think your conclusion for (1) is wrong.

You're right; my mistake.
Comment on attachment 223929 [details] [diff] [review]
patch v2 (checked in)

r=dmose; patch landed on both trunk and branch using cross-commit.
Attachment #223929 - Flags: first-review?(dmose) → first-review+
If you could spin off another bug for "Investigate in-view task UI", then this bug can be resolved FIXED.  Thanks for the patch!
OS: Windows 2000 → All
Hardware: PC → All
(In reply to comment #16)
> If you could spin off another bug for "Investigate in-view task UI", then this
> bug can be resolved FIXED.  Thanks for the patch!

I filed Bug #339955.
Great; thanks!
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The bug is resolved for local calendars but I still can not see a task in a remote calendar with a start date before the start of the view in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060613 Sunbird/0.3a2+.
(In reply to comment #19)
Confirmed with Lightning 0.1+ (20060615). Works ok when task is shown in storage calendar but fails with task stored in ics file. -> REOPEN
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Requesting blocking0.3 due to possible dataloss.
Flags: blocking0.3?
Flags: blocking0.3? → blocking0.3+
Whiteboard: [no l10n impact]
Attachment #223929 - Attachment description: patch v2 → patch v2 (checked in)
(In reply to comment #23)
> If I subscribe a remote iCalendar with the URL from comment #22,
> I don't see any tasks. If I store the ics file on my local disk
> and subscribe to that file, I see tasks. Any ideas?
which build do you have?
comment 22 works with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060907 Calendar/0.3a2+

I had this problem but opposite result, could subscribe but couldn't import from file
(In reply to comment #24)
> which build do you have?
I used a Thunderbird debug build (20060817) and a Lightning debug
build (2006090817), both Windows.
With this patch also tasks in ics files are shown, if the entry date of
the task is before the first day in the view.
Attachment #237730 - Flags: first-review?(dmose)
Comment on attachment 237730 [details] [diff] [review]
memory calendar patch

r1=lilmatt

Yay for parens!
Attachment #237730 - Flags: second-review?(dmose)
Attachment #237730 - Flags: first-review?(dmose)
Attachment #237730 - Flags: first-review+
Comment on attachment 237730 [details] [diff] [review]
memory calendar patch

Looks good; thanks for the patch!
Attachment #237730 - Flags: second-review?(dmose) → second-review+
Whiteboard: [no l10n impact] → [patch in hand][needs checkin]
"memory calendar patch" checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: [patch in hand][needs checkin]
Target Milestone: --- → Sunbird 0.3
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060917 Calendar/0.3a2+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.