cytoscape / cytoscape-explore

Network visualization webapp.
MIT License
12 stars 4 forks source link

Feature/history phase1 #135

Closed mikekucera closed 2 years ago

mikekucera commented 2 years ago

General information

Associated issues: #122, #123

Checklist

Author:

Reviewers:

Notes

mikekucera commented 2 years ago

I don't know why these checks are failing. The build and test suite run clean on my machine.

maxkfranz commented 2 years ago

Some initial comments:

Note, I could not get PouchDB to accept a proper filter from the design document. I followed the documentation to a T, and tried a bunch of things, and I couldn't get it to work. I'm fairly certain that the problem is not on the CouchDB side, because I was able to call the filter from the http API and it worked fine. I assume there is something wrong with PouchDB.

Did you use a filter on an existing Pouch object, or on a new, separate one? What kind of filter were you applying?

I don't know why these checks are failing. The build and test suite run clean on my machine.

The package-lock.json file isn't in-sync with the package.json file. The simplest way to solve this problem is to delete package-lock.json, run npm i again, and then commit the fresh lock file. This may update some minor/patch versions of some dependencies, though usually that's OK.

maxkfranz commented 2 years ago

Aside: I think that bug with the extra node in the snapshot images would be fixed by upgrading to the latest version of edgehandles: https://github.com/cytoscape/cytoscape.js-edgehandles

mikekucera commented 2 years ago

Aside: I think that bug with the extra node in the snapshot images would be fixed by upgrading to the latest version of edgehandles: https://github.com/cytoscape/cytoscape.js-edgehandles

Did you run into this bug? I made some changes to how the snapshot doc was created, then I didn't see this bug anymore, so I was hoping it was fixed.

maxkfranz commented 2 years ago

Aside: I think that bug with the extra node in the snapshot images would be fixed by upgrading to the latest version of edgehandles: https://github.com/cytoscape/cytoscape.js-edgehandles

Did you run into this bug? I made some changes to how the snapshot doc was created, then I didn't see this bug anymore, so I was hoping it was fixed.

I didn't run into it now. Not sure how to reproduce it. It just came to mind

mikekucera commented 2 years ago

Did you use a filter on an existing Pouch object, or on a new, separate one? What kind of filter were you applying?

When calling sync() or replicate() on a pouch object, you can pass the name of a filter in the options object. The filter is a javascript function that is part of the design doc. I could not get this to work on the client side, it would just filter out everything.

Here's the documentation I was following... https://pouchdb.com/2015/04/05/filtered-replication.html https://pouchdb.com/api.html#replication (section on Filtering Options)

mikekucera commented 2 years ago

I didn't run into it now. Not sure how to reproduce it. It just came to mind

Yeah, it was very intermittent for me, happening maybe 1/50 snapshots. I'm not sure what was causing it, but I refactored some of my code and then the bug went away. Let me know if you ever see it again.

maxkfranz commented 2 years ago

I didn't run into it now. Not sure how to reproduce it. It just came to mind

Yeah, it was very intermittent for me, happening maybe 1/50 snapshots. I'm not sure what was causing it, but I refactored some of my code and then the bug went away. Let me know if you ever see it again.

I think updating edgehandles should definitely remove the possibility of it happening, in any case. I think it should just be an npm i cytoscape-edgehandles@4, in which case it may be worthwhile including the upgrade in this PR.

Edit: edgehandles, not cytosnap

maxkfranz commented 2 years ago

Did you use a filter on an existing Pouch object, or on a new, separate one? What kind of filter were you applying?

When calling sync() or replicate() on a pouch object, you can pass the name of a filter in the options object. The filter is a javascript function that is part of the design doc. I could not get this to work on the client side, it would just filter out everything.

Here's the documentation I was following... https://pouchdb.com/2015/04/05/filtered-replication.html https://pouchdb.com/api.html#replication (section on Filtering Options)

Good to know that the functions don't work (maybe a serialisation issue?). The doc IDs approach would probably be faster / less expensive anyway, right?

maxkfranz commented 2 years ago

Changed PR to merge onto new dev branch, as discussed.

mikekucera commented 2 years ago

I think updating edgehandles should definitely remove the possibility of it happening, in any case. I think it should just be an npm i cytoscape-edgehandles@4, in which case it may be worthwhile including the upgrade in this PR.

The extra node was actually showing up in the snapshot document in couchDB! There was an extra node, with only position info and no other attributes, at the bottom of the document below all the edges.

I don't know how edgehandles might be related to this issue, but I'm glad to update the version if that might be the cause.

maxkfranz commented 2 years ago

I don't know how edgehandles might be related to this issue, but I'm glad to update the version if that might be the cause.

The extension used to use elements for the handle (red dot) and preview edges so that previews would be drawn properly like the real, resultant elements. The update is better in that it doesn't use a node for the handle. The preview edge still exists, but that shouldn't be a problem. We only use the draw mode feature in CE (without the handle), so I don't think there should be any issues with the removal of some of the edgehandles API in the update