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

Add support for scroll position sync #2404

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

guanglinn
Copy link
Contributor

@guanglinn guanglinn commented Sep 3, 2024

Add basic support for scroll position sync between Markdown preview (view-mode) and its source code (edit-mode).

This feature supports:

  • Sync position from edit to preview;
  • Sync position from preview to edit.

The related issues:
#2373 #2189 #1070 #1060 #679.

@guanglinn
Copy link
Contributor Author

guanglinn commented Sep 3, 2024

You can see the demonstration ↓

2024-09-03-markdown-scroll-sync.mp4

@guanglinn guanglinn changed the title Add support for scroll sync Add support for scroll position sync Sep 5, 2024
@gsantner
Copy link
Owner

gsantner commented Sep 16, 2024

has conflicts

@guanglinn
Copy link
Contributor Author

guanglinn commented Sep 17, 2024

Conflicts are resolved :)

Copy link
Owner

@gsantner gsantner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried locally,

unfortunately not works quite well for both directions. From View-Mode to Edit-Mode it seems to works good enough.
But from Edit to View mode it seems to not work, always jumps to top of document.
This is the builtin Markdown reference.

screenrecord-2024-09-25_00.59.29.webm

@gsantner
Copy link
Owner

I've merged your other PR to master as well, made conflicts again

@guanglinn
Copy link
Contributor Author

Conflicts are resolved now.

@guanglinn
Copy link
Contributor Author

guanglinn commented Sep 25, 2024

Sure, from Edit to View, it works well for quite a lot common text blocks such as Headings, Bold, Italic, Blockquotes, Lists, Links, ... , but it works not quite well for some special text blocks.

I have tried to solve this, it's quite difficult, and I think this is already a big progress for Markor can provide a rough position synch for Markdown.

As mentioned above, you can merge it first, it is stable now, and I will improve it if I have some better solutions later, or other contributors would make it perfect on this basis.

@gsantner
Copy link
Owner

gsantner commented Sep 25, 2024

What I would suggest for now: Save/Restore last webview scroll position after short delay (when blocks have the right rendered size).

As far I recall this has been before like that, but somehow it doesn't work currently on master either

@guanglinn
Copy link
Contributor Author

guanglinn commented Sep 25, 2024

Can an option be added? Allow users to choose to let Markor restore last WebView scroll position, or sync WebView scroll position from Edit.

@gsantner
Copy link
Owner

I don't think it's neccassary. When scroll sync work good enough in praxis there is also the editor position saved/restored - so will be view-mode position when switiching to.

@gsantner
Copy link
Owner

gsantner commented Sep 25, 2024

Unfortunatley also keep in mind what I tried to say always - Markor is not only Markdown. This all relies on Markdown specific injections/adjusting the original.

This won't work for stuff that doesn't base on Markdown converter. So we need in any case such a fallback. And saving/restoring the view-mode scroll position should be viable for that.

It can't be resolved generically for everything and not easy to get much stable.

@guanglinn
Copy link
Contributor Author

guanglinn commented Sep 26, 2024

Improvements: If we want to go back to the last view position, just long click the toolbar.

@gsantner gsantner closed this Oct 21, 2024
@gsantner gsantner reopened this Oct 21, 2024
@gsantner
Copy link
Owner

gsantner commented Oct 21, 2024

@harshad1 @wshoy @elyahw
may I ask about your thoughts, how good does it work on your device?

I watched guanglinns video again, but it just not works as good here at least in one way, which overall seems also worse then without the PR. In such case I don't feel too well either with bearing that by default to users.

Also very important to me: it's a feature for Markdown. People will rumble because not available/broken for others, issues inevitable.

@wshoy
Copy link
Contributor

wshoy commented Oct 25, 2024

This feature feels great on an emulator, but unfortunately, it mostly doesn't work on my phone.
When it does, it's only a couple times after reloading the file, and only from view mode to edit mode. Then it doesn't work at all.

On an emulator, when going from view mode to edit mode, it often jumps to the next line. Probably because of the word wrapping.

showSelection(text, text.getSelectionStart(), text.getSelectionEnd());
}

public static void setSelectionAndShow(final EditText edit, boolean setSelection, final int... sel) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have showSelection to show the selection and setSelectionAndShow to set selection and show. Having a boolean flag here to setSelection in a function called setSelectionAndShow is poor design imo. Just call the right function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have made some improvements for this.

showSelection(edit, start, end);
}
}

public static void setSelectionAndShow(final EditText edit, final int... sel) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate functions are better than flags unless there is a good reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants