-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add Previewer to NoteEditor #18724
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 Previewer to NoteEditor #18724
Conversation
a84112d to
0c2b1c0
Compare
|
I don't have time to do a code review now, but I really like the look of it! |
Thanks! But david gave some feedback and it was decided that some re architecting is required! |
0c2b1c0 to
5e5e6d4
Compare
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way this can be split into a separate commit (or probably in a separate Pull Request), there's a lot to review here, and I worry this will slow things down getting anything moving
I'll do a separate PR. |
f95706f to
442acc4
Compare
|
One of the tests might still fail, I'll try to fix by tomorrow |
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can understand the logical structure now. Great progress!
As commented, this feels like it should be 3 PRs (borderline feasible for this PR to consist of the 3 commits, no concern with trying that if you want)
The main blocker is the hard dependency on NoteEditor, which will need to be removed, I feel the rest will be nitpicks.
|
PS: I'd recommend adding a GitHub issue, and moving most of the labels which are currently applied to this PR there |
Arthur-Milchior
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like what I'm seeing when I execute the code. But I agree with David wholeheartedly about the fact that this should be multiple PR and that the architecture is too coupled.
If it's not clear what we mean here, please don't hesitate to ask question or discord, or event let us know if you want a call to discuss it, so that you can ask all questiuons that may be relevant and help you understand why we want it
| refreshPreviewerFragmentRunnable?.let { refreshPreviewerFragmentHandler.removeCallbacks(it) } | ||
| val updateRunnable = | ||
| Runnable { | ||
| loadNoteEditorPreviewer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen_recording_20250710_030629.webm
I admit that I'm not very fan of having to reload the fragment and the webview. Because it means the webview is always at the top after reloading.
This video recording is admittedly absurd. But I can easily imagine that I've a big image, or a complex mathjax formula at the top of the note, and that I want to edit the text/formula below, and that it'll become frustrating to always scroll back at the bottom.
I'd prefer either:
- that we keep the same webview and change its content, if the scroll position is not changed with this or
- that we recall where the scroll was and put it back at the same absolute location
I must note that on anki, the template previewer has the behavior I want. As there is no editor previewer, I can't really compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that we keep the same webview and change its content, if the scroll position is not changed with this
Isn't really possible since we need to update the tab layout properly.
that we recall where the scroll was and put it back at the same absolute location
I think I'll work on this, but I will prolly push this out a bit since I am working on toggle-able hints rn
|
@Haz3-jolt on edit note mode, previewer icon is showing |
|
I am not able to resize layout. @Haz3-jolt Will you check from your side once ? |
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TemplatePreviewerFragment.kt
Show resolved
Hide resolved
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question on the de-conflict, the rest looks great!
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TemplatePreviewerFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TemplatePreviewerFragment.kt
Outdated
Show resolved
Hide resolved
4f8f5d0 to
dbdfa30
Compare
sanjaysargam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add option to disable/enable split-pane on xlarge devices.
Settings -> Developer Options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome, cheers! 🥳🥳🥳🥳
One request, but treat this as 'pending merge' after it's addressed
dbdfa30 to
73916b9
Compare
…d tags The TemplatePreviewerArguments is a data class that only stores the data - it doesn't need to mutate the lists
73916b9 to
652010c
Compare
- Introduce two-pane NoteEditor layout (editor + previewer) for xlarge screens - Add NoteEditorFragmentDelegate to notify host activity of editor events (ready, text changes, save, type change) so preview can stay in sync - Implement previewer loading in NoteEditorActivity using TemplatePreviewerFragment with tab navigation for multiple card templates - Retains tab position on text change - Rename allowSaveAndPreview(): - allowSaveAction() – controls when note can be saved - allowPreviewAction() – controls when preview menu item is shown (disabled in split mode since previewer is always visible) - Added method to update existing card being previewer updateContent() without full fragment reloading and while retaining currently selected card - Added method to get safe ord for Cloze card refresh
652010c to
6dbf592
Compare
|
THANK YOU SO MUCH!!!!! |

Purpose / Description
Fixes
Approach
How Has This Been Tested?
Tested on the following Android Emulators:
Pixel Tablet API 36
Large Desktop API 34
Screenshots of affected screen in different themes:
Black Theme:

Plain Theme:

Default Light Theme:

Default Dark Theme:

Emulator Video:
t4.webm
Checklist
Please, go through these checks before submitting the PR.