Bug: When setting "editable:true" after initialization, items that were added previously are not editable #662

daattali · 2020-09-11T01:23:46Z

If you initialize a timeline with no options, and then use setOptions({editable:true}), any items that were created on initialization are not editable (cannot be moved or deleted). If the timeline is initialized with editable:true then all items are editable.

Reproducible example, based on the example in the README with only the last line added by me:

<!doctype html>
<html>
<head>
  <title>Timeline</title>
  <script type="text/javascript" src="https://unpkg.com/vis-timeline@latest/standalone/umd/vis-timeline-graph2d.min.js"></script>
  <link href="https://unpkg.com/vis-timeline@latest/styles/vis-timeline-graph2d.min.css" rel="stylesheet" type="text/css" />
  <style type="text/css">
    #visualization {
      width: 600px;
      height: 400px;
      border: 1px solid lightgray;
    }
  </style>
</head>
<body>
<div id="visualization"></div>
<script type="text/javascript">
  // DOM element where the Timeline will be attached
  var container = document.getElementById('visualization');

  // Create a DataSet (allows two way data-binding)
  var items = new vis.DataSet([
    {id: 1, content: 'item 1', start: '2014-04-20'},
    {id: 2, content: 'item 2', start: '2014-04-14'},
    {id: 3, content: 'item 3', start: '2014-04-18'},
    {id: 4, content: 'item 4', start: '2014-04-16', end: '2014-04-19'},
    {id: 5, content: 'item 5', start: '2014-04-25'},
    {id: 6, content: 'item 6', start: '2014-04-27', type: 'point'}
  ]);

  // Configuration for the Timeline
  var options = {};

  // Create a Timeline
  var timeline = new vis.Timeline(container, items, options);
  
  timeline.setOptions({editable:true})  
</script>
</body>
</html>

This has been previously reported and remained open in both almende/vis#3852 and yotamberk/timeline-plus#99

The text was updated successfully, but these errors were encountered:

yotamberk · 2020-09-30T13:06:09Z

Hey, I appreciate the enthusiasm and work investigating the issues but can you please try gathering all your discoveries to a single message? You are spamming maintainers' emails. We love community help! Just take these comments as a thread and not a chat.

strazto · 2020-09-30T16:12:11Z

Ah, sorry. Yeah, I don't appreciate mass emails either, I forgot that you'd be subscribed to this

strazto · 2020-10-01T05:34:09Z

Okay, I've gotten to the bottom of this @daattali , and I'm not sure if this is a bug, or rather a feature (lmao).

Overview

Essentially, at some stage, more granular control (per item) of editability was introduced, & setting: timeline.setOptions({editable: true}); does not implicitly override per-item configuration after intantiation time.

In order to do this, we must explicitly set timeline.setOptions({editable: {overrideItems: true}});, & we see the behaviour we expect.

Reproducible example

To modify your given example:

Slightly modified version of original reprex

<!doctype html>
<html>
<head>
  <title>Timeline</title>
  <script type="text/javascript" src="https://unpkg.com/vis-timeline@latest/standalone/umd/vis-timeline-graph2d.min.js"></script>
  <link href="https://unpkg.com/vis-timeline@latest/styles/vis-timeline-graph2d.min.css" rel="stylesheet" type="text/css" />
  <style type="text/css">
    #visualization {
      width: 600px;
      height: 400px;
      border: 1px solid lightgray;
    }
  </style>
</head>
<body>
<div id="visualization"></div>
<script type="text/javascript">
  // DOM element where the Timeline will be attached
  var container = document.getElementById('visualization');

  // Create a DataSet (allows two way data-binding)
  var items = new vis.DataSet([
    {id: 1, content: 'item 1', start: '2014-04-20'},
    {id: 2, content: 'item 2', start: '2014-04-14'},
    {id: 3, content: 'item 3', start: '2014-04-18'},
    {id: 4, content: 'item 4', start: '2014-04-16', end: '2014-04-19'},
    {id: 5, content: 'item 5', start: '2014-04-25'},
    {id: 6, content: 'item 6', start: '2014-04-27', type: 'point'}
  ]);

  // Configuration for the Timeline
  var options = {};

  // Create a Timeline
  var timeline = new vis.Timeline(container, items, options);
  
  timeline.setOptions({editable:true});
  timeline.setOptions({editable: {overrideItems: true}});
</script>
</body>
</html>

here's a fiddle

Discussion on validity of this behaviour

It's not clear to me whether this needs fixing or not, but I belive if any aspect of this behaviour is buggy, it's that:

When editable: true is passed, this option currently propagates to:

editable.updateTime
editable.updateGroup
editable.remove

We possibly need to extend it to also propagate to editable.overrideItems.

In fact - non-propagation to overrideItems is explicitly set here:

vis-timeline/lib/timeline/component/ItemSet.js

Line 458 in de9498c

this.options.editable.overrideItems = false;

Introduced by: b2560d0

Given that passing {editable: true} is sort of a sledgehammer approach, & users always have the option to control these options with a more fine-grained approach by passing editable as an object, I think that letting editable: true propagate to editable.overrideItems might be good behaviour.

From investigation (demo'd in the jsfiddle above), it looks like we can set a global overrideItems that propagates across to everything, and then a granular per-item switch for fine-grained control.

_{Unfortunate that I didn't clock this earlier, I would have saved myself a bit of a rabbithole of binary searching for the "buggy" commit.}

Conclusion

I'd like to hear the thoughts of @daattali & the maintainers ( @yotamberk , sorry for the spam previously - this time it's real) on what the correct behaviour should be w.r.t propagation of overrideItems.

I'll submit a PR that enables propagation, & its at the maintainers disgression whether this "fix" is neccessiated.

I will say that the difference in behaviour between setting the option @ instantiation time vs afterwards is confounding to users, enough so that regarding it as a bug may very well be appropiate.

daattali · 2020-10-01T15:06:10Z

My personal opinion is that if the timeline itself, as a whole, becomes editable, then that implies anything in it, including previous items, should be editable. That's what would make sense to me intuitively, and I'd imagine most people would expect this behaviour. The settings isn't "makeNewItemsEditable", it's just "editable" meaning "whether or not the widget is editable".

strazto · 2020-10-01T15:58:43Z

I think that's reasonable, it's weird and confusing to see behaviours so wildly different for something that should feel like the same operation

yotamberk · 2020-10-03T18:31:09Z

First, I'd like to say thank you to all that are participating in this discussion and are taking interest in this project to help improve it!

My opinion is as such:
when defining the options as editable: true it should NOT affect items that are defined individually as not editable.
If one is interested in doing so, you should pass editable: { overrideItems: true }.
This is the current behavior and I think it should be preserved as such. I agree that the behavior detailed above is a bit complicated and I will look at the PR suggested. If it doesn't affect the current behavior except the initial state, I will agree to accept it.

daattali · 2020-10-03T18:59:59Z

@yotamberk agreed that if an item explicitly disallows an option, then setting the option globally shouldn't override it.

The issue at hand refers only to the case where the entire timeline first has one option, and then receives another option

yotamberk · 2020-10-03T19:15:44Z

And what is the state of the items? Are they defined as editable?

EDIT: Ok I see what you mean. I need to check your PR more in depth.

daattali · 2020-10-03T19:27:29Z

Default, no explicit definition.

…

On Sat., Oct. 3, 2020, 15:15 Yotam Berkowitz, ***@***.***> wrote: And what is the state of the items? Are they defined as editable? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#662 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHIQFFJTK26JFPEOC7TSXDSI5Z6XANCNFSM4RGI67XQ> .

strazto · 2020-10-08T09:29:11Z

I've come back to this, and I'm glad @yotamberk didn't merge my PR because I understand the intended behaviour (/why it isn't happening) better now.

The fiddle has been updated https://jsfiddle.net/matthewstrasiotto/432afdpu/

To summarize my findings:

The "real" (reflected in DOM) editability property of an item is given by:

timeline.ItemSet.items[i].editable

There are two independant inputs with varying precedence that determine an item.editable :

timeline.ItemSet.items[i].data.editable
timeline.ItemSet.options.editable

Unless timeline.ItemSet.options.editable.overrideItems = true, settings are inherited from: timeline.ItemSet.items[i].data.editable before timeline.ItemSet.options.editable.

the processing of this inheritance is performed by item._updateEditStatus().

Previously, _updateEditStatus() was called in two places:

vis-timeline/lib/timeline/component/item/Item.js

Line 56 in ced7bc8

this._updateEditStatus();

vis-timeline/lib/timeline/component/item/Item.js

Lines 100 to 103 in ced7bc8

    
           this.data = data; 
        
           this._updateEditStatus(); 
        
           this.dirty = true;

Ie - when updating item data explicitly, or at construction. When we setOptions(), neither method is invoked (no new items are added, and no explicit item data is changed).

So why did explicitly setting timeline.setOptions({editable: overrideItems: true}) appear to fix the problem, by giving us our delete buttons back?

Description of some duplicated logic, probably intended to help mitigate this bug

vis-timeline/lib/timeline/component/item/Item.js

Lines 249 to 251 in ced7bc8

    
           _repaintDeleteButton(anchor) { 
        
             const editable = ((this.options.editable.overrideItems || this.editable == null) && this.options.editable.remove) || 
        
                            (!this.options.editable.overrideItems && this.editable != null && this.editable.remove);

_repaintDeleteButton does not really respect the inheritance model we've implemented (There should be one source of final truth, this.editable['property'], which is derived from options.editable, & item.data.editable.

Because of this, even though the editable status update was not updated correctly, the item is able to derive it from the inputs to the editable status.

Compare this with _repaintDragCenter:

vis-timeline/lib/timeline/component/item/Item.js

Lines 185 to 186 in ced7bc8

    
           _repaintDragCenter() { 
        
             if (this.selected && this.editable.updateTime && !this.dom.dragCenter) {

Only one this.editable is used for logic control.

Now see _repaintOnItemUpdateTimeTooltip:

vis-timeline/lib/timeline/component/item/Item.js

Lines 302 to 308 in ced7bc8

    
           _repaintOnItemUpdateTimeTooltip(anchor) { 
        
             if (!this.options.tooltipOnItemUpdateTime) return; 
        
             const editable = (this.options.editable.updateTime ||  
        
                             this.data.editable === true) && 
        
                            this.data.editable !== false;

This uses similar duplicated logic to repaintDeleteButton.

One more note

The following annotated line means that if you explicitly set an items option via its data attribute (Expecting that option to override that particular global setting), this will work, however it clobbers everything else provided by the global setting for that item.

vis-timeline/lib/timeline/component/item/Item.js

Lines 548 to 553 in ced7bc8

    
           } else if (typeof this.data.editable === 'object') { 
        
             // TODO: in timeline.js 5.0, we should change this to not reset options from the timeline configuration. 
        
             // Basically just remove the next line... 
        
             this.editable = {}; 
        
             util.selectiveExtend(['updateTime', 'updateGroup', 'remove'], this.editable, this.data.editable); 
        
           }

For example:

timeline.ItemSet.items[0].data.editable.remove = true
timeline.setOptions({editable: {updateTime: true}})
//updateTime setting is ignored here, time isn't updatable.

This was not a bug, & was actually explicitly tested for, but was slated to change with the major release 5.0.0 .

We're a few versions beyond that, & the TODO note is still hanging around. I've updated the behaviour, but perhaps the right thing to do here is to defer this update until major release 8.0.0.

It's overdue, but also, still constitutes a breaking change. What are your thoughts @yotamberk ?

strazto added a commit to strazto/vis-timeline that referenced this issue Sep 30, 2020

test: add example demonstrating issue visjs#662

f321197

This comment has been minimized.

Sign in to view

strazto linked a pull request Oct 1, 2020 that will close this issue

fix(timeline.setoptions): make editability hierachy & settings behave correctly #700

Open

yotamberk added the question Further information is requested label Oct 3, 2020

strazto added a commit to strazto/vis-timeline that referenced this issue Oct 24, 2020

test: add example demonstrating issue visjs#662

03cc3ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: When setting "editable:true" after initialization, items that were added previously are not editable #662

Bug: When setting "editable:true" after initialization, items that were added previously are not editable #662

daattali commented Sep 11, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

yotamberk commented Sep 30, 2020

strazto commented Sep 30, 2020

strazto commented Oct 1, 2020 •

edited

Loading

daattali commented Oct 1, 2020

strazto commented Oct 1, 2020

yotamberk commented Oct 3, 2020

daattali commented Oct 3, 2020

yotamberk commented Oct 3, 2020 •

edited

Loading

daattali commented Oct 3, 2020 via email

strazto commented Oct 8, 2020 •

edited

Loading

Bug: When setting "editable:true" after initialization, items that were added previously are not editable #662

Bug: When setting "editable:true" after initialization, items that were added previously are not editable #662

Comments

daattali commented Sep 11, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

yotamberk commented Sep 30, 2020

strazto commented Sep 30, 2020

strazto commented Oct 1, 2020 • edited Loading

Overview

Reproducible example

Discussion on validity of this behaviour

Conclusion

daattali commented Oct 1, 2020

strazto commented Oct 1, 2020

yotamberk commented Oct 3, 2020

daattali commented Oct 3, 2020

yotamberk commented Oct 3, 2020 • edited Loading

daattali commented Oct 3, 2020 via email

strazto commented Oct 8, 2020 • edited Loading

One more note

strazto commented Oct 1, 2020 •

edited

Loading

yotamberk commented Oct 3, 2020 •

edited

Loading

strazto commented Oct 8, 2020 •

edited

Loading