Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: ObservableList #1959

Closed
wants to merge 15 commits into from
Closed

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Mar 19, 2017

Removes Vector and replaces ObservableVector with ObservableList. More closely matches the Google CollaborativeList API, while removing the values from the change signal to enable cheaper notifications to be emitted from a server-based backend.

  /**
   * The change types which occur on an observable list.
   */
  export
  type ChangeType =
    /**
     * Item(s) were added to the list.
     */
    'add' |

    /**
     * An item was moved within the list.
     */
    'move' |

    /**
     * Item(s) were removed from the list.
     */
    'remove' |

    /**
     * An item was set in the list.
     */
    'set';

  /**
   * The changed args object which is emitted by an observable list.
   */
  export
  interface IChangedArgs {
    /**
     * The type of change undergone by the list.
     */
    type: ChangeType;

    /**
     * The new index associated with the change.
     */
    newIndex: number;

    /**
     * The old index associated with the change.
     */
    oldIndex: number;

    /**
     * The number of items added or removed.
     */
    count: number;
  }

@blink1073
Copy link
Contributor Author

cc @ian-r-rose, @ellisonbg

@blink1073
Copy link
Contributor Author

cf #1767.

@blink1073 blink1073 added this to the Beta milestone Mar 19, 2017
@blink1073 blink1073 mentioned this pull request Mar 19, 2017
@ellisonbg
Copy link
Contributor

Does this add the uuid for each change?

@blink1073
Copy link
Contributor Author

No, I didn't have a clear sense of how we wanted to handle that yet.

@blink1073 blink1073 self-assigned this Mar 20, 2017
@ian-r-rose
Copy link
Member

I am a little concerned about the non-atomicity of these change signals (as @ellisonbg suggested the other day). If the underlying list has changed between when the signal is received and when the new entries are queried, then the view will get out of sync with the model.

On the other hand, with the CellList approach to the notebook, we will need some special logic for cell operations anyways. That is, if a new cell is inserted, the information will not hit the cellMap and cellOrder objects at the same time. For a single user this is easy enough to resolve, but will be trickier for multiple users.

@ellisonbg
Copy link
Contributor

ellisonbg commented Mar 20, 2017 via email

@blink1073
Copy link
Contributor Author

An alternative is to make the changed signal dumb and treat it as a an atomic value for the consumer. The underlying implementation would provide the undo capability as it sees fit.

@blink1073
Copy link
Contributor Author

We could treat ObservableList the same as ObservableValue, but the subclass would allow backend implementations to use whichever mechanism they like.

@ian-r-rose
Copy link
Member

That would be fine with me, overall. That would make it much harder for the model and view to go out of sync.

@blink1073
Copy link
Contributor Author

Going to close this in favor of #2039 approach.

@blink1073 blink1073 closed this Apr 19, 2017
@blink1073 blink1073 deleted the observablelist branch May 16, 2017 21:12
@blink1073 blink1073 mentioned this pull request May 17, 2017
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintenance pkg:notebook status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants