Closed Bug 304741 (warOnBoxes) Opened 15 years ago Closed 13 years ago

non-colliding events too narrow on days with colliding events

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dmose, Assigned: michael.buettner)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [roadmap 0.7])

Attachments

(7 files, 11 obsolete files)

5.39 KB, text/plain
Details
706 bytes, application/vnd.mozilla.xul+xml
Details
23.29 KB, text/calendar
Details
57.60 KB, image/jpeg
Details
41.81 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
38.48 KB, image/jpeg
Details
4.89 KB, application/octet-stream
Details
In the new week and day views, on a day where there are multiple events at the
same, all events end up with a width of 1 / max(simultaneous events).
Vlad: when we were talking about this last week, my understanding was that you
thought it'd require changes to XUL itself in order to be able to fix it.  If
that's the case, this would seem to suggest that we won't be able to have
Lightning that's shippable to users against TBird 1.5.  What specific changes
did you have in mind?  It could also be helpful to know the reasoning for making
the new views use a different strategy than the 0.2 views, which seem to mostly
handle this case correctly.
The one solution I came up with was to put boxes in a stack; let's say you have
3 conflicting events. You'd have one hbox that had 3 vboxes, then on top of that
another hbox that had 2, and on top of that a hbox with a single vbox.  You'd
use the appropriate depth depending on how many other events that event was in
conflict with, to have it "stretch" across the right number of columns.  I ran
into problems with event handling, where transparent empty boxes were eating
mose events, so you couldn't click on any events etc.  Noone seemed to know a
good solution, and the general opinion was "<stack> sucks, don't use it" with no
good alternative to doing something like this.
Blocks: 321164
Unfortunately, I do not have cycles to work on Calendar stuff these days (just as it's getting to the good part!), so I am a bad owner for these bugs.  To delete the tragically-large chunk of bugspam, search for gregorianabdication.
Assignee: shaver → nobody
QA Contact: shaver → lightning
bsmedberg suggested that using extra vboxes inside the <stack> might help avoid the problems vlad was describing.
Alias: warOnBoxes
Component: Lightning → Base
Blocks: 322979
Blocks: 322981
*** Bug 330389 has been marked as a duplicate of this bug. ***
*** Bug 336384 has been marked as a duplicate of this bug. ***
Assignee: nobody → base
QA Contact: lightning → base
Attached patch work in progress (obsolete) — Splinter Review
Work-in-progress patch.  Works fairly well for most of the basic-conflict cases.  I'm hoping some of our QA folk can attach some ICS testcases that include more pathological conflicts.  mvl has also suggested trying to remove the rare case where we hard-code the width, so that we don't need to relayout as often.

Main work to still be done:
-The layers algorithm still isn't correct.  Need to study Cantor to get that right I think.  (f:N^2 -> N)
Assignee: base → jminta
Status: NEW → ASSIGNED
Attached patch for testing (obsolete) — Splinter Review
This patch *should* have all the layout issues worked out.  We still have to hard-code a couple of widths, and the resize-handler isn't working yet, but I wanted to put this here for people to test and make sure i'm right that we've got what we need here.
Attachment #224441 - Attachment is obsolete: true
Attached patch proper layout v1 (obsolete) — Splinter Review
This patch should win the majority of the war on boxes.  I wasn't able to completely avoid specifying exact widths, but I've reduced that case to an absolute minimum.  Also, in contrast to the old code, rather than relaying out on resize, we're able to simply resize the boxes that need it.  This cuts down on the 'sluggish' behavior that mvl disliked about the old layout code.

Because this code is fairly complex, I've tried to be as liberal as possible with comments, so that it's easy to maintain.  However, having lived in it for a few weeks now, I may not have explained parts that I should have.  Please point out any of those areas so I can add additional documentation.  I'd also like to encourage our qa folk to continue testing the iterations of this patch for any cases where it fails to properly layout events.  Testcases strongly encouraged.

Note: I'd like to keep the debug code in there, if possible, since I expect once the code makes it into nightly builds, we'll have a bit of fallout that this will help us debug.  I'll attach debug.js, which includes the necessary functions, for anyone else who might want to use it.
Attachment #225507 - Flags: first-review?(dmose)
Attached file my debug.js
File defines jDebug and its associated functions.  Add it to calendar.jar, and edit messanger-overlay-sidebar.xul or calendar.xul to include it.  (The necessary script tag is included at the top of debug.js.)
Blocks: 279268
Flags: blocking0.3+
Keywords: regression
Comment on attachment 225507 [details] [diff] [review]
proper layout v1

aww crap, i'm seeing some mouse-event-eating with this patch now.
Attachment #225507 - Attachment is obsolete: true
Attachment #225507 - Flags: first-review?(dmose)
Comment on attachment 225507 [details] [diff] [review]
proper layout v1

>+                                           colEndArray[lastCol].dueDate;
>+                          // If the next column's item ends after we start, we
>+                          // can't expand any further
>+                          if (nextColEnd.compare(itemStart) == 1) {
>                               break;
>                           }
>+                          lastCol++;
Need colEndArray[lastCol] = item here

>+                              var spanOfShrunkItem = currentBlob[kk].colSpan;
>+                              currentBlob.push({item: item, 
>+                                                startCol: currentBlob[kk].startCol + 1,
>+                                                colSpan: spanOfShrunkItem -1});
Need Number(currentBlob[kk].startCol) here
Whiteboard: [swag:2d]
Whiteboard: [swag:2d] → [swag:2d][high risk]
Attached patch updated patch (obsolete) — Splinter Review
After talking with sspitzer about some similar <stack> bugs he was seeing, we convinced some of the layout guys to poke at it.  Some fixes have landed and I can no longer reproduce the click-eating problems previously mentioned.  This is an updated patch that should apply to current CVS.  It will conflict a moderate amount with the selection patch I attached earlier.

I've gone ahead and create local copies of this diff that keeps my jDebug stuff around.
Attachment #224481 - Attachment is obsolete: true
Attachment #233893 - Flags: second-review?(dmose)
Attachment #233893 - Flags: first-review?(mvl)
Whiteboard: [swag:2d][high risk] → [swag:2d][high risk][patch in hand]
Unfortuantely, we've got enough other big patches that we absolutely need to take that we should probably punt on this.
Flags: blocking0.3+ → blocking0.3-
Comment on attachment 242343 [details] [diff] [review]
more up to date merge (untested)

This patch also backs out all changed made after the previous patch. That's not good.
Attachment #242343 - Attachment is obsolete: true
Attached patch merged patch (obsolete) — Splinter Review
Patch updated the hard way: by hand.

I tested the patch, but i can't select any items when there is more then one column. So still some click-eating problems. But the clicks are not really gone: when trying to drag an event, i create a new event instead.
After poking the click issue a bit more, it seems the problem is not that clicks get eaten. The problem is that the eventboxes don't see them, and the click arrive at the day column, starting the drag to create a new event.
The problem already shows when you have to event, not overlapping. You get two items in the stack, and only the last item will see the clicks.
I know sspitzer saw some similar behavior when he was playing around with <stack>s for improving the tab interface of Firefox.  I think the solution was to add some position: relative bits to the elements, forcing gecko to assign them their own views.
Attached file sample xul
Attachement is a stand-alone xul file.
After playing with it, it is not surprising that the proposed solution doesn't work: on top of the event ('a' in the testcase) there is a spacer. The spacer doesn't eat the event, so the containing element gets the click: the stack.

One idea I had to fix this was to use the positioning features of the stack, but that would make give the element it's minimal size, instead of extending. And from looking at the c++ source of the stack, that's not fixable in xul. (We could try to change xul itself, but that won't be easily portable to the 1.8 branch)
Attached patch updated again (obsolete) — Splinter Review
updated the patch once again to trunk. Just so that i can remove the code from my tree and still have a patch somewhere...
Attachment #245331 - Attachment is obsolete: true
Comment on attachment 233893 [details] [diff] [review]
updated patch

this patch is obsolete, it doesn't apply anymore.
And when updated, it doesn't work yet. click-eating problems.
Attachment #233893 - Flags: second-review?(dmose)
Attachment #233893 - Flags: first-review?(mvl)
Attachment #233893 - Flags: first-review-
Duplicate of this bug: 377712
Blocks: 380810
Since this bug is on the roadmap for 0.7 (see [1] for details) and presumably nobody else will take care of this one, I'm going ahead and assign it to me.

[1] http://wiki.mozilla.org/Calendar:Roadmap
Assignee: jminta → michael.buettner
Status: ASSIGNED → NEW
Duplicate of this bug: 383575
Attached patch final patch v1 (obsolete) — Splinter Review
This version of the patch resolves all the flaws the previous versions contained. First, there's no click-eating (at least on branch, I didn't try on trunk), even with loads of different layers and complex layouts. Second, several bugs have been removed. The most important among them belongs to fitting boxes into existing columns while shrinking already placed boxes. Another problem was the optimal arrangement of layers in case a chunk of items needs to split across different layers. This happens if we encounter items covering more than a single span in a layer. All these problems have been eliminated.

As an example, this is one of the layouts that can be achieved with this patch:

       |______|      |      |      |
       |ev1   |      |      |      |
       |      |______|______|      |
       |      |ev2   |ev3   |      |
       |      |      |      |______|
       |      |      |      |ev4   |
       |______|      |      |      |
       |      |      |      |      |
       |      |______|______|      |
       |____________________|      |
       |ev5                 |      |
       |                    |      |
       |____________________|      |
       |      |      |      |      |
       |______|      |      |      |
       |ev6   |      |      |      |
       |      |_____________|      |
       |______|ev7          |      |
       |      |             |      |
       |      |_____________|      |
       |      |      |      |      |
       |      |      |      |______|

Please note how event5 and event7 span across the otherwise conflicting columns.
Attachment #233893 - Attachment is obsolete: true
Attachment #245901 - Attachment is obsolete: true
Attachment #268600 - Flags: review?(daniel.boelzle)
Comment on attachment 268600 [details] [diff] [review]
final patch v1

Great work! During testing with new created events it really works fine. 

However, it seems there is a problem with events in non-default timezones. I imported an .ics file from one of our timezone testdays into a storage calendar, restarted Sunbird and this is the result:

Sunbird 0.5 (20070616) MOZILLA_1_8_BRANCH build without patch: http://img256.imageshack.us/my.php?image=tz1withoutpatchvg9.png

Sunbird 0.5 (20070616) MOZILLA_1_8_BRANCH build with patch: http://img256.imageshack.us/my.php?image=tz2withpatchdq7.png

Without the patch the events are displayed mapped to default timezone (Europe/Berlin). Without the patch the events are all shown on the same time without respect to timezone and stacked.
Attached patch final patch v2 (obsolete) — Splinter Review
fixed the issue stefan found in the previous iteration (thanks for trying out this patch). moving review over to philipp, i don't want to choke daniel's review queue.
Attachment #268600 - Attachment is obsolete: true
Attachment #268946 - Flags: review?(bugzilla)
Attachment #268600 - Flags: review?(daniel.boelzle)
Comment on attachment 268946 [details] [diff] [review]
final patch v2

It seems there are still some problems with this patch, therefore I'll just address some style nits and small changes.

The following happens on sunbird branch (and probably others):

1. create an event in any timeslot
2. create a second event that overlaps that timeslot, that has a long text

Actual Results:
The first event box gets very small or even disappears totally.
The text of the event box with the long text doesn't wrap. Its overflow is hidden (this is probably worth a spinoff bug, its happens without this patch too)

>-      <method name="findChunkForOccurrence">
why get rid of findChunkForOccurrence? The code in selectOccurrence and unselectOccurrence both can use a such method.

>       <method name="selectOccurrence">
...
>+              for each (var chunk in this.mEventBoxes) {
>+                  if (chunk.occurrence.hasSameIds(aOccurrence)) {
>+                      this.mCurrentSelection = chunk;
>+                      chunk.selected = true;
>+                      break;
>+                  }
You probably meant to use |this.mSelectedChunks.push(chunk);| When fixing this, make sure that unselectOccurrence will find the event, since indexOf needs a member that is exactly the same (especially chunk.selected)

>       <method name="unselectOccurrence">
>+              for each (var chunk in this.mEventBoxes) {
>+                  if (chunk.occurrence.hasSameIds(aOccurrence)) {
>+                      chunk.selected = false;
>+                      var index = this.mSelectedChunks.indexOf(chunk);
>+                      this.mSelectedChunks.splice(index, 1);
>+                      break;
>+                  }
If the chunk was deleted while it was selected (internalDeleteEvent), indexOf will not find the event. Calling splice(-1, 1) will remove one element from the array.

>+              for each (var column in layer) {
>+                  if (column.length == null || column.length == undefined) {
>+                      continue;
>+                  }
A comment why you are skipping the colum if its length is null or undefined but not if its 0 would be nice.

>+                      var pixSpan = column.specialSpan*this.topbox.boxObject.width;
>+                      if (orient == "vertical") {
>+                          style += "max-width: "+pixSpan+"px;";
>+                      } else {
>+                          style += "max-height: "+pixSpan+"px;";
>                       }
Style nit: Please insert spaces before and after operators like +-*/% (throughout the patch)

>+                          chunkBox.calendarView = this.calendarView;
>+                          chunkBox.occurrence = chunk.event;
>+                          chunkBox.parentColumn = this;
>+
>+                          this.mEventBoxes.push(chunkBox);
>+
>+                          if (this.mEventToEdit && 
Redundant whitespaces here and elsewhere
    
>+              for (var jj=0; jj<colEndArray.length; ++jj) {
>+                  if (jj == 0) {
>+                      // We're going to compare with the previous col (jj-1)
>+                      // which won't work nicely for the 0 case.
>+                      continue;
>+                  }
Why not start with for (var jj=1; ... ) ?

>+              currentBlob.push({item: item, startCol: colEndArray.length, 
>+                               colSpan: 1});
Style nit: if you span objects over mutiple lines, then one attribute per line

>+          //var test = "layers:\n";
>+          //for each (glob in blobs) {
>+              //test += " totalCols: "+glob.totalCols+"\n";
>+              //for each (data in glob.blob) {
>+                //test += "  --> "+data.item.title+"\n";
>+                //test += "  startCol:"+data.startCol+"\n";
>+                //test += "  colSpan:"+data.colSpan+"\n";
>+              //}
>+          //}
>+
>+          //var cs = Components.classes["@mozilla.org/consoleservice;1"].getService(Components.interfaces.nsIConsoleService);
>+          //cs.logStringMessage(test);
left over debug stuff

>+          for each (glob in aBlobs) {
Missing "var" here and in other for each() loops.

>+                  // Make sure there's room to insert stuff
>+                  while (layerIndex >= layers.length) {
>+                      layers.push([]);
>+                  }
>+                  
>+                  while(data.startCol >= layers[layerIndex].length) {
    Missing blank

>+                  var start = data.item.startDate || data.item.entryDate;
>+                  if (start.timezone != this.mTimezone) {
>+                      start = start.getInTimezone (this.mTimezone);
>+                  }
Redundant blank before arguments (happens again shortly after)
Attachment #268946 - Flags: review?(bugzilla) → review-
Duplicate of this bug: 385064
Another Scenario that is problematic. This, just like the issue above happens
at least in week view. Create a couple events like so:

|______|______|______|
|ev1   |ev3   |ev4   |
|      |      |      |
|      |______|______|
|      |ev2          |
|      |             |
|      |             |
|______|             |
|      |_____________|
|      |      |      |

Now click ev1 twice to trigger the inline edit. The gradient shadow from ev1
now overlaps ev2.

BTW: I changed my bugzilla email to philipp@bugzilla.kewis.ch in hope that its
enough to just type "philipp" in the requestee box :)
Flags: blocking-calendar0.7+
Whiteboard: [swag:2d][high risk][patch in hand] → [swag:2d][high risk][patch in hand][roadmap 0.7]
Whiteboard: [swag:2d][high risk][patch in hand][roadmap 0.7] → [patch in hand][roadmap 0.7]
Duplicate of this bug: 388474
Attached patch final patch v3 (obsolete) — Splinter Review
I finally found a way to get this working. Both problems that were found are caused by the same underlying problem. The core of the event boxes consists out of the following structure:

<vbox>
  <description/>
  <textbox/>
</vbox>

Both elements (description as well as the textbox) extend beyond their parent elements. The description element makes the event box wider than it is allowed in case the description.value is some text that doesn't fit and the textbox (which gets activated during inline editing) has a similar flaw. I found that forcing a width of 0px makes them behave correctly. Admittedly, I can't explain why it works, but it works like a charm. The element starts out with a css rule that forces a specific width (0px in this case) but behaves like a normal element that has a flex="1" attribute. If anybody knows why it works, please shed some light in here.

Furthermore, I addressed all the bits and pieces from the previous review.
Attachment #268946 - Attachment is obsolete: true
Attachment #273755 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Whiteboard: [patch in hand][roadmap 0.7] → [roadmap 0.7]
Blocks: 199732
Attached file Testcases
I'm sorry to say, but there are still some issues. They usually show up when there are many events. I am attaching a calendar that has some testcases. See Weeks 25 and 31.

Jun 20, Jun 22, Jun 23, Jul 31 - Events overlap
Aug 1  - Extra space on 8:30-8:45 Event to the right. The three events (8:15 - 8:45 twice, 8:30-8:45) should share the space each to a third if possible.
Aug 2  - Minor: 7:45 event could have more room on the right

Some days don't have anything specific and are just leftovers from testing. I also noticed that the events don't align at the bottom, off by 1-2px.

I promise I'll be a bit quicker on the next review so we can get this stuff fixed.
Attachment #273755 - Flags: review?(philipp) → review-
Also there are some paint issues, that I think need to be taken care of, maybe in a followup bug:

Aug 1 - 8:30 Event: Edit the event inline, press enter. The event expands to 8:15 for a moment and then goes back to the same size.

In general, there is also a delay on resize, I'm not sure its easy to avoid this though.
Whiteboard: [roadmap 0.7] → [roadmap 0.7][patch in hand]
>+.calendar-event-details > * {
>+  width: 0px;
>+  margin: 0px;
>+}

This will also cause a regression (and problems for the alarm indicator patch), since in the month view, the alarm icon and <xul:image anonid="item-icon"/> will also be reduced to 0px width.

Also, * is a very general selector and should be avoided. Selectors work from bottom up, so *every* tag needs to be checked if its parent item has the given class. Try to be more specific.
Attached image showcase v1
argh, the layout problems were a simple off-by-one bug. the line
 ...for (var ll = jj; ll < jj + spanOfShrunkItem; ll++)...
should read
 ...for (var ll = jj; ll < jj + spanOfShrunkItem - 1; ll++)...
as this is responsible for shrinking previously placed items in case later added items need their column. this off-by-one bug resulted in ghost columns to be added which screwed up the end result. see the screen shot for how the testcase file now looks like...
Attached patch final patch v4 (obsolete) — Splinter Review
See the above posted screen shot for how the layout now looks like even in extreme situations. I also changed the css selector to be more specific. Regarding the 'paint issues', this also happens without this patch. As I consider this to be out of scope for this patch I suggest to file a separate bug on that.
Attachment #273755 - Attachment is obsolete: true
Attachment #277097 - Flags: review?(philipp)
Create an event that is not all-day, but spans more than one day. Result: The event looks like two events, that only span the difference in time of day. This worked before the patch and should obviously.
Comment on attachment 277097 [details] [diff] [review]
final patch v4

This is totally minor and can also go into a different bug, but to be pixel-perfect, the event boxes need to be aligned if they end at the same time. If you take a close look at the screenshot, then you see that on the last day , the second event from the left is a couple of pixels longer than the others to the right of it.

Also you should remove the normalize() call that causes the error console message and remove all the trailing whitespaces that are in the files.

r- for now based on comment 39.
Attachment #277097 - Flags: review?(philipp) → review-
Attached patch final patch v5Splinter Review
Admittedly, I didn't take events into account that span more than a single day. Thanks for spotting this. I also removed the call to normalize() which is no longer necessary.

Regarding pixel-perfection, I feel that this patch already bends xul to its extremes. I don't have much control on the final visual result and rounding errors are simply out of reach. I still feel that it would have been worth while to implement the views in SVG or utilize the canvas element, but as long as these features are not enabled we need to stick to plain xul and its drawbacks.
Attachment #277097 - Attachment is obsolete: true
Attachment #277708 - Flags: review?(philipp)
Redoing the views in SVG someday would indeed be nice.

As far as pixel rounding errors: I don't know if this approach would work here, but one way that I've successfully dealt with that problem in the past is by having a float accumulator that tracks the exact rounding errors when they happen, and whenever abs(accumulator) > 1.0, you add or subtract a padding pixel as appropriate.  The code I wrote to do that is in the writeInterestSlices function in <http://redpuma.net/longview/tree/longview.py>.
Comment on attachment 277708 [details] [diff] [review]
final patch v5

Nice work! I think we finally made it. Please check files for trailing whitespaces though, there were a bunch. I didn't look through the bug since its pretty late now, but if there are any non-filed followup bugs, please file them :)

r=philipp
Attachment #277708 - Flags: review?(philipp) → review+
Nice work, but this patch looks like it regresses the pixel rounding code that was in the previous version.  The principle is easy to understand (see bug 353999 comment 4) and apply (see short patch for that bug in attachment 239893 [details] [diff] [review]).
Patch checked in on HEAD and MOZILLA_1_8_BRANCH.

gekacheka, if this indeed regresses bug 353999, then please file a new regression bug. The patch in this bug however needs to get in, so that the new code can be thoroughly tested on the upcoming testday on Tuesday.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [roadmap 0.7][patch in hand] → [roadmap 0.7]
Target Milestone: --- → 0.7
Version: Trunk → unspecified
Hi! There is the description error in http://bonsai.mozilla.org/cvsquery.cgi?branch=HEAD&file=mozilla/calendar/&date=month
Bug 304171 is not 'non-colliding events too narrow on days with colliding events aka War-On-Boxes, p=mickey, r=philipp'
Attached image Screenshot
This patch doesn't work very well (look at these two white stripes)
Attached file Test calendar
This cal will show the problem
Omar, please file a new bug for regressions or errors you find, add the test case calendar to the new bug and report the bug here. (See Bug 393969 for example).
Blocks: 393994
(In reply to comment #49)
> Omar, please file a new bug for regressions or errors you find, add the test
> case calendar to the new bug and report the bug here. (See Bug 393969 for
> example).
> 

Done
Verified with lightning 2007082803 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070827 Calendar/0.7pre
Status: RESOLVED → VERIFIED
Bug 393994 is filled
Duplicate of this bug: 394344
No longer blocks: 393994
Depends on: 393994
You need to log in before you can comment on or make changes to this bug.