Closed Bug 437412 Opened 16 years ago Closed 16 years ago

zero-length and short events are shown at the wrong times in the timegrid

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: andreas.treumann, Assigned: berend.cornelius09)

References

Details

(Keywords: regression)

Attachments

(6 files, 5 obsolete files)

STEPS TO REPRODUCE:
===================

- create an event start time 8.00 AM , end time 8:00 AM
- create an second event start time 8.30 AM , end time 8:30 AM
- check the calendar views -> looks okay
- create an third event star time 8:00 AM, end time 9:00 AM
- check the calendar view (dayview/weekview)

RESULT:
=======

- the second event jumps to a wrong place in the time grid

EXPECTED RESULT:
================

- events should be displayed correctly

REPRODUCIBLE:
=============

- always
Summary: zero-lenght events are sometimes not in sync with the timegrid → zero-length and short events are sometimes not in sync with the timegrid
Flags: blocking-calendar0.9?
Flags: blocking-calendar0.9? → blocking-calendar0.9+
Assignee: nobody → Berend.Cornelius
Blocks: 387402
Keywords: regression
Summary: zero-length and short events are sometimes not in sync with the timegrid → zero-length and short events are shown at the wrong times in the timegrid
Attached patch patch v. #1 (obsolete) — Splinter Review
This patch basically solves the problem and hopefully all other issues around overlapping event boxes. However I noticed one serious side effect that should not be ignored: When the mouse hovers over an event with small width the grippys are shown (as usually) and the event-box starts to change its width for the time the mouse pointer resides over the box (not as usually). This should not be but I am ashamed to say that I just could not figure out how to resolve this problem. 
Philipp - (or anybody else), could you have a look at this?
I am aware that it's formal wise not correct to ask for a review already at this state, but I am hoping that there is just another uncritical thing to add to this patch.
Attachment #331732 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Comment on attachment 331732 [details] [diff] [review]
patch v. #1

>+          var configBox = document.getAnonymousElementByAttribute(this, "anonid", "config-box");
>+          var minSize = configBox.getOptimalMinSize();
>+//          configBox.setAttribute("orient", orient);
>+//          var minSize = 25; //configBox.getOptimalMinSize();
>+          this.mMinDuration = Components.classes["@mozilla.org/calendar/duration;1"]
>+                                            .createInstance(Components.interfaces.calIDuration);
>+          this.mMinDuration.minutes = parseInt(minSize/this.mPixPerMin);
Either remove or annotate commented code.


>-                          var chunkBox = createXULElement("calendar-event-box");
>+                          var chunkBox = configBox.cloneNode(false);
>+//                          var chunkBox = createXULElement("calendar-event-box");
I'm not a fan of cloning nodes, but in any case remove the commented code

>+
>+
>+function getOtherOrientation(aOrientation) {
>+   if (aOrientation != null) {
>+       if (aOrientation == "vertical") {
>+          return "horizontal";
>+       } else {
>+          return "vertical";
>+       }
>+   } else {
>+      return "vertical";
>+   }
>+}
It should be enough to do 
       return (aOrientation == "vertical" ? "horizontal" : "vertical");
Also, redundant newlines above.

r=philipp
Attachment #331732 - Flags: review?(philipp) → review+
Andreas tested the last patch this morning and found one bug that I fixed with this patch. I also addressed Philipp's comments in the new patch. Still the layout problem remains. As far as I see this patch fixes all issues regarding overlapping items. If Philipp does not succeed to solve the layout issue it is probably an option to remove the rules for the grippy images from the css file temporarily.
Attachment #331732 - Attachment is obsolete: true
Attachment #331842 - Flags: review?(philipp)
Attached image screnn shot
Checked patch v. #2:

- no drag&drop shadow in the day view
- resizing the horizontal width of the calendar view -> some boxes overlap and don't use the complete horizontal space (see screen shot) 
The first point is wrong. The d&d shadow is visible, but a very long time is needed to get the shadow.
I used a second PC and it's not possible for me to reproduce the performance issue.

Only the screen shot issue is valid!
 
Concerning the performance I still see room for improvement as "getlayoutEnd" is currently called various times in a loop and instead "layoutEnd" could be kept in a member variable. I tried the recent days to resolve the remaining problem with the dancing events on mouseover but I was not successfull.
Maybe it's an option to remove the rules 

calendar-event-box[orient="vertical"]:hover .calendar-event-box-grippy-top {
    list-style-image: url("chrome://calendar/skin/event-grippy-top.png");
}

calendar-event-box[orient="vertical"]:hover .calendar-event-box-grippy-bottom {
    list-style-image: url("chrome://calendar/skin/event-grippy-bottom.png");
}

calendar-event-box[orient="horizontal"]:hover .calendar-event-box-grippy-top {
    list-style-image: url("chrome://calendar/skin/event-grippy-left.png");
}

calendar-event-box[orient="horizontal"]:hover .calendar-event-box-grippy-bottom {
    list-style-image: url("chrome://calendar/skin/event-grippy-right.png");
}
temporarily until we find a solution.
Comment on attachment 331842 [details] [diff] [review]
[checked in] patch v. #2

>                           var chunkBox = createXULElement("calendar-event-box");
>+//                          var chunkBox = createXULElement("calendar-event-box");
Remove comment



r=philipp
Attachment #331842 - Flags: review?(philipp) → review+
Attachment #331842 - Attachment description: patch v. #2 → [checked in] patch v. #2
Patch checked in, keeping open to fix the hover problem.
Assignee: Berend.Cornelius → philipp
Status: ASSIGNED → NEW
Day and week view are empty in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.2pre) Gecko/2008080918 Calendar/0.6a1. Error Console shows:

    Error: syntax error
    Source File: chrome://calendar/content/calendar-multiday-view.xml
    Line: 1111, Column: 14
    Source Code:
              }

    Error: aDayBox.startLayoutBatchChange is not a function
    Source File: chrome://calendar/content/calendar-multiday-view.xml
    Line: 3242
I see also this error. With removing this bracket at line 1111, the Day and Week views works again.
Whiteboard: [needs patch]
Checked in a bustage-fix type patch to remove the bracket. Thanks for testing.
Although the remaining issue is a bug, I'd almost call it a feature ;-) Its kind of nice that when you hover over an event you get to see more of it. Christian what do you think?
I'd prefer a different "solution". This "feature" is from an ux perspective more or less a bug. The jumping events are not nice, even if they are easer to select.
We should turn back this change.

Is the overlapping event caused by the change? See attachment.
To be honest, the only difference I see is that the boxes of the old build seem to be wider (you can read more of the text).
In the screenshot of commment #18 can be seen that actually the eventbox in the bottom right corner has the right width (2/3 of the width of the column). The other three eventboxes are supposed to have equal width (each 1/3 of the column width) but they actually differ. The first one with the category consumes more space than it is actually allowed to have. 
All these 3 boxes are on one layer within a stack element whereas the fourth eventbox is wihin another layer. Within each layer open space is filled with xul:spacer elements. So what I tried to do (among many other things) is to set "equalsize" to the parental box of the three eventboxes to enforce equal width - but without success and I don't know why. I admit that I ran out of ideas on how to solve this issue and I am thankful for any assistance. 
I could not reproduce the bug scenario with eventboxes without category. Unfortunately the behaviour is not any better on trunk as I tested so we somehow won't get around to deal with this problem.
Attached patch patch v. #3 (obsolete) — Splinter Review
This patch should resolve the problem with the dancing events on mouse hover. By setting the "visibility" attribute instead of "display" I take care the grippies don't lose their size when made invisible.
Yet I still don't have a solution for the wider category-eventboxes.
Assignee: philipp → Berend.Cornelius
Status: NEW → ASSIGNED
Attachment #334761 - Flags: review?(philipp)
Attached patch patch v. #4 (obsolete) — Splinter Review
I think I found a solution for the eventboxes containing categories, too and extended the patch v. #3 by this functionality: I moved the category child element into the stack element of the event box where actually all other anonymous elements also reside. One side effect is that now the title is also using the space of the category box which I find very positive although admittedly the category box has another background than the rest of the event-box. We currently use the space of the category box for nothing else than the display of the category color and I have always found this is a waste of space.
Attachment #334761 - Attachment is obsolete: true
Attachment #334777 - Flags: ui-review?(daniel.boelzle)
Attachment #334777 - Flags: review?(philipp)
Attachment #334761 - Flags: review?(philipp)
As can be seen in this screenshot the title of the category-event now also uses the space of the category-box.
Comment on attachment 334777 [details] [diff] [review]
patch v. #4

The alarm bell overlays on the category space which IMO looks odd. I don't feel like the right person to judge on this.
Attachment #334777 - Flags: ui-review?(daniel.boelzle)
Attachment #334777 - Flags: ui-review-
Attachment #334777 - Flags: review?(philipp)
Attachment #334777 - Flags: review?(christian.jansen)
Comment on attachment 334777 [details] [diff] [review]
patch v. #4

Oops, I should get some sleep.
Attachment #334777 - Flags: ui-review?(christian.jansen)
Attachment #334777 - Flags: ui-review-
Attachment #334777 - Flags: review?(philipp)
Attachment #334777 - Flags: review?(christian.jansen)
Attached patch patch v. #5 (obsolete) — Splinter Review
I tried to address Daniel's concerns and put the alarm-image left to the category-box. To my dismay I observed that the problem with the colliding events returned. As I found out the culprit is the alarm image that consumes the space that it needs similarily to the category-box beforehand. The patch attached does not solve the problem but merely contains the current state of the art and thus contains some codelines that still need a clean up.
I still keep asking for assistance!
I talked to Daniel about it: If we don't come with a proper solution I will and extend the alarm.pngs to the right by the width of the category-box with a transparent background. I know that this is an ugly hack of course that should only be temporary.
>If we don't come with a proper solution I will and
>extend the alarm.pngs to the right by the width of the category-box with a
>transparent background.

This is not a solution because the alarm images do not always turn up in combination with a category.
Berend, are you uploading a new version of this patch today? If you have any further problems, feel free to give me a call.
Attachment #334777 - Flags: review?(philipp)
Attached patch patch v. #6 (obsolete) — Splinter Review
I added the functionality as desired and hope I got it all straight now. I have to send special thanks to philipp who help to spot the reason why the subelements of the eventboxes consumed too much space (It was the width of the gradient that for some reason was set to 1px).
Attachment #334898 - Attachment is obsolete: true
Attachment #335097 - Flags: ui-review?(daniel.boelzle)
Attachment #335097 - Flags: review?(philipp)
As I investigated the source code in question that caused the problem was a deprecated relict and was checked in by
Bug 402683 – new category layout causes colliding event boxes
Comment on attachment 335097 [details] [diff] [review]
patch v. #6

This patch makes the gripbar be larger than it is thought to be. It looks a bit stretched.
Attachment #335097 - Flags: ui-review?(daniel.boelzle)
Attachment #335097 - Flags: review?(philipp)
Attachment #335097 - Flags: review-
Attached patch patch v. #7Splinter Review
This patch takes care, the only thing I really changed is adding

xbl:inherts="align=whichside"

to the grippy's "thebox".

I also took the liberty of removing crufty code from the gripbar. berend, if this version is r+ for you, please go ahead and check it in.
Attachment #334777 - Attachment is obsolete: true
Attachment #335097 - Attachment is obsolete: true
Attachment #335167 - Flags: review?(Berend.Cornelius)
Attachment #334777 - Flags: ui-review?(christian.jansen)
Whiteboard: [needs patch] → [needs review]
Blocks: 439309
Blocks: 437347
Comment on attachment 335167 [details] [diff] [review]
patch v. #7

patch looks good; r=berend
Attachment #335167 - Flags: review?(Berend.Cornelius) → review+
Attachment #335167 - Flags: ui-review?(daniel.boelzle)
Whiteboard: [needs review]
Attachment #335167 - Flags: ui-review?(daniel.boelzle) → ui-review+
Thanks Philipp for you latest changes. I checked in the patch on trunk and MOZILLA_1_8_BRANCH.
<I also took the liberty of removing crufty code from the gripbar. berend, if
<this version is r+ for you, please go ahead and check it in.

I also found the code quite hard to understand and think we should consolidate this after the next release has landed. It also makes sense to split up the file ./base/content/calendar-multiday-view in several other files.

bug is fixed.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
Checked in lightning 2008082620 and Sunbird 20080826 -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.