daattali commented 4 years ago

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 https://github.com/almende/vis/issues/3852 and https://github.com/yotamberk/timeline-plus/issues/99

strazto commented 4 years ago

Copying over a comment from your previous issues:

In version 4.16.1 this bug did not exist.

strazto commented 4 years ago

I'm finding the commit / roughly where this was introduced, so I'm just going to put down some release numbers here to get an idea of when this first occurred:

4.17: d790b3488060457fd8020ffe82b0abb70377b93d

strazto commented 4 years ago

Bug not present: https://github.com/almende/vis/blame/2b913d0b62ae601bd7d3e60cb07bb80f50a34790/package.json

strazto commented 4 years ago

Bug not present https://github.com/almende/vis/blame/cb20857c2d9a4316edd33425048314825dc8b9fe/package.json

strazto commented 4 years ago

Not present @ 4.18.1 https://github.com/almende/vis/blame/bcba4ce4825a35da91e45adf5125f2e75718f337/package.json

strazto commented 4 years ago

Bug appears present @ 4.18.1 SNAPSHOT https://github.com/almende/vis/blame/4f97bc871c7803f5ab182f761715d0c876570842/package.json

strazto commented 4 years ago

The regression was introduced somewhere between https://github.com/almende/vis/compare/6ac924c4...4f97bc87

strazto commented 4 years ago

https://github.com/almende/vis/compare/bcba4ce48...4f97bc87

~Potential commits in this range that intro'd break:~

Hidden because breaking commit has been found

https://github.com/almende/vis/commit/411a43cdc5a4df21dc72045fbb5ca88ee86a1f4f > fix(timeline): #2679 TypeError: Cannot read property 'hasOwnProperty'… > Changed data.editable to options.ediable

BREAKING COMMIT:

The breaking commit is https://github.com/almende/vis/commit/ceb698626acd38f967acfb96886d1e99e571042d

fix(timeline): #2720 Problems with option editable (#2743)

Introduced Item._updateEditStatus(), significant refactor of edit updates (This is a good one to test)

This is where the regression was originally introduced ( https://github.com/almende/vis/pull/2743 ), however, it seems as though the issue was flagged by ghttps://github.com/almende/vis/issues/2793 & possibly addressed by https://github.com/almende/vis/pull/2796

I'll check out the ref @ 2796 & see if that works

Update https://github.com/almende/vis/commit/3df8107467bdcf88f1c9f55836dc33a12be21f1f is the ref that intends to fix (a) regression added byhttps://github.com/almende/vis/pull/2743 , and perhaps it succeeded, but not in addressing this particular bug

At this state ( https://github.com/almende/vis/commit/3df8107467bdcf88f1c9f55836dc33a12be21f1f ) , Using the example given by Dean above, with the only difference that the source scripts/styles are not external resources, but rather relative)

To fix this, you must explicitly supply timeline.setOptions({editable: {overrideItems: true}});

I will test this on the head of the new repo

yotamberk commented 4 years ago

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 commented 4 years ago

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

strazto commented 4 years ago

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

```html Timeline

```

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:

https://github.com/visjs/vis-timeline/blob/de9498c6fdd77dbb84a65ae6731ecc88fe3f56c3/lib/timeline/component/ItemSet.js#L458-L458

Introduced by: https://github.com/visjs/vis-timeline/commit/b2560d0b2c7bec8b2b0bd38e919abb60b48b4020

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Default, no explicit definition.

On Sat., Oct. 3, 2020, 15:15 Yotam Berkowitz, notifications@github.com 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 https://github.com/visjs/vis-timeline/issues/662#issuecomment-703152211, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHIQFFJTK26JFPEOC7TSXDSI5Z6XANCNFSM4RGI67XQ .

strazto commented 4 years ago

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:

https://github.com/visjs/vis-timeline/blob/ced7bc88ea1f6dd987cf1e336eee4cbd06e2a949/lib/timeline/component/item/Item.js#L56

https://github.com/visjs/vis-timeline/blob/ced7bc88ea1f6dd987cf1e336eee4cbd06e2a949/lib/timeline/component/item/Item.js#L100-L103

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

https://github.com/visjs/vis-timeline/blob/ced7bc88ea1f6dd987cf1e336eee4cbd06e2a949/lib/timeline/component/item/Item.js#L249-L251 `_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`: https://github.com/visjs/vis-timeline/blob/ced7bc88ea1f6dd987cf1e336eee4cbd06e2a949/lib/timeline/component/item/Item.js#L185-L186 Only one `this.editable` is used for logic control. Now see `_repaintOnItemUpdateTimeTooltip`: https://github.com/visjs/vis-timeline/blob/ced7bc88ea1f6dd987cf1e336eee4cbd06e2a949/lib/timeline/component/item/Item.js#L302-L308 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.

https://github.com/visjs/vis-timeline/blob/ced7bc88ea1f6dd987cf1e336eee4cbd06e2a949/lib/timeline/component/item/Item.js#L548-L553

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 ?

visjs / vis-timeline

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

~Potential commits in this range that intro'd break:~

BREAKING COMMIT:

Overview

Reproducible example

Discussion on validity of this behaviour

Conclusion

One more note