304741 - (warOnBoxes) non-colliding events too narrow on days with colliding events

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

bsmedberg suggested that using extra vboxes inside the <stack> might help avoid the problems vlad was describing.

Comment 5

[Joey Minta]

*** Bug 330389 has been marked as a duplicate of this bug. ***

Comment 6

[Stefan Sitter [:ssitter]]

*** Bug 336384 has been marked as a duplicate of this bug. ***

Comment 7

[Joey Minta]

Attachment: work in progress

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)

Comment 8

[Joey Minta]

Attachment: for testing

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.

Comment 9

[Joey Minta]

Attachment: proper layout v1

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.

Comment 10

[Joey Minta]

Attachment: 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.)

Comment 11

[Joey Minta]

Comment on attachment 225507 [details] [diff] [review]
proper layout v1

aww crap, i'm seeing some mouse-event-eating with this patch now.

Comment 12

[Joey Minta]

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

Comment 13

[Joey Minta]

Attachment: updated patch

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.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Unfortuantely, we've got enough other big patches that we absolutely need to take that we should probably punt on this.

Comment 15

[Matthew (lilmatt) Willis]

Attachment: more up to date merge (untested)

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: merged patch

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.

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 19

[Joey Minta]

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.

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: 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)

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated again

updated the patch once again to trunk. Just so that i can remove the code from my tree and still have a patch somewhere...

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 24

[Michael Büttner (no reviews TFN)]

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

Comment 26

[Michael Büttner (no reviews TFN)]

Attachment: final patch v1

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.

Comment 27

[Stefan Sitter [:ssitter]]

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.

Comment 28

[Michael Büttner (no reviews TFN)]

Attachment: final patch v2

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.

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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)

Comment 31

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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 [e-mail] in hope that its
enough to just type "philipp" in the requestee box :)

Comment 33

[Michael Büttner (no reviews TFN)]

Attachment: final patch v3

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.

Comment 34

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 35

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 36

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 37

[Michael Büttner (no reviews TFN)]

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

Comment 38

[Michael Büttner (no reviews TFN)]

Attachment: final patch v4

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.

Comment 39

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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 40

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 41

[Michael Büttner (no reviews TFN)]

Attachment: final patch v5

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.

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

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 43

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 44

[gekacheka]

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

Comment 45

[Simon Paquet [:sipaq]]

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.

Comment 46

[Omar Bajraszewski]

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'

Comment 47

[Omar Bajraszewski]

Attachment: Screenshot

This patch doesn't work very well (look at these two white stripes)

Comment 48

[Omar Bajraszewski]

Attachment: Test calendar

This cal will show the problem

Comment 49

[Stefan Sitter [:ssitter]]

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

Comment 50

[Omar Bajraszewski]

(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

Comment 51

[Omar Bajraszewski]

Bug 393994 is filled