Closed Bug 386558 Opened 17 years ago Closed 17 years ago

ltnDateTextPicker-onChange changes view without (real) change of content

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: giermann, Assigned: giermann)

Details

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Lightning 0.5 (build 2007062404)

The "onchange" event for 'ltnDateTextPicker' is triggered on every focus-left, even if no change has been done.


Reproducible: Always

Steps to Reproduce:
1. open Inbox
2. click into ltnDateTextPicker (maybe copy the displayed date?)
3. leave focus, i.e. by clicking into the message pane
4. wait a few seconds...
Actual Results:  
View is changing to Calendar view by
ltnMinimonthPick().

Expected Results:  
View should stay on selected Mail folder, because no change to date was made.

I created a quick&dirty hack to check whether the dates in ltnDateTextPicker
and ltnMinimonth are different, if the "onchange" comes from ltnDateTextPicker.

Well, I think there could be a much better solution, but I converted the dates
to strings, because the initial setting (today) contains the time as well...
Attachment #270551 - Flags: review?(michael.buettner)
Comment on attachment 270551 [details] [diff] [review]
ltnMinimonthPick() to not change view without (real) date change

>--- messenger-overlay-sidebar.js	2007-04-17 04:02:40.000000000 +0200
>+++ messenger-overlay-sidebar.js-mod	2007-06-28 10:09:39.328125000 +0200
>@@ -217,16 +217,24 @@ function nextMonth(dt)
> }
> 
> var gMiniMonthLoading = false;
> function ltnMinimonthPick(minimonth)
> {
>     if (gMiniMonthLoading)
>         return;
> 
>+    // SG, 27.06.2007 - return if no (real) change on ltnDateTextPicker
>+    if (minimonth.id == "ltnDateTextPicker") {
>+        var trgDate = minimonth.value.getFullYear() + "-" + minimonth.value.getMonth() + "-" + minimonth.value.getDate();
>+        var srcDate = document.getElementById("ltnMinimonth").value.getFullYear() + "-" + document.getElementById("ltnMinimonth").value.getMonth() + "-" + document.getElementById("ltnMinimonth").value.getDate();
>+        if (trgDate == srcDate)
>+            return;
>+    }
>+
>     var jsDate = minimonth.value;
>     document.getElementById("ltnDateTextPicker").value = jsDate;
>     var cdt = new CalDateTime();
>     cdt.year = jsDate.getFullYear();
>     cdt.month = jsDate.getMonth();
>     cdt.day = jsDate.getDate();
>     cdt.isDate = true;
>
Sorry, I'm not very familiar with the use of Bugzilla.
I made a mistake in the patch and submitted the corrected version.
Attachment #270570 - Flags: review?(michael.buettner)
Comment on attachment 270551 [details] [diff] [review]
ltnMinimonthPick() to not change view without (real) date change

Removing request per Comment #3.
Attachment #270551 - Attachment is obsolete: true
Attachment #270551 - Flags: review?(michael.buettner)
Assignee: nobody → giermann
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 270570 [details] [diff] [review]
Rev2: ltnMinimonthPick() to not change view without (real) date change

>+        var trgDate = minimonth.value.getFullYear() + "-" + ...
There's no need to create a string by examining the components of a calIDateTime object, ignoring the fact that this won't work for different timezones. A much cleaner way would be to use toString() in order to create a literal string from it, see [1]. This gives you something like 'Tue Jul 03 2007 00:00:00 GMT+0200'. Comparing two calIDateTimeObjects with the '==' operator obviously doesn't work here since you're comparing two distinct objects. Comparing the textual representation is one way of comparing, but there's a method available that does just what you want, comparing two calIDateTime objects, see [2] for the details.

Generally, I think it's a better strategy to not fire the event at all if there's nothing to do on the receiver side. You could compare the calIDateTime objects at [3] and [4] and avoid firing the event if those objects represent the very same time. This would cut the unnecessary steps just before anything happens. It strikes me that this is much more efficient than filtering out these unwanted events at the receiver side.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIDateTime.idl#179
[2] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIDateTime.idl#228
[3] http://lxr.mozilla.org/mozilla1.8/source/calendar/resources/content/datetimepickers/minimonth.xml#566
[4] http://lxr.mozilla.org/mozilla1.8/source/calendar/resources/content/datetimepickers/minimonth.xml#551
Attachment #270570 - Flags: review?(michael.buettner) → review-
Michael,

thank you for checking my code. I agree that it would be best to not fire the event if no change was made.
Checking your link to 'minimonth.xml' I found out, that the my initial approach could also be achieved by modifying the 'datetimepickers.xml' in '<method name="update">'.

I attached the new patch to only fire a change-event, if the date portion changed.
Attachment #270723 - Flags: review?(michael.buettner)
OS: Windows XP → All
Hardware: PC → All
Comment on attachment 270723 [details] [diff] [review]
Modify datetimepickers.xml to only fire real changes

>+                var dateChanged = true;
>+                try {
>+                  if (this.mValue != null) {
>+                    dateChanged = (this.mValue.getDate() != aValue.getDate()) ||
>+                                  (this.mValue.getMonth() != aValue.getMonth()) ||
>+                                  (this.mValue.getFullYear() != aValue.getFullYear());
>+                  }
>+                } catch (ex) {}
>+
I don't think we need to wrap these statements in a try/catch block since these methods don't throw anyway.

You're right, this is indeed the most elegant solution -> r=mickey. Thanks for the patch, Sven.
Attachment #270723 - Flags: review?(michael.buettner) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
Verified in Lightning build 2007080803 -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: