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

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Sven Giermann]

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...

Comment 1

[Sven Giermann]

Attachment: ltnMinimonthPick() to not change view without (real) date change

Comment 2

[Sven Giermann]

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;
>

Comment 3

[Sven Giermann]

Attachment: Rev2: ltnMinimonthPick() to not change view without (real) date change

Sorry, I'm not very familiar with the use of Bugzilla.
I made a mistake in the patch and submitted the corrected version.

Comment 4

[Stefan Sitter [:ssitter]]

Comment on attachment 270551 [details] [diff] [review]
ltnMinimonthPick() to not change view without (real) date change

Removing request per Comment #3.

Comment 5

[Michael Büttner (no reviews TFN)]

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

Comment 6

[Sven Giermann]

Attachment: Modify datetimepickers.xml to only fire real changes

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.

Comment 7

[Michael Büttner (no reviews TFN)]

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.

Comment 8

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 9

[Andreas Treumann]

Verified in Lightning build 2007080803 -> task is fixed.