Skip to content
This repository has been archived by the owner on Nov 19, 2019. It is now read-only.

Range items overflowing the timeline start and end dates are initially hidden #139

Open
taavi-halkola opened this issue Mar 17, 2019 · 8 comments
Labels
bug Something isn't working

Comments

@taavi-halkola
Copy link

taavi-halkola commented Mar 17, 2019

Hi,

There seems to be a problem with range and background items where they are not initially visible in the timeline if the item(s) start and end range overflows the timelines visible window.

For example having a range item starting from 2019-03-15 to 2019-03-21 is not visible in a timeline initiated with start date of 2019-03-17 and end date 2019-03-19. Once the window is scrolled enough to the future and the item becomes visible it will then stay visible even when scrolling back to the said dates of 2019-03-17 to 2019-03-19.

The hidden items are also not found with timeline.getVisibleItems()

I can see how this could possibly cause long events to never be seen. Any idea what might be causing this?

@taavi-halkola
Copy link
Author

taavi-halkola commented Mar 19, 2019

Did some digging and it seems to be caused by a block of code in Group.js which was added back after being removed from vis.js and being discussed in #81 :

if (!this.isVisible && this.groupId != "__background__") {
    for (var i = 0; i < oldVisibleItems.length; i++) {
        var item = oldVisibleItems[i];
        if (item.displayed) item.hide();
     }
     return visibleItems;
}

Removing this block seems to remove the problem but performance suffers greatly. Any ideas?

It was mentioned to have been removed because of an issue with an old vis.js version. What issue might that have been? And what was the different fix for that issue in timeline-plus?

@taavi-halkola
Copy link
Author

taavi-halkola commented Mar 19, 2019

Here is something to help out if anyone is looking into this. The problem appears to be the item.hide(); call hiding all previously visible items without checking if they actually should be hidden. Causing the next call to Group._updateItemsInRange() to completely miss these items. Replacing

var item = oldVisibleItems[i];
 if (item.displayed) item.hide();

with

this._checkIfVisibleWithReference(oldVisibleItems[i], visibleItems, visibleItemsLookup, range);

seems to do the trick and scrolling performance with hidden groups still seems much better than without the whole block.

@sbusch
Copy link
Contributor

sbusch commented Mar 21, 2019

I have the same problem but the block @taavi-halkola mentioned is IMO unrelated to the problem of such items not appearing (it's about hiding all items of invisible groups). Removing the block did not solve the problem.

I'm currently working on a fix.

@yotamberk yotamberk added the bug Something isn't working label Mar 30, 2019
@jaytrepka
Copy link

Hi, is there any progress here or is there something i can help with? I really need this.

@sbusch
Copy link
Contributor

sbusch commented Apr 23, 2019

@jaytrepka I have a working fix, but with the downside of losing some optimisations regarding large item sets. That's why I didn't push a PR yet...

I'm considering to start such a PR but marked as "not ready to be merged" to start a discussion how this could be merged by @yotamberk. I hope I can continue to work on it soon, maybe in the next few days.

@taavi-halkola
Copy link
Author

taavi-halkola commented Apr 24, 2019

@sbusch I've come to the same conclusion. There are two occasions where this is affected and both seem to affect performance...

@jaytrepka There seems to be a possible workaround but i am not sure what negative side effects it may or may not have. When you are initializing your timeline start with empty groups and items. Then first add items through timeline.setItems() and finally the groups with timeline.setGroups(). Also i do not know if this also has the negative effects of losing performance or not. You should probably test that first if running a performance critical application.

timeline = new Timeline(container, [], [], options)
timeline.setItems(dataset)
timeline.setGroups(groups)

The main difference here is when initializing the timeline with groups and items the order is the other way around and for some reason this makes the difference. Although drawing the groups first is the correct way to go and commented as important in Timeline.js.

// IMPORTANT: THIS HAPPENS BEFORE SET ITEMS!
if (groups) {
    this.setGroups(groups);
}
// create itemset
if (items) {
    this.setItems(items);
}

@sbusch
Copy link
Contributor

sbusch commented Apr 24, 2019

Et voila, here's the draft PR #157

Appreciating any comments

@sbusch
Copy link
Contributor

sbusch commented Apr 24, 2019

@jaytrepka If you're making your own builds, you can safely merge this PR into your fork. The scope of changes is quite small and besides the performance drawback it should just work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants