use startIndex and endIndex for restoring selection instead of opaque… #328
Conversation
| restoreInfo.cursorL = this; | ||
| } | ||
|
|
||
| if (!restoreInfo.cursorParent) { |
There was a problem hiding this comment.
I double checked that tests fail if we remove any of these guards in the "close" version of the method. So I think test coverage is good enough
src/services/latex.ts
Outdated
| if (restoreInfo) { | ||
| ctx.restoreInfo = { | ||
| startIndex: restoreInfo.uncleanStartIndex, | ||
| endIndex: restoreInfo.uncleanEndIndex |
There was a problem hiding this comment.
I wonder if it would be worth using uncleanStartIndex and uncleanEndIndex for the property names on ctx.restoreInfo. (Not a huge deal / no need to block on that if it would touch a bunch of files.)
There was a problem hiding this comment.
Yeah I had that same thought and was worried about touching a lot of files. I can take a look.
There was a problem hiding this comment.
It's a bit of a bummer to use uncleanedLatex and uncleanedStartIndex and uncleanedEndIndex in so many places. It's just ugly. But, on balance, it feels like it's worth it. This idea of clean and unclean is really subtle and really easy to not appreciate. I think it's worth thinking through at every stage if you're dealing with the clean or unclean version so that you know if it's safe to compare them.
| uncleanStartIndex += 1; | ||
| } | ||
| if (cleanIdx <= cleanEndIdx) { | ||
| uncleanEndIndex += 1; |
There was a problem hiding this comment.
I'm trying to think about whether these comparisons should be < or <=. I think maybe the answer is that it sort of doesn't matter, because a certain "clean" index can technically correspond to a range of possible "unclean" indexes, so the clean values are inherently lossy?
There was a problem hiding this comment.
Yeah, to be honest I just number wanged this until it worked. Afterwards I felt like I had an intuition about why the <= made sense. It's been long enough that I don't truly have an intuition anymore.
| // to selectionL by traversing leftward? That would be a quick and easy sanity check | ||
| // to run ahead of time to prevent messing up selection if an invalid selection is | ||
| // passed in. If an invalid selection is passed in we shouldn't loop infinitely but | ||
| // we could end up in a weird state. |
There was a problem hiding this comment.
Seems reasonable to add this as a sanity check, to fail loudly and help debugging. But I think it's probably also fine to not do so, since this would really just be a programming error, not something we'd ever expect.
There was a problem hiding this comment.
I think it's more expected if youre programmatically generating the startIndex and endIndex instead of always taking a snapshot of an existing selection. Maybe that also is a programming error but a much more reasonable one to expect and maybe do something reasonable with
There was a problem hiding this comment.
I think I'm going to just leave this as a todo for now. I honestly don't know what the best behavior should be if you pass in an invalid selection. Should it error, ignore the restoration, or do it's best? Without a clear cut answer I don't want to write less than ideal error handling.
mikehaverstock
force-pushed
the
mike/rm-opqaue-snapshot-squashed
branch
from
45f1179 to
1b30e5d
Compare
mikehaverstock
force-pushed
the
mike/rm-opqaue-snapshot-squashed
branch
from
1b30e5d to
2bfd8e2
Compare
The idea is to just use the startIndex and endIndex to restore selection instead of saving an opaqueSnapshot. This requires us to store less in the selection, makes it more reasonable to try to programmatically generate a selection, and actually fixes a bug in the old implementation when you'd start off in the denominator of a fraction and select your way out of it.
Supersedes the exploratory work here:
https://github.com/desmosinc/mathquill/pull/new/mike/rm-opqaue-snapshot-squashed