New algorithm for day view layout #677
Conversation
|
🎉🎉 awesome! This looks great. What's wrong with master at the moment? (did I forget to fix something??) |
|
|
||
| <div className={cn('rbc-events-container', { rtl: this.props.rtl })}> | ||
| {this.renderEvents()} | ||
| </div> |
There was a problem hiding this comment.
We may need to handle a tweak to DnD to account for the new div? maybe not
There was a problem hiding this comment.
Good point, I'll test the DnD stuff before merging.
There was a problem hiding this comment.
I only tested some basic DnD in the examples, and it worked as far as I can tell.
src/less/time-column.less
Outdated
| height: 100%; | ||
| position: absolute; | ||
| top: 0; | ||
| width: 100%; |
There was a problem hiding this comment.
any particular reason for this set of styles to accomplish this? as opposed to like
position: absolute;
top: 0, bottom: 0: left: 0; right 10px;i'm not saying the above is better/worse just curious
There was a problem hiding this comment.
Honestly don't remember, I think I added this a few months back. I'll look over it...
| return Math.min(diff, total) | ||
| } | ||
|
|
||
| export class Event { |
There was a problem hiding this comment.
it seems like many of the getters here are essentially readonly at object construction? maybe it'd be worth doing some of the calculations then so they aren't run more than once? Does data change? Maybe it's over optimization, but it might simplify the compiled code if nothing else
There was a problem hiding this comment.
That's definitely possible, I haven't really done any sort of optimizations yet...
There was a problem hiding this comment.
I should probably do this before merging though since it's an easy change 👍
There was a problem hiding this comment.
This was a really good call, I even ended up removing the extra MultiDayEvent class.
|
Also @taion if you have second I'd appreciate your expertise, if you could take a quick look on the layout algorithm code w/r/t complexity, efficiency, etc. The scale here is possibly 100s of events but probably mostly <50 |
|
Well, for dev at least, we'd need to upgrade R-B to work with React 16 |
src/utils/dayViewLayout/index.js
Outdated
| .sort((a, b) => { | ||
| if (a.start === b.start) { | ||
| if (a.end === b.end) { | ||
| return events.indexOf(a) - events.indexOf(b) |
There was a problem hiding this comment.
This could be expensive if it gets hit a lot. I see that Lodash is already a dep here, so maybe use sortBy there, as that thing is a stable sort.
src/utils/dayViewLayout/index.js
Outdated
|
|
||
| const sorted = [] | ||
| while (sortedByTime.length > 0) { | ||
| const [event] = sortedByTime.splice(0, 1) |
There was a problem hiding this comment.
just
const event = sortedByTime.shift()?
it's faster to remove things from end rather than start of array, but shouldn't matter if it's only 100s
There was a problem hiding this comment.
Haha yes should definitely use shift() instead (it didn't always splice from index 0 before).
src/utils/dayViewLayout/index.js
Outdated
| const sorted = [] | ||
| while (sortedByTime.length > 0) { | ||
| const [event] = sortedByTime.splice(0, 1) | ||
| event && sorted.push(event) |
There was a problem hiding this comment.
It can't, I'll remove that check...
| continue | ||
| } | ||
|
|
||
| // We've found the first event of the next event group. |
There was a problem hiding this comment.
This logic seems a little weird. Suppose you have the following hour-long events, respectively at:
- 09:30
- 10:00
- 15:00
The sorting would give:
- 09:30
- 15:00
- 10:00
But if you had:
- 09:30
- 10:00
- 13:00
- 15:00
The sorting would give:
- 09:30
- 13:00
- 10:00
- 15:00
This might be innocuous, but in a vacuum, it looks a little odd.
There was a problem hiding this comment.
Agreed it seems a bit weird, even more so now when their new algorithm doesn't do it like this, but up until a few weeks ago google actually did it this way (and I tried to just copy their behavior).
src/utils/dayViewLayout/index.js
Outdated
| const event = eventsInRenderOrder[i] | ||
|
|
||
| // Check if this event can go into a container event. | ||
| const container = containerEvents.find(c => c.endSlot >= event.endSlot) |
There was a problem hiding this comment.
Yeah, I'll see if I can simplify it.
|
Assuming I didn't mess something up, this doesn't work quite right for overlapping events. For: [
{
title: 'test',
start: new Date(y, m, d, 10, 0, 0, 0),
end: new Date(y, m, d, 11, 0, 0, 0),
},
{
title: 'test 2',
start: new Date(y, m, d, 10, 30, 0, 0),
end: new Date(y, m, d, 11, 30, 0, 0),
},
],I get: GCal gives: |
|
The Google algorithm is a bit simpler, as far as I can tell: It goes something like:
Some more examples: |
|
As an edge case example, see Note the "***" event is rendered in column 4. A non-greedy algorithm could probably render it "better" (i.e. to the left of its actual parent), but this is pretty straightforward. |
|
To demonstrate the greedy nature: In this case it would definitely be better to render the "***" event to the left, but the greedy algorithm isn't cost-sensitive |
|
Hey thanks a lot @taion, this is awesome! I've barely looked at how google's new algorithm works, but I'll see if it seems straightforward enough for me to update. This test case you provided: shouldn't be rendered like that though, so I'll have a look at it later today. Again, really appreciate your help! |
|
Can't wait for this to be merged, amazing work :) |
tobiasandersen
force-pushed
the
new-dayview-algo
branch
from
fd5b5bd to
02f8bda
Compare
| .rbc-event { | ||
| border: 1px solid @event-border; | ||
| display: flex; | ||
| max-height: 100%; | ||
| min-height: 20px; |
There was a problem hiding this comment.
Events smaller than this looks really weird. Is it okay to add this here?
|
I've addressed most of your and @taion's comments. But I kept the same algorithm (where I try to mimic google's old one) - I don't have the time or energy to do it all over again at this point. If anyone else wants to take a stab at the newer algorithm, please do! As far as I can tell, this should be good enough to merge now. And as stated above, there're probably (definitely) problems with this one as well, but I'm pretty confident in this being better than what we have now. |
|
I don't understand. The classic Google calendar uses the same layout algorithm: It handles event widths a little differently, and it doesn't "de-offset" things that don't overlap a title, but it follows (1) and (2) in the greedy algorithm I describe in #677 (comment) just the same. |
|
Yes their algorithm definitely is better than what I've done here. For this example I think mine renders it ok though:
But I'm sure one can find other examples where the differences are more striking and problematic. I know you know what you're talking about, so I'm not arguing here, but to me (2) isn't obvious. I'd love to fix/improve it, but I'm not going to have a lot of time the following weeks, and it seems like a pretty fundamental change is required. If anyone wants to pick this up from here, I really don't mind. And if @jquense wants to leave this here until I, or someone else, have the time to get back to it, that's also fine! |
|
I think this strictly an improvement over what we have so I say we merge it. For anyone that hits edge cases and may want to improve it this thread is a good resource. Thanks y'all |
|
Hi, Not sure how you wanted to treat this case. |
|
You're definitely right! Care to make a PR? :) |
|
Any plans for a new release on npm soon? Also will this fix events that have the exact same start and end time being not viewable? or only fix ones that are slightly staggered. thanks again :) |
|
That should be fixed here! About the release, I personally feel like there're a few minor issues left to fix on master before we're ready for a release. E.g:
But once those are fixed, I think we should publish a new one. |
This PR replaces the old day view layout algorithm with a new one. There are some problems with the old one that are quite hard to solve (e.g. #512). The render order is also quite messed up. I did my best to understand how Google render events in their (old) calendar. I think I got pretty close (they have since updated Google Calendar with a new layout, along with a new algorithm).
This new layout should fix #512 and #649. It's also more space efficient, and renders events in a more logical order (no need to keep track of different z-indexes for overlapping events, which was the only solution in the old version). The whole algorithm also makes a lot more sense than the old one, and is easier to follow/read (at least to me).
I've also introduced a 10px wide empty space column so that new events can always be created on a slot (same way google does it):
I'm sure this is far from perfect and still has bugs, but it should be a pretty big improvement to the old one. The master branch is currently a bit broken though, so I haven't tested this with the latest code, so we should probably hold off on merging this until master is fixed and this has been tested. I'd welcome anyone to try it out before we merge though!
For anyone trying and finding problems, please post the relevant data set here!