rounding issues for grid boxes in calendar multiday view

VERIFIED FIXED in 0.7

Status

defect
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: thomas.benisch, Assigned: luc.mousseau)

Tracking

unspecified

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

12 years ago
There are some rounding issues for the grid boxes in the
calendar multiday view:

1.) When using a rather high value, e.g. 20 for the
    calendar.view.visiblehours preference 
    (Tools/Options/Views/Show:...hours at a time),
    then depending on the screen resolution and window
    size more than 20 hours are displayed (see attachment #1 [details] [diff] [review]).

2.) When using the maximum value of 24 for the
    calendar.view.visiblehours preference 
    (Tools/Options/Views/Show:...hours at a time),
    then depending on the screen resolution and window
    size there's some unused space at the bottom of the
    week view (see attachment #2 [details] [diff] [review]).
    In addition, there are some vertical grid lines in
    the unused space, which should be removed.

The reason for those rounding issues is due to the fact, that
pixelsPerMinute is rounded to a precision of 0.1.
This makes a precision of 6 pixels (60 minutes x 0.1 ppm) per box
or 144 pixels for 24 hours. That means, that the boxes are only
resized to the next bigger size, when the visible area increases
by 144 pixels. In principal we need only a precision of 1 pixel
per box, that means 24 pixels for 24 hours.

Therefore I suggest the following solution for the mentioned problem:

a) In the onResize method of the calendar-multiday-view binding
   don't round pixelsPerMinute to a precision of 0.1, but to much
   higher precisions, e.g. 0.001 or don't round at all.

b) In all places, where mPixPerMin is used to calculate the size of a
   box, e.g. makeTimeBox(timeString, dur * this.mPixPerMin) round
   the pixels: makeTimeBox(timeString, Math.floor(dur * this.mPixPerMin)).
Reporter

Comment 1

12 years ago
For this screenshot a value of 20 is used for the number
of visible hours, but the view displays 22 hours.
Reporter

Comment 2

12 years ago
For this screenshot the maximum value of 24 is used for the
number of visible hours. There's some unused space at the
bottom of the week view. In addition the vertical grid lines
in the unused space can be seen.
Assignee

Comment 3

12 years ago
Hey, I was bored tonight and thought I'd checkout the latest nightly. Noticed this bug and worked out a patch. Hope you don't mind.
Assignee

Comment 4

12 years ago
Posted patch Patch v1 - Fixes the issue (obsolete) — Splinter Review
As you can see, I used the same chunk of code used in other locations for the relayout method of the calendar-time-bar (ie: startPix, endPix and durPix). I commented out "adjustScrollBarSpacers()" in the onoverflow and onunderflow events of the scrollbox object, I don't think they're necessary anymore. Finally I set the ppm to a precision of 0.001 (though I think 0.01 may be enough, but it probably doesn't hurt).

This patch may possibly have an effect in regards to Bug 362501.
Attachment #261099 - Flags: first-review?(michael.buettner)
Comment on attachment 261099 [details] [diff] [review]
Patch v1 - Fixes the issue

>+                    viewElement.refresh();
Could you please explain why this call here is necessary? I can't see a reason for this...

>+                    viewElement.refresh();
Same question here...

>-                       onoverflow="adjustScrollBarSpacers();" 
>-                       onunderflow="adjustScrollBarSpacers();">
>+                       onoverflow="//adjustScrollBarSpacers();" 
>+                       onunderflow="//adjustScrollBarSpacers();">
You're right, these calls here are no longer necessary. They should have been removed at the same time the 24 hours patch has landed, they're just a left-over. I'd suggest to remove those two lines completely.

Generally, I thought it would be necessary to round all locations were mPixPerMin is used. But I couldn't find any location were this is still missing (bug #353999 is an example for a patch that introduced the necessary rounding). So, the rest of the patch looks fine to me. But while paying close attention to this, I found another problem. Configure the view to always display 24 hours, this makes the scrollbar disappear. Then grab any eventbox (not one of the grabbers, the whole box) and drag it so that the end of the box falls beyond 12pm (don't release the button) and note the scrollbar appearing/disappearing without adjusting the allday-grid at the top of the view. Since you're touching the adjustScrollBarSpacers() location here, please either address this problem in this patch as well or file a follow-up bug (this is up to you).

Marking r- for now.
Attachment #261099 - Flags: first-review?(michael.buettner) → first-review-
Reporter

Updated

12 years ago
Assignee: thomas.benisch → nobody
Assignee

Comment 6

12 years ago
The reason I put in the viewElement.refresh() calls was because I found that the very first time the view is loaded, the size of boxObject.height/width used to calculate the pixelsPerMinute (variable ppm, Lines 1844-1856 in the patch), is wrong because the navLabels aren't initially there, causing the ppm to be slightly off. The result was, for example, with 24 hours displayed, instead of having the full 24 hours in view, you'd be missing part of an hour and you would see the scrollbar. The only way I could find to avoid this was to refresh the view after the navLabels were set. And of course, it needed to be done in both the week and day view, hence both locations.

About scrollbars appearing/disappearing, I hadn't noticed it with the event dragging, but I did notice it when you resize the window from larger to smaller. This was one of the reasons I wasn't sure about removing the onoverflow/onunderflow lines, they might be necessary to avoid this. I'm not sure what the best solution is since as soon as you stop resizing, or drop the event, the scrollbar disappears anyway. In the case of 24 hours being visible only, since it's the only case where you would have no scrollbar, maybe the best solution would be to have no scrollbars appear at all, however I'm not sure how to accomplish that.
Assignee

Comment 7

12 years ago
Adds a fix to prevent the scrollbar from appearing / disappearing when both moving an event past 12pm or when resizing the window to a smaller size.

Actually, the issue of moving events causing the scrollbar to appear / disappear while not adjusting the header, exists in the current nightly, though it's not as obvious because of the issues caused by the rounding issue. You'll notice in the patch that I set the overflow to hidden for a display setting of 24 hour and only for the vertical view.

The horizontal view is another story altogether. There appears to be a bug where resizing the left sidebar does not trigger a resize event (both Lightning and Sunbird appear affected), so setting the overflow to hidden would cause the view to be clipped until refreshed. I'm not really familiar enough with the inner workings to figure out how to detect the sidebar being resized, however that should probably be filed as a separate bug from this one anyway, if it's not already. As a result of this though, the horizontal view isn't refreshed until you manually refresh it. I uncommented the onoverflow and onunderflow, in case they do something to help the horizontal view situation, though not sure they do. Basically the only thing the patch fixes for the horizontal view is the rounding to 1/1000.
Attachment #261099 - Attachment is obsolete: true
Attachment #261357 - Flags: first-review?(michael.buettner)
(In reply to comment #6)
> The reason I put in the viewElement.refresh() calls was because I found that
> the very first time the view is loaded, the size of boxObject.height/width used
> to calculate the pixelsPerMinute (variable ppm, Lines 1844-1856 in the patch),
> is wrong because the navLabels aren't initially there, causing the ppm to be
> slightly off. The result was, for example, with 24 hours displayed, instead of
> having the full 24 hours in view, you'd be missing part of an hour and you
> would see the scrollbar. The only way I could find to avoid this was to refresh
> the view after the navLabels were set. And of course, it needed to be done in
> both the week and day view, hence both locations.
Sorry for not making myself clear. I understood that you're calling relayout() since you need to rely on some calculation that this function performs. What I don't like is that setDateRange() already calls relayout(), just to be called again by the call you would introduce in goToDay(). Since relayout() is a rather expensive function I'd try to avoid any unnecessary calls. A simple solution would be to call setNavLabels() *before* calling setDateRange(). That way all XUL elements are up to date and the calculation of pixels per minute works just fine, plus we avoided two successive calls to relayout(). So, instead of writing:

  viewElement.setDateRange(aDate, aDate);
  viewElement.selectedDay = aDate;
  this.setNavLabels(aDate);
  viewElement.refresh();

reorder the statements like so:

  this.setNavLabels(aDate);
  viewElement.setDateRange(aDate, aDate);
  viewElement.selectedDay = aDate;

While you're on it, there's a semicolon missing at the setNavLabels() line.

> About scrollbars appearing/disappearing, I hadn't noticed it with the event
> dragging, but I did notice it when you resize the window from larger to
> smaller.
That's not the issue I was referring to. Furthermore, this behavior is related to Gecko's internal layout timing. I don't mind if we ignore this for now.

> This was one of the reasons I wasn't sure about removing the
> onoverflow/onunderflow lines, they might be necessary to avoid this.
No, they were introduced before the views have been switched to 24 hours. They are no longer necessary, and you were absolutely right with removing them.

> I'm not sure what the best solution is since as soon as you stop resizing,
> or drop the event, the scrollbar disappears anyway. In the case of 24 hours
> being visible only, since it's the only case where you would have no
> scrollbar, maybe the best solution would be to have no scrollbars appear at
> all, however I'm not sure how to accomplish that.
Don't mind, it's probably best to just file a follow-up bug. It's outside of the scope of this bug as it has nothing to do with the rounding issues. Sorry, for the confusion.
Comment on attachment 261357 [details] [diff] [review]
Patch v2 - Adds fix to prevent scrollbars with 24hr view

>+                    viewElement.refresh();
Please see my comment #8 above and reorder the statements as suggested.

>+                    viewElement.refresh();
Same applies here.

>+          var timebar = document.getAnonymousElementByAttribute(this, "anonid", "timebar");
>+
>+          if(this.mVisibleMinutes == 1440 && orient == "vertical") {
>+              daybox.setAttribute("style", "overflow: hidden;");
>+              timebar.setAttribute("style", "overflow: hidden;");
>+          } else {
>+              daybox.setAttribute("style", "");
>+              timebar.setAttribute("style", "");
>+          }
Please remove this and file a follow-up bug for the problems related to resizing. Furthermore, this is not the problem I was initially referring to and it is really outside the scope of this bug. Just as a side-note, in order to get rid of an attribute, it is almost always better to use removeAttribute().

The rest of the patch looks fine to me... ;-)
Attachment #261357 - Flags: first-review?(michael.buettner) → first-review-
Assignee

Comment 10

12 years ago
Sorry, I didn't think of looking in the setDateRange function, I guess I should have looked a little deeper, chalk it up to rookie mistake :)

I'll submit a follow up bug for the scrollbar issue with a patch as I had it here.
Attachment #261357 - Attachment is obsolete: true
Attachment #261524 - Flags: first-review?(michael.buettner)
Comment on attachment 261524 [details] [diff] [review]
Patch v3 - Removed previous scrollbar fix and corrected issues as directed

Thanks for the patch and your patience ;-) Good job...
Attachment #261524 - Flags: first-review?(michael.buettner) → first-review+
This probably wants to wait until 0.5 has been shipped. At least, I wouldn't want to land it earlier.
Whiteboard: [needs checkin]
Whiteboard: [needs checkin] → [checkin needed after 0.5]

Updated

12 years ago
Assignee: nobody → luc.mousseau
Checked in on HEAD and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Last Resolved: 12 years ago
OS: Windows Server 2003 → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [checkin needed after 0.5]
Target Milestone: --- → 0.7
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6pre) Gecko/20070726 Calendar/0.7pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.