Creates collections with Elements in a removed state

[Alexithemia]

Associated issues: 3019

• This PR adds the ability to create elements in the removed state without them being added to the graph and rendered.
• This PR adds the ability to add a parent element to the params of a new node, so the node may have an assigned parent even if it or the parent is not currently in the graph.

Use case for this functionality is mainly to save loading operations when adding extra data to a graph using the expand-collapse extension when data has already been collapsed.

A test has been added

I built the dist folder, not sure if I needed to do that for this, but there are a lot of changes in it that I did not make. I only edited collection/element, collection/index, and core/search as well as the test.

Reviewers:

• [ ] All automated checks are passing (green check next to latest commit).
• [ ] At least one reviewer has signed off on the pull request.
• [ ] Just after this pull request is merged, it should be applied to both the master branch and the unstable branch. Normally, this just requires cherry-picking the corresponding merge commit from master to unstable -- or vice versa.

[maxkfranz]

I built the dist folder, not sure if I needed to do that for this, but there are a lot of changes in it that I did not make. I only edited collection/element, collection/index, and core/search as well as the test.

The dist build isn't necessary. You could reset that to make the PR a bit cleaner, if you prefer. dist is needed only just as a release is being made.

I'll give some feedback in the next day or so

[maxkfranz]

This looks like a great start on the internal changes.

Some next steps:

• [ ] More tests:
  • [ ] When restoring an element that starts off as removed, does it get restored/added properly?
  • [ ] The above w.r.t. restoration, taking into account cases re. compound nodes.
  • [ ] Ensure that it's forbidden to create a node, N, with a removed parent, P, if N is set to be added (not removed) at creation.
  • [ ] @Alexithemia, are there any other test cases that come to mind? While this may not involve that many changed lines, it's important that we're not introducing any regressions or new problems (e.g. new edge cases that weren't possible before).
• [ ] Deciding on the public API. E.g., while it's fine internally to require a flag like removed: true, it's probably better if users of the library can just call a function similar to cy.add(). This may come down to what the defaults are, and what we decide to document.
• [ ] Documenting the public API.

@Alexithemia, what do you think?

[Alexithemia]

@maxkfranz Restoring the nodes works perfectly. I'll add tests for checking after adding them with and without parents. There is no error when adding a node to the graph that has a parent does not exist, but it does not compound correctly when that parent node exists afterward, so I'll see about a check for parent existence before getting added.

I can't think of any other tests that would be needed seeing that removed nodes have been supported and are able to be added already, and creating them as removed was already something you did, I'm just skipping the restore step and keeping track of the floating removed nodes.

I would be happy to make a separate API instead of just extending part of collection() but I kept getting a forEach error, I'll see if I can get the error again.

Any preference for the name of the API function?

[maxkfranz]

@maxkfranz Restoring the nodes works perfectly. I'll add tests for checking after adding them with and without parents. There is no error when adding a node to the graph that has a parent does not exist, but it does not compound correctly when that parent node exists afterward, so I'll see about a check for parent existence before getting added.

I suppose it wouldn't make sense to add restrictions to creating elements, but it does make sense to have those restrictions for adding. There are at least three related cases:

• A child is added and its referenced parent does not exist. (Failure -- warning or error)
• A child is added and its referenced parent already exists. (Success)
• A child is added and its referenced parent is being added in the same call as the child. (Success)

I can't think of any other tests that would be needed seeing that removed nodes have been supported and are able to be added already, and creating them as removed was already something you did, I'm just skipping the restore step and keeping track of the floating removed nodes.

There are three cases in the previous paragraph. We'll have to decide how exactly to handle the failure case:

(A) Do we add the child anyway as an orphan node with a warning.

(B) Do we throw an error?

(C) Do we not add the child and make a warning?

I would be happy to make a separate API instead of just extending part of collection() but I kept getting a forEach error, I'll see if I can get the error again.

Any preference for the name of the API function?

I think cy.collection() is fine. I'm concerned that requiring the flag to be explicitly set by the library consumer could feel kludgy. Could the flag be set to the right default value for external use and left undocumented? We could still use the flag explicitly internally if needed, but consumers probably shouldn't.

[Alexithemia]

I'll work on the tests, and I would probably just add it with a warning. From what I have seen while testing, it does not cause any errors or problems when it has been added to the graph and it's parent has not. It does not render it's parent as a compound node around it when the parent is added though.

It seems like a pretty obscure use case, so I am fine not adding a separate API call for it. It is currently setup in this way so it works as it normally has unless they explicitly include the removed flag as true.

[maxkfranz]

Sounds good

On Jun 7, 2022, at 12:56 PM, Alexithemia @.***> wrote:

@Alexithemia commented on this pull request.

In src/core/search.js https://github.com/cytoscape/cytoscape.js/pull/3020#discussion_r891490565:

@@ -16,7 +16,10 @@ let corefn = ({ return eles.collection();

 } else if( is.array( eles ) ){

•  return new Collection( this, eles, opts );
  

•  if (!opts) {
  

•    opts = {};
  

•  }
  

•  return new Collection( this, eles, opts.unique, opts.removed );
  

I thinks it's currently ready to go with option 2 I just need to add the documentation.

— Reply to this email directly, view it on GitHub https://github.com/cytoscape/cytoscape.js/pull/3020#discussion_r891490565, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHRO42WD3DX3WBPSRU2CI3VN55NLANCNFSM5VSZAXWQ. You are receiving this because you were mentioned.

[maxkfranz]

@Alexithemia, do you have any questions re. the docs? Do you think this will be ready in time for the 3.22.0 release on 5 July 2022? It's a nice feature, so it would be great to include it.

[stale[bot]]

This issue has been automatically marked as stale, because it has not had activity within the past 14 days. It will be closed if no further activity occurs within the next 7 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

[maxkfranz]

@Alexithemia, are you still interested in this feature?

[stale[bot]]

This issue has been automatically marked as stale, because it has not had activity within the past 14 days. It will be closed if no further activity occurs within the next 7 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

[Alexithemia]

@Alexithemia, are you still interested in this feature?

Sorry about the delay! Really busy with things at work, and they put the need for this on the back burner. I'll finish it up here today though

[maxkfranz]

Sorry about the delay! Really busy with things at work, and they put the need for this on the back burner.

No worries

I'll finish it up here today though

Great. I'll come back to this tomorrow to get this merged in

Thanks

[maxkfranz]

The content looks great. It needs to go in the md/json files so that it doesn't get overridden when the documentation is rebuilt. You can build the docs with npm run docs

json for spec: https://github.com/cytoscape/cytoscape.js/blob/958fb4f43caee8cd217a8e0dbc5c39ef78618e9c/documentation/docmaker.json#L116-L125

md for notes & examples: https://github.com/cytoscape/cytoscape.js/blob/958fb4f43caee8cd217a8e0dbc5c39ef78618e9c/documentation/md/core/collection.md?plain=1#L1-L15

[maxkfranz]

Thanks, @Alexithemia

Published in 3.23.0