Bidirectional Linking #622
Conversation
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-09-10T18:01:44.067Z) |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-09-13T18:25:46.351Z) |
jvorob
force-pushed
the
link-bidirectional
branch
from
7142cd0 to
671f889
Compare
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-09-13T18:43:06.709Z) |
jvorob
force-pushed
the
link-bidirectional
branch
from
671f889 to
2de08cb
Compare
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-09T01:59:22.195Z) |
jvorob
added
preview
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-12T08:29:16.785Z) |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-12T09:12:17.523Z) |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-12T09:28:05.780Z) |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-12T11:47:43.741Z) |
jvorob
force-pushed
the
link-bidirectional
branch
from
0b61eb8 to
8fe3498
Compare
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-12T11:53:33.548Z) |
| // (i.e. two sections back) instead of looking at our srcSection's activeRowId. This way it doesn't matter how | ||
| // section B is filtered, since we're getting our cursorPos straight from A (through a computed in B.LS) | ||
| // | ||
| // However, each linkingState needs to decide whether to use the cursorPos from the srcSec (i.e. its activeRowId), |
There was a problem hiding this comment.
Why it neeeds to decide it?
There was a problem hiding this comment.
As in:
the linkingState can take cursorPos from this.srcSection or this.srcSection.linkingState
If the srcSection is "driving" the linking, i.e. was most recently clicked, then we use it (e.g. B.LS pulls from its srcSection directly).
Otherwise the driver is further up the chain, so we use srcSection.linkingState to get to it (e.g. C.LS pulls from its srcSection.linkingState, since B isn't the driver of this chain)
Whichever it is, there needs to be some logic that decides which to use, is what I was trying to say.
Does that explain it better? And/or is the original explanation misleading?
| //linked section (srcSection) and all upstream sections (through srcSection.linkingState) | ||
| this.incomingCursorPos = this.autoDispose((ko.computed(() => { | ||
| // Get previous linkingstate's info | ||
| // NOTE: prevlink = this.srcSec.linkingState is 2 hops back, (i.e. it reads from from this.srcSec.linkSrcSec) |
There was a problem hiding this comment.
| // NOTE: prevlink = this.srcSec.linkingState is 2 hops back, (i.e. it reads from from this.srcSec.linkSrcSec) | |
| // NOTE: prevlink = this.srcSec.linkingState is 2 hops back, (i.e. it reads from this.srcSec.linkSrcSec) |
Can you rewrite it and add more explanation I don't follow this..
There was a problem hiding this comment.
rewritten, is the new one better?
|
Have you thought about the UI changes it should bring along? |
|
When I have a filtered section, and the cursor "vanishes" becuase it was set to a filtered our row, and then I click on the same row as it was before, the position is not updated. In screenshot, section 4 is selected by 1, but the cursor is not updated
When deleting and undoing, the cursor is not updated, or wrongly reflect the state
https://grist-link-bidirectional.fly.dev/wFKyLXrPdX1z/jarek/p/38 |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-14T19:37:57.824Z) |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-19T07:06:47.704Z) |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-19T07:11:53.712Z) |
|
RE: Have I thought about UI changes it should bring
|
|
@jvorob Thank you very much for this work, which would be really handy!
|
|
|
||
| it('should allow creating cursor-linked-cycles', async function() { | ||
| assert.equal(await driver.findContent('.test-select-row', /Performances detail/).isPresent(), true); | ||
|
|
There was a problem hiding this comment.
But I think we don't have any tests that cover this bi-directional linking, nothing that is testing that it actually works?
app/client/ui/selectBy.ts
Outdated
| // NOTE: we're guaranteed to hit target before the end of the array (because of the `if(...includes...)` above) | ||
| // ALSO NOTE: isAncestorSameTableCursorLink may be 1 shorter than ancestors, but it's accounted for by the above | ||
| let i = 0; | ||
| for (;;) { |
There was a problem hiding this comment.
Clever :D Eslint is tricked, but it is my fault, I don't know how to configure rules to cover this case. Still, though, some safe guard for the number of loops would be good to add, like while (i < 10000) {} if (i >= 10000) throw new Error()
There was a problem hiding this comment.
So, there's already a if i == source.ancestors.length - 1 { throw Error } in the body of the loop to keep it from going infinite. Maybe I could change it to if i >= to make it a little safer? But I think trying to push that condition into the while would be ugly, since the success condition is breaking out of the loop. Would have to do some sort of thing with a successFlag = false, if(successCondition) { successFlag = true; break; } ... /* after loop */ if(!successFlag) { throw Error }, whereas the current code already covers it. Do you think it's acceptable to leave it as is or should I force that condition into the while?
Hi @jperon, Can you explain what you have in mind by "on cards". The "Select By" is enabled for all kind of widgets by default, so a "Card list" widget is supported out of box. A single "Card Widget" is also supported, but as it doesn't have navigation buttons, this feature wouldn't make much sense there. |
A Card widget sometimes has navigation buttons, so maybe @jperon is suggesting to enable those in this case?
|
|
@dsagal You have exactly seen what I meant. |
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-23T04:05:10.501Z) |
Previously we'd set cursorPos=null if linked cursorPos was outdated relative to us. However, this caused issues (little arrow would disappear, wouldn't get set correctly when first setting selectBy) Removing it seems to work fine, since observables prevent looping and version keeps cycle terminated correctly
Previously I updated cursorEdited on rowIndex() change However, this ran into the exact same bug that properRowId was created for, namely that if the data changes (filtered out), then rowIndex won't update. Switched the dependency to properRowId seems to fix it. Also, weird timing/dependency stuff, need to subscribe version AFTER subscribing activeRowId
We now want to disallow most cycles, but allow same-table cursor-linked cycles Note: I tested that filtered cycles are forbidden, same-table-cursor cycles are allowed, but I never checked if different-table cursor cycles are forbidden might be something to add, but would require more fiddling
Also removed cursorPos null tweak
christ that's a whole paper in there
If several sections cyclically cursor-linked, and a row was deleted, the sections would go to different places. (default behavior on row delete is jump to row 0, but active section's cursor explicitly keeps its index on row-delete. The setCursorPos from this WAS actually called last, so should have correctly set cursorEdited(), but didn't get called)
jvorob
force-pushed
the
link-bidirectional
branch
from
4fb8c79 to
cd8af59
Compare
|
Deployed as https://grist-link-bidirectional.fly.dev (until 2023-10-25T22:22:14.701Z) |
Adds bidirectional linking (same-table cursor-links)
Allows linking sections in a cycle as long as all are same-table cursor links
(e.g. A->B->A, A->B->C->A, etc)
Clicking/moving cursor in any of the linked sections updates them all.
Works correctly even if sections have differing filters
Tests now check that same-table cursor cycles are allowed in selectBy,
but others are forbidden.
No actual tests of behavior, though