Visual indicator in day and week view to show current time (slot)

RESOLVED FIXED in 1.4

Status

--
enhancement
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: carlvonoelreich, Assigned: bv1578)

Tracking

unspecified

Details

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

In week view and day view, a visual indicator positioned in the calendar to show
'where' exact current time is. A coloured bar for instance that changes position
as time changes. In essence a "you are here" marker, where time is the area.

Reproducible: Always
Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove
Assignee: mostafah → nobody
Component: General → Calendar Views
QA Contact: general → views
Duplicate of this bug: 383636
Summary: Enhancement: visual indicator positioned in the calendar to show 'where' exact current time is. → Visual indicator in day and week view to show current time (slot)
When patching, see also bug 368722, which is related.
Duplicate of this bug: 399870

Comment 5

11 years ago
Created attachment 325945 [details]
example gcal way

This (time indicator line) was also requested in ng (tb and whish) and I was also thinking of something like this. The gcal example I find a basic or minimal one and also works only in week AFAIK. 

This could be much better than in this pic. I'll propose some img and maybe some more than that (very limited patch skills though ..)
This is an OLD bug. I wonder what we can do to get it moving...it's not a huge structural change, so it's not exciting, but it's a very useful feature that every other calendar I know has...
(In reply to comment #6)
Don't expect to see this feature anytime soon unless a new contributor shows up and volunteers to work on this feature.
<http://weblogs.mozillazine.org/calendar/2009/02/calendar_project_at_a_critical.html>
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 506065
Flags: blocking-calendar1.0?
(Assignee)

Comment 9

9 years ago
Created attachment 413411 [details] [diff] [review]
[long] Proposal patch for a time indicator

This patch adds a time visual indicator in day view and week view like showed in the next screenshot.
It allows to set the indicator update frequency between 1 minute (default) and 15 minutes by changing the preference "calendar.view.timeIndicatorInterval", and allows to delete the indicator (and the related timer) by setting to 0 the preference.
The look and the color of both indicators, on the time bar and in the view, are just a proposal (like the whole patch) and could be changed with something different (maybe a little arrow icon on the time bar instead of the small red rectangle, but this is just an idea).

A timer, with a timeout given by the preference value, moves the indicator to a position, in minutes, multiple of the preference interval value. With the first timeout the timer synchronize itself to the multiple.
When hours size is small enough the update interval automatically increase to avoid useless updates of the indicator.
When the user switch to month/multiweek view or to another mode (mail or task) the timer is deleted, and restarts when the user comes back to day-view or week-view (hopefully it should work fine for every situation).

Apart from what I might have missed (and done in a wrong way), there is a problem when the user opens a modal window or dialog (e.g. the "Options" one), because in this case the indicator doesn't move until the window is closed (then the indicator move to the right position).
Don't know if this is a problem that *must* be solved and how it might be solved. I wouldn't know anything else than a thread but it seems a bit exaggerate for an issue like this.
Moreover I have some doubt about the use of the timer that I have always added to  "week-view" element to avoid a duplicate between two |this| elements "week-view" and "day-view". I'm asking for review to clarify this as well.
Thanks Philipp.
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
Attachment #413411 - Flags: review?(philipp)
(Assignee)

Comment 10

9 years ago
Created attachment 413412 [details]
Screenshot: the indicator in day and week views
Attachment #413411 - Attachment description: Proposal patch for a time indicator → [long] Proposal patch for a time indicator
Comment on attachment 413411 [details] [diff] [review]
[long] Proposal patch for a time indicator

>+      <xul:stack anonid="boxstack" style="min-width: 1px; min-height: 1px">
>+        <xul:box xbl:inherits="orient,width,height" flex="1" anonid="topbox">
>+        </xul:box>
>+        <xul:box  anonid="fgboxTimeIndicator" xbl:inherits="orient" class="fgTimeIndicatorContainer">
>+          <xul:box anonid="fgTimeIndicatorspacer"/>
>+          <xul:hbox anonid="fgboxTimeIndicatorInner"> 
>+            <xul:spacer flex="1"/>
>+            <xul:box anonid="fgTimeIndicatorboxTimeBar" class="timeIndicator-timeBar"/>
>+          </xul:hbox>
>+        </xul:box>
>+      </xul:stack>
Instead of adding so many boxes, I think we should try absolute positioning. To do this, try making the stack a box that has the style "display: block". Then you can use CSS positioning, i.e top/left/width/height. This way instead of adding lots of spacers, you can just position the indicator at the right position. Let me know if this doesn't work out. 

>+          // Check interval update limits: 1 - 15 minutes
>+          //                               0 = no time indicator
>+          let interval = Math.max(0, Math.min(15, aInterval));
Why limit to 15 minutes? I think we should set a minimum of 0 and leave the upper limit open.

>+          if (aInterval != interval) {
>+              cal.setPref("calendar.view.timeIndicatorInterval", interval);
>+          }
Just for my understanding, why are you setting the timer interval pref here?

>+          if (weekView.timeIndicatorTimer) {
>+              clearTimeout(weekView.timeIndicatorTimer);
>+          }
I believe clearTimeout() doesn't set the variable to null. To be safe, add  weekView.timeIndicatorTimer = null.

>+          let self = this;
>+          weekView.timeIndicatorTimer = setTimeout(function() {self.updateTimeIndicatorTimer();},
>+                                                   intervalSec * 1000);
Also, you may be interested in using setInterval instead. Thats up to you though, I'll take setTimeout too.



> function showCalendarView(type, event) {
>+    // delete the time indicator timer if type is different from day or week
>+    if (type != "day" && type != "week") {
>+        let weekView = document.getElementById("week-view");
>+        if (weekView.timeIndicatorTimer) {
>+            clearTimeout(weekView.timeIndicatorTimer);
>+            delete weekView.timeIndicatorTimer;
>+        }
>+    }
If at all possible, I'd like to keep this logic somewhere in the bindings, since its very implementation specific. I'll need another coffee to find a solution for this. Regarding this specific piece, maybe the view binding can fire some sort of DOM event when the current view changes (maybe it even already does this?).


Regarding the timer, maybe you can store the timer as a static member in the multiday-base-view. To do this, try adding a:

<field name="mStaticMembers">
  { timerInterval: null,
    testSeeNextParagraph: "123" }
</field>

If XBL works similar to js prototype objects, then storing an object as a field in the xbl definition causes the object instance to be accessible from all bindings that extend it. I'm not sure if this works though. To test, change testSeeNextParagraph in the week-view binding, and then check its value from the day-view binding. If the day-view binding gets the same value as you set in the week-view binding, then it works.



>+.timeIndicator {
>+.timeIndicator[show="true"][orient="vertical"] {
>+.timeIndicator[show="true"][orient="horizontal"] {
...
Regarding the CSS rules. Do they differ for mac and windows? Please put all common rule parts in base/themes/common and only the differing parts into base/themes/?instripe/


>+// time indicator update interval in minutes (0 = no indicator)
>+pref("calendar.view.timeIndicatorInterval", 1);
Should we default to 1 minute? I think 15 minutes is totally sufficient, if the user wants to have this updated faster then he can change the pref himself. I'd like to hear other opinions on this though.


r- to consider comments. Thank you for the patch, and sorry for the long review wait!
Attachment #413411 - Flags: review?(philipp) → review-
Duplicate of this bug: 551208

Comment 13

9 years ago
What needs to be done to get this bug considered for a target milestone? Or is this i already in progress, just not reflected on the bug status? Thanks!
Michael, the comments mentioned in comment 11 need to be taken care of. Then I can re-review and it will be part of the next release thereafter.
Flags: blocking-calendar1.0?

Comment 15

8 years ago
Created attachment 482439 [details] [diff] [review]
patch to add a marcus-bains line to lightning

Attached is another patch against this bug, independent of the one in
comment #11 (sorry, independent work).  My patch has tentative support
for orientation.  I've tested it on the comm 1.9.2
version of Lightning (I think that's sunbird-1.0-0.28.b2pre)
(Assignee)

Comment 16

8 years ago
Created attachment 482456 [details]
marcus-bains line patch screenshot

For those ones (like me) that don't know what is a marcus-bains line, this is a screenshot from the previous patch.
Looking at the visual design, personally I prefer the first design. To my mind the line doesn't need to show the exact time (users have a system clock for that), but only give an idea where in their schedule they currently are. John, what is your opinion about the first patch/design?

Comment 18

8 years ago
(Sorry for delay replying, I wasn't on the cc list.)

About comment #17, I would prefer that the line go all the way across the day, as comment #11's patch does.

I personally like having the exact time shown (after all, I have a watch and a cellphone and a wall clock too---the trick is getting the time *where I need it*).

But although I like my patch, I'm not going to quibble.  Please put SOME kind of Marcus Bains line in the release, then we can improve it.

(If I had infinite time I'd (a) make the line go all the way across like #11, and (b) show the time in a small font, like http://alumni.media.mit.edu/~rahimi/marcus-bains-line/, and (c) make the time flip which side of the line it's on when it's in 29 to 1 minute before the hour so it doesn't collide with the regular hour times.  I tried to do (a) but my XUL-foo is not strong enough.  (b) should be just css-hacking, and (c) requires absolute positioning, again exceeding my knowledge of XUL.)
(Assignee)

Comment 19

8 years ago
Created attachment 493830 [details] [diff] [review]
patch - v2

(In reply to comment #11)

I've taken care of all notes in comment #11. For some solutions I need your opinion though.

> >+      <xul:stack anonid="boxstack" style="min-width: 1px; min-height: 1px">
> >+        <xul:box xbl:inherits="orient,width,height" flex="1" anonid="topbox">
> >+ ...
> Instead of adding so many boxes, I think we should try absolute positioning. 
> ...

I've done that, with some problems though. It seems that the "flex" attribute doesn't work in any direction with absolute positioning, hence I've added to the boxes with class "calendar-time-bar-box-even" the same width of the "calendar-time-bar" element in order to give to the children boxes the same size of their parent. Let me know if there is another way.

Since I was at it, I tried to do the same for the indicator in day/week view. The patch worked with Thunderbird 3.1 but the behavior was completely different with Thunderbird 3.3a2 (Shredder). At last I've turned back to the previous structure with a container box and the indicator inside, which works on both Thunderbird 3.1 and 3.3. It has a different way for the positioning i.e. by setting the margin-top attribute, could it be a problem if that attribute it's not available for the users in the userChrome.css file?


> >+          // Check interval update limits: 1 - 15 minutes
> >+          //                               0 = no time indicator
> >+          let interval = Math.max(0, Math.min(15, aInterval));
> Why limit to 15 minutes? I think we should set a minimum of 0 and leave the
> upper limit open.

Set the upper limit to 1440 minutes (24 hours).


> >+          if (aInterval != interval) {
> >+              cal.setPref("calendar.view.timeIndicatorInterval", interval);
> >+          }
> Just for my understanding, why are you setting the timer interval pref here?

setTimeIndicatorInterval() method is also called when the user changes the preference with the config. editor. My idea is to check whether the preference just edited is inside the valid range, and set it to the upper or the downer limit when the user inputs a wrong value (out of the range).


> >+          let self = this;
> >+          weekView.timeIndicatorTimer = setTimeout(function() {self.updateTimeIndicatorTimer();},
> >+                                                   intervalSec * 1000);
> Also, you may be interested in using setInterval instead. Thats up to you
> though, I'll take setTimeout too.

I've used setTimeout for the first update and setInterval for the next.


> > function showCalendarView(type, event) {
> >+    // delete the time indicator timer if type is different from day or week
> >+    if (type != "day" && type != "week") {
> >+        let weekView = document.getElementById("week-view");
> >+        if (weekView.timeIndicatorTimer) {
> >+            clearTimeout(weekView.timeIndicatorTimer);
> >+            delete weekView.timeIndicatorTimer;
> >+        }
> >+    }
> If at all possible, I'd like to keep this logic somewhere in the bindings,
> ...

I've done that by adding the logic inside onResize() method, which already has a call to the function that updates the indicator, and is called every time the views change.
About the mode instead, I've added a listener for the mode change. Since it should be called only one time, I've added it only to the weekview binding.


> Regarding the timer, maybe you can store the timer as a static member in the
> multiday-base-view. To do this, try adding a:
> 
> <field name="mStaticMembers">
>   { timerInterval: null,
>     testSeeNextParagraph: "123" }
> </field>
> 
> If XBL works similar to js prototype objects, then storing an object as a field
> in the xbl definition causes the object instance to be accessible from all
> bindings that extend it. I'm not sure if this works though. ...

As you suspected, the code doesn't work, it returns the error 

Error: invalid label
Source File: chrome://calendar/content/calendar-multiday-view.xml
Line: 2665, Column: 9
Source Code:
       testSeeNextParagraph: "123" }

I thought that the basebinding() method could be a solution but the equivalent in JS  this.__proto__.__proto__.call() doesn't work as expected (at least, I can't make it work as expected). At last, without waiting for your answer to my mail of May, sorry Philipp ;-), I asked on mozilla.dev.tech.xbl and they told me that it's impossible to do it directly in XBL, and it has to be done with a global object inside a owner document or with js prototype objects.
In this patch I've used a global object inside calendar-views.js. Let me know whether it's a right choice.


> Regarding the CSS rules. Do they differ for mac and windows? Please put all
> common rule parts in base/themes/common and only the differing parts into
> base/themes/?instripe/

It seems that bug 533096 has not been fixed yet and the file calendar-views.css has not been moved to base/themes/common for the common code. Is it right?
Attachment #413411 - Attachment is obsolete: true
Attachment #493830 - Flags: review?(philipp)
(Assignee)

Comment 20

8 years ago
(In reply to comment #17)
> Looking at the visual design, personally I prefer the first design. To my mind
> the line doesn't need to show the exact time (users have a system clock for
> that), but only give an idea where in their schedule they currently are.

Before starting to work on this bug, I sent to you an e-mail and you explained me the reasons why the update interval should be customizable, automatically growing when pixels per minute is small and erasable as well (along with the indicator) according to users preference. Some of these features are not possible with an indicator that shows the current time. It would be a nonsense showing a time that update himself e.g. every five minutes or that shows the same time for five minutes because pixel per minute is small.
If we want to show the time, we must update the indicator every minute (obviously turn off the timer when the mode is different from calendar or the view is different from "day" or "week" is always possible). In this case the user should only have the choice to show or hide the indicator by setting a boolean preference instead of an integer.

If we want to show the current time, the indicator must be _reliable_. Strange to say it but, IMHO, more reliable than the case of an indicator with just a line (an arrow, an icon or every other symbol with immutable look) because the current time might be used by the users to beat the time of their daily (office\work) life instead of the system clock or the wrist watch.
At the moment, johnn's patch, and mine too (as I said in  comment #9), have the problem that the code doesn't update the indicator if the user opens e.g. the "Options..." dialog or the "Account setting..." dialog i.e. modal windows. These windows might remain open with the indicator visible and, in this case, the presence of a visual indicator that shows a *static* current time is, by far, more dangerous than an indicator with a line in a static position.
If we want to show the time, IMHO this problem should be solved.


(In reply to comment #18)
> I personally like having the exact time shown (after all, I have a watch and a
> cellphone and a wall clock too---the trick is getting the time *where I need
> it*).
> 
> But although I like my patch, I'm not going to quibble.  Please put SOME kind
> of Marcus Bains line in the release, then we can improve it.
> ... 

Apart from that mentioned above, doesn't seem a problem adding a label with the current time, but a problem might be finding the right look. In particular, adding the current time on the left side of the timebar and reducing the font size doesn't solve the overlap issue with the fixed hours, because when time is in the format "hh.mm AM/PM", both labels take almost the whole timebar width.
(Assignee)

Comment 21

8 years ago
Created attachment 494142 [details]
Different indicators for the time bar

These are some examples of indicators that can be easily implemented in the patch (obviously the red color is just an example).
(Assignee)

Updated

8 years ago
Duplicate of this bug: 617533
(Assignee)

Comment 23

8 years ago
Comment on attachment 493830 [details] [diff] [review]
patch - v2

I apologize Philipp, I remove the review request because there is a problem in the patch. I'm going to attach a new one soon.
Attachment #493830 - Attachment is obsolete: true
Attachment #493830 - Flags: review?(philipp)
(Assignee)

Comment 24

8 years ago
Created attachment 497135 [details] [diff] [review]
[after beta] patch - v2.1

Compared with patch-v2, I've changed a bit the conditions that trigger the timer, moreover, the method that moves the indicator is now some ms faster.
Considerations about the first patch's review are the same in comment #19.

Testing this patch is a bit annoying. Different modes, different views (with today or not), switch from one to another, close and restart TB, enable and disable the indicator in different situations, change the update interval, resize the views, wait every time at least one minute to see if the indicator is moving or the timer it's off. I hope I don't miss something.
Since another patch is (most) probably necessary, in this patch I've left a few "LOG()" in order to check the timer activity and the view type. I will delete them in the next patch after the review.
Attachment #497135 - Flags: review?(philipp)
Decathlon, do you think we should take this patch for the upcoming release?
(Assignee)

Comment 26

8 years ago
(In reply to comment #25)
> Decathlon, do you think we should take this patch for the upcoming release?

There are many questions from comment 19 on that require an answer, probably it will need a further patch, and, above all, the patch needs to be tested. From this point of view nothing better than a beta for a bit of test ;-) but it would be better to avoid a new feature when it has not been tested enough. Honestly, at this point it seems to me there is too little time before the release.
Duplicate of this bug: 649096
Attachment #497135 - Attachment description: patch - v2.1 → [after beta] patch - v2.1

Comment 28

8 years ago
Any news on where this is going?  I'd like to add my support for getting some form of Marcus Bains line added to Lightning.  If including the time is an issue, please just add a plain line without the time and then we can sort out adding the time later on.
This is just a matter of me finding time for review and testing and answering questions. I won't do this until after the release though, since it would be too risky.
Now that Lightning is getting close to 1.0, can we consider including this patch into the main distribution?  If it is too complicated, I'm willing to spend some time to write a minimally invasive version, perhaps just a single red line.  However, before I do that I'd like to know if the maintainers have enough time to review it.  Please provide an update on the status of this bug.
My personal opinion: The release off Lightning 1.0 is in less than two weeks. Therefore it is way too late in the release cycle to include it now. In addition new features should land on comm-central only to allow enough testing and feedback. If it get reviewed in the next two weeks it might be included in the Lightning release for Thunderbird 10.
I appreciate that it is late in the release cycle.  However, this bug is many years old.  Furthermore, a patch was created to address the issue, but it languished.  There needs to be a better process by which simple features can be added without undue delay.  Otherwise, the talents of the open source community are not leveraged.

From a larger perspective, it seems silly that a simple red line that exists in every other calendar program has not been implemented in over 6 years.  If I can get some level of commitment from the maintainers, I would be happy to spend time working on the code.
I understand your pain and I too would like to see this in. I have postponed review since there were other bugs more important. If there were more people actively working on the project, we would have more time for reviews and this bug might have made it in earlier.

I am willing to take the patch already attached, but right now I need to make sure that all the sharp corners for 1.0 are rounded off. I don't think we should force this into 1.0, remember that Thunderbird 10 is due in less than 14 weeks, so its not all that long away.

If you would like to help out and make sure that situations like these don't happen as often in the future, I'd be delighted to have you working with us. See me in IRC (nick: Fallen) on irc.mozilla.org, channel #calendar for more details or drop me an email.
Comment on attachment 497135 [details] [diff] [review]
[after beta] patch - v2.1

Review of attachment 497135 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Decathlon from comment #19)

> I've done that, with some problems though. It seems that the "flex"
> attribute doesn't work in any direction with absolute positioning, hence
> I've added to the boxes with class "calendar-time-bar-box-even" the same
> width of the "calendar-time-bar" element in order to give to the children
> boxes the same size of their parent. Let me know if there is another way.
Correct, flex won't work for absolute positioning since its then not following the normal XUL box model but doing things differently. I think you can use the width: attribute to make up for it (i.e width: 100% or such). If it doesn't work thats ok.


> 
> Since I was at it, I tried to do the same for the indicator in day/week
> view. The patch worked with Thunderbird 3.1 but the behavior was completely
> different with Thunderbird 3.3a2 (Shredder). At last I've turned back to the
> previous structure with a container box and the indicator inside, which
> works on both Thunderbird 3.1 and 3.3. It has a different way for the
> positioning i.e. by setting the margin-top attribute, could it be a problem
> if that attribute it's not available for the users in the userChrome.css
> file?
users can change anything in userChrome.css, there should be no limits. Given that we are now with a much later Thunderbird, is there anything you can improve?

> (at least, I can't make it work as expected). At last, without waiting for
> your answer to my mail of May, sorry Philipp ;-), I asked on
Not sure if I've said this, but no worries. My fault for not answering in time!

> It seems that bug 533096 has not been fixed yet and the file
> calendar-views.css has not been moved to base/themes/common for the common
> code. Is it right?
Yep, ignore that for now.

All other parts of comment 19 are acknowleged and I am ok with it.

Given the patch works as expected and only has some minor issues, I'm going to set r+. I'll attach the unbitrotted patch in a moment.

::: calendar/base/content/calendar-multiday-view.xml
@@ +2405,5 @@
>      <implementation implements="calICalendarView">
>        <constructor><![CDATA[
> +        // add a listener for the mode change
> +        if (this.id == "week-view") {
> +            this.mModeHandler = function modeHandler(event) {

Given you special-case for the week view, could you move this into calendar-views.xml's week view binding? I've done this in the unbitrot patch I'll attach in a moment, please revert if done for a reason.

::: calendar/base/themes/pinstripe/calendar-views.css
@@ +249,5 @@
>      text-align: center;
>      overflow: hidden;
>  }
>  
> +.timeIndicator {

I think we should add a 2px border-radius only on the left side of this box, it looks much slicker that way :)

@@ +255,5 @@
> +    background-color: red;
> +    opacity: 0.5;
> +}
> +
> +.timeIndicator[show="true"][orient="vertical"] {

Instead of adding a "show" attribute, you could consider flipping it to be "hidden", then you don't need extra rules to hide the timer since XUL css already defines display: none for [hidden] being set.
Attachment #497135 - Flags: review?(philipp) → review+
Created attachment 589164 [details] [diff] [review]
patch - v2.2
Attachment #497135 - Attachment is obsolete: true
Attachment #589164 - Flags: review+
If we get this in before the end of the month, I could imagine pushing it to comm-aurora so it can be part of Lightning 1.3. This would fit well with the fact that we have a few UI improvements in 1.3 and Thunderbird wants Lightning as part of the default addon set for their recommendations tab.
(Assignee)

Comment 37

7 years ago
Created attachment 594309 [details] [diff] [review]
patch - v3

(In reply to Philipp Kewisch [:Fallen] from comment #34)

> Correct, flex won't work for absolute positioning since its then not
> following the normal XUL box model but doing things differently. I think you
> can use the width: attribute to make up for it (i.e width: 100% or such). If
> it doesn't work thats ok.

I tried with "width:100%" and other solutions as well, but I'm able to make it working only by setting the exact width.
 
> 
> users can change anything in userChrome.css, there should be no limits.
> Given that we are now with a much later Thunderbird, is there anything you
> can improve?

I'm not able to make working the absolute positioning in the view. The behavior is the same I got with TB 3.3: the indicator gets stuck on top of the column and changing the "top" attribute doesn't move it. Changing boxes attributes (in my knowledge) doesn't help. This problem and the previous would need someone with more experience about XUL/XBL.

I've improved the use of the boxes. Instead of three boxes (container, indicator and spacer) now the structure needs only one box (the indicator is the top border), but again the margin-top attribute has to be used.


> Given you special-case for the week view, could you move this into
> calendar-views.xml's week view binding? I've done this in the unbitrot patch

Well done.

> Instead of adding a "show" attribute, you could consider flipping it to be
> "hidden", then you don't need extra rules to hide the timer since XUL css
> already defines display: none for [hidden] being set.

Thanks for pointed that out.


I also changed the first positioning of the indicator after the activation. Now it's more coherent with the interval setting because it gets the position of the multiple of the time interval which comes before the moment of the activation (e.g it get the 13:15 position if the indicator is being activated at 13:22 with a 15 minutes interval).

During the normal functioning, the code executes only few instructions. The function "findColumnForDate()" is the more expensive in terms of time. Since I think the users will set 1 minute as interval for the timer, do you think it's worth trying to find something faster?
Attachment #589164 - Attachment is obsolete: true
Attachment #594309 - Flags: review?(philipp)
Comment on attachment 594309 [details] [diff] [review]
patch - v3

Review of attachment 594309 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Decathlon from comment #37)
> Created attachment 594309 [details] [diff] [review]
> patch - v3
> 
> (In reply to Philipp Kewisch [:Fallen] from comment #34)
> 
> > Correct, flex won't work for absolute positioning since its then not
> > following the normal XUL box model but doing things differently. I think you
> > can use the width: attribute to make up for it (i.e width: 100% or such). If
> > it doesn't work thats ok.
> 
> I tried with "width:100%" and other solutions as well, but I'm able to make
> it working only by setting the exact width.
Ok, lets keep it as it is for now. I wouldn't want to block this bug just because of the width issue.

> I've improved the use of the boxes. Instead of three boxes (container,
> indicator and spacer) now the structure needs only one box (the indicator is
> the top border), but again the margin-top attribute has to be used.
Great!

> During the normal functioning, the code executes only few instructions. The
> function "findColumnForDate()" is the more expensive in terms of time. Since
> I think the users will set 1 minute as interval for the timer, do you think
> it's worth trying to find something faster?
Since there are at most 7 boxes, thats max 7 compare operations, which I think is ok. This should rather be fixed by improving the views data structure in a different bug.

r=philipp. Let me know if you can fix the following, otherwise I'll just check it in as it is.

::: calendar/base/content/calendar-multiday-view.xml
@@ +2566,5 @@
> +      <method name="setTimeForTimeIndicator">
> +        <body><![CDATA[
> +            let now = cal.now();
> +            document.getElementById("day-view").mTimeIndicatorMinutes = now.hour * 60 + now.minute;
> +            document.getElementById("week-view").mTimeIndicatorMinutes = now.hour * 60 + now.minute;

Can you make this part of the particular bindings too?
Attachment #594309 - Flags: review?(philipp) → review+
(Assignee)

Comment 39

7 years ago
Created attachment 594500 [details] [diff] [review]
patch - v4

(In reply to Philipp Kewisch [:Fallen] from comment #38)
 

> Can you make this part of the particular bindings too?

Do you mean like this?
Attachment #594309 - Attachment is obsolete: true
Attachment #594500 - Flags: review?(philipp)
Comment on attachment 594500 [details] [diff] [review]
patch - v4

Nice, r=philipp
Attachment #594500 - Flags: review?(philipp) → review+
I'm going to wait for bug 533096 before checking this in, since it will be much easier to debitrot this one than to go through the common css again and again. If that bug is fixed and I haven't checked in this one, please remind me!
Pushed to comm-central: <http://hg.mozilla.org/comm-central/rev/0312c7227484>
and comm-aurora: <http://hg.mozilla.org/releases/comm-aurora/rev/55ae7a6b32ac>
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4

Comment 43

3 years ago
Why is the "calendar.view.timeIndicatorInterval" still a default of 15? This seems counter-intuitive to its purpose which is to show the user's current progress through the day and the current event. It's worse than having a 60 minute value, because it doesn't snap to any of the grid lines and gives the false impression that it is an accurate measure of your progress.

Also, this is counter to the default functionality in all major calendar applications (Outlook, Google, Apple) which will confuse any users who have migrated to Thunderbird.
(Assignee)

Comment 44

3 years ago
Hi dexgecko,
this bug is fixed because the feature that had been requested here now exists on Lightning and, overall, it works as planned (included the 15 minutes update interval).
To take account of your observations, please open a new bug and specify there what is wrong and what should be changed according to your point of view.
You need to log in before you can comment on or make changes to this bug.