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

Getting the current Editor Page for page rendering happens out of sync #3317

Open
calculuschild opened this issue Feb 22, 2024 · 2 comments
Open
Labels
P3 - low priority Obscure tweak or fix for a single user

Comments

@calculuschild
Copy link
Member

calculuschild commented Feb 22, 2024

See #3000 which fixes the symptom but not the root cause.

The error is caused by the addition of this line:

renderedPages[props.currentEditorPage] = renderPage(rawPages[props.currentEditorPage], props.currentEditorPage);

This line is to make sure when rendering pages, we always render the currently-edited page first (which may not be in view on the brewRenderer preview). This ensures that any changes that have cross-page effects (e.g. variables or links) have their changes propagated correctly to the pages in view.

Unfortunately, updating the "current editor page" happens slightly out of sync here:

  1. Codemirror sees a text change in codeEditor.jsx
  2. Sends this new text up to handleTextChange() in editPage.jsx (or newPage or homePage)
  3. handleTextChange() requests from editor.jsx the currently edited page based on cursor position
  4. editPage.jsx propagates new text down to editor.jsx and brewRenderer.jsx.

As you can see, editor.jsx is being asked to calculate the latest cursor position before it it has received the latest text.

currentEditorPage : this.refs.editor.getCurrentPage() - 1 //Offset index since Marked starts pages at 0

This means, deleting the last \page (or part of it) will cause currentEditedPage to point beyond the total page count, thus attempting to render (and sanitize) an empty value.

It might be worth moving getCurrentPage() to come directly from Codemirror in codeEditor.jsx instead , or just add the page number as a second parameter to handleTextChange(). Not sure the best way to handle this, but I don't want this to be lost.

@calculuschild calculuschild added the P3 - low priority Obscure tweak or fix for a single user label Feb 22, 2024
@dbolack-ab
Copy link
Collaborator

I believe this has been fixed by somewhat recent code as part of the spread view.

@5e-Cleric
Copy link
Member

The issue still stands, showing only in some screens, not sure why. I get the issue in one of my screens, not the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 - low priority Obscure tweak or fix for a single user
Projects
None yet
Development

No branches or pull requests

3 participants