[GSoC'24] Add TemplatePreviewerFragment to CardTemplateEditor #16529
Conversation
|
|
|
|
@SanjaySargam run and debug the listed test |
|
@david-allison I run this test in main branch, still it fails. There might the issue with test case I think |
|
CI in main passes. It's likely an issue with this PR I'm not going to have time to dig deeper today |
lukstbit
added
Needs Author Reply
|
@david-allison test fails due to |
|
|
Logs when failingLogs when passing
|
|
and @SanjaySargam Next step is for you to figure out why |
SanjaySargam
force-pushed
the
card-template-editor
branch
from
23f9de4 to
7cc3887
Compare
|
@david-allison Finally figured it out |
david-allison
added
Needs Review
and removed
Needs Author Reply
There was a problem hiding this comment.
Really great PR, thank you very much. I'm happy to see your work going so well, and already having results that'll be visible for the users :D
I tried it on simulator, not chromebook. Still a few issues, that may be resolved in future PR if it's not trivial to solve them here.
On big vertical screen, I think we can keep the buttons for front/back/css even when the keyboard appear on th escreen.
Moreso if you type with the plugged in keyboard, and there is no keyborad on screen. (Actually, the case of using an external keyboard might be true even on small screen)
I am not a fan of refreshing using the eye icon. I think we should follow anki lead and refresh when you are done editing the text for one second. Don't update for each character, but let the user directly see the result of their work.
Admittedly, when you have javascript in your card, that don't always work great. Especially if you have some alert('foo') in it. But I assume most people don't do it. And if we get complaint, we may want to consider adding a way to disable the automated refresh. But I suspect for most user, being as WISIWYG as possible will be a nicer experience.
For a reason I've not yet understood, on pixel tablet simulator at least, if you split the screen in two (using android split, to show a second app) and then put ankidroid back in full view mode, you'll get the CSS content that is copied to the front of the currently displayed card.
Plenty of smaller style remarks inside the code. Nothing that should require a lot of works to correct.
My main remark, really, is that I'd want to know in each commit what I'll see. And really, I'm loosing time because most often, I need to read the code in details to understand what the commit title meant. Please, unless your change is really trivial, add a paragraph. Explain what change is visible from the user point of view and how you achieve that change.
| @@ -95,6 +96,8 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| // the current editor view among front/style/back | |||
| private var tabToViewId: HashMap<Int, Int?> = HashMap() | |||
| private var startingOrdId = 0 | |||
| private var templatePreviewerFrame: View? = null | |||
There was a problem hiding this comment.
I think you can use FragmentContainerView instead of View. Or at least FrameLayout.
Just in case you one day need to use frame specific method, you won't have to cast.
By the way, if you don't already know it, you can use the debugger to see the type of variables at run time. This is how I found the type that is actually returned.
| @@ -35,6 +35,7 @@ import androidx.core.view.ViewCompat | |||
| import androidx.core.view.WindowInsetsCompat | |||
| import androidx.fragment.app.Fragment | |||
| import androidx.fragment.app.FragmentActivity | |||
| import androidx.fragment.app.commit | |||
There was a problem hiding this comment.
It would help me a lot to review your PR if you could add more content to the commit messages. That will even be required if we merge as multiple commit.
In the case of the first commit, what I understand after reading it in details, and I'd have loved you to explain is something such as:
On xlarge screen, display the previewer on the trailing side of the card template editor
This commit introduces a new view, card_template_editor.xml, that contains the card template editor (implemented in card_template_editor_activity.xml) and potentially the previewer on xlarge screen.
CardTemplateEditor.kt simply set the content view to card_template_editor instead of card_template_editor_activity, and add the previewer if needed.
This ensure that, we know what change is made. And then, how it's made.
There was a problem hiding this comment.
Nit: I'd add a upper case to "Attach" and generally to the first word of commit message.
| requireActivity().onBackPressedDispatcher.onBackPressed() | ||
| val toolbar = view.findViewById<MaterialToolbar>(R.id.toolbar) | ||
| fragmented = requireActivity().javaClass == CardTemplateEditor::class.java | ||
| if (!fragmented) { |
There was a problem hiding this comment.
For the sake of readability, I'd generally prefer the if not to contains negation.
If it's fragmented, we want no navigation icon, otherwise ensure that the arrow request the transmit the "back" action to its parent
seems slightly easier to read than
If it's not fragmented the arrow request the transmit the "back" action to its parent otherwise , we want no navigation icon
| if (templateEditor.fragmented) { | ||
| templateEditor.loadTemplatePreviewerFragment() | ||
| } |
There was a problem hiding this comment.
It seems each time you call loadTemplatePreviewerFragment you first check whether it's fragmented.
I believe you should change loadTemplatePreviewerFragment to loadTemplatePreviewerFragmentIfFragmented, and let this method do an early return if the view is not fragmented. It'll reduce the size of the code and the logic to consider in your PR
| @@ -47,7 +47,7 @@ | |||
| android:layout_height="wrap_content" | |||
| android:layout_gravity="bottom" | |||
| style="@style/BottomNavigationViewStyle" | |||
| android:background="?attr/appBarColor" | |||
| android:background="?attr/alternativeBackgroundColor" | |||
There was a problem hiding this comment.
For the last commit, I admit I can't give very much feedback. It's really hard to read xml and state directly whether it's an improvement or not.
I'd appreciate a lot if you could add to your PR screenshot of everything that changed. Since those changes seems to be applied on phone too, it'll be easy to take screenshot of before/after and we can easily see the difference and whether most maintainers agree its an improvement
SanjaySargam
force-pushed
the
card-template-editor
branch
from
7cc3887 to
9f32704
Compare
Light Theme
Plain Theme
Black Theme
Dark Theme
|
Tablet
|
There was a problem hiding this comment.
While the fragment is loading, there is a flicker where the right side of the screen is white. This is subtle in the video, but much more noticeable in real life
Could this be fixed (either have the screen show a loading bar, or load the fragment immediately, with the card showing a loading screen)
16529-white.webm
@Arthur-Milchior |
|
There was a problem hiding this comment.
I'm done reviewing for now, I must leave. I'll do the last commits later.
Please re-read all commit messages and documentation. Think about the "it" and whether anything is ambiguous. Whether there is missing context.
Don't hesitate to write too much. It's not that bad if you write too much, at least, I'll be sure to get all information I need in order to review.
| @@ -132,13 +145,47 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| tempModel = CardTemplateNotetype.fromBundle(savedInstanceState) | |||
| } | |||
|
|
|||
| // check, if tablet layout | |||
There was a problem hiding this comment.
I'm sorry but I don't understand what you mean here.
I kind of have guesses of what you meant, but not sure.
Please, for the time being, do long sentence. Don't worry about doing sentences that are too long. If that is ever the case will correct that later. Adding too much details is generally not a problem.
What did you mean by "check, if table layout"?
| } | ||
|
|
||
| /** | ||
| * This function initializes a TemplatePreviewerFragment with specific arguments based on the current note type. |
There was a problem hiding this comment.
While I appreciate you had documentation, I'd really would like you to be far more specific in the details.
"This function" is not useful. We know you are documenting the function, so you can remove this part.
"Initializes" is not a notion that seems well defined in kotlin or android. There are init block, but I doubt this is what you refer to.
Also, what really matters is not that the object is initialized. What matter is that the fragment is shown to the user.
What do you mean by "specific arguments"?
This function has no parameter. TemplatePreviewerFragment is a class without parameter (which is normal, given that it is a fragment).
When is "current"? You probably do not mean "right now" when you wrote this or when I read it. I guess what matters is what note type this is based on. The one of tempModel, the one of getNote's type ?
By the way, what mean "based on"? This is far too vague. I'd expect that it's the exact note type. If so "based on" should be removed, as it implicitly means that it's similar to, but not exactly the same.
|
|
||
| /** | ||
| * This function initializes a TemplatePreviewerFragment with specific arguments based on the current note type. | ||
| * It loads the contents of previewer fragment when there is change in template editor. |
There was a problem hiding this comment.
"when there is a change" seems strange.
I'd assume this function loads the content of the previewer fragment (on the trailing side) as soon as it's called.
Do you mean that it should be called when there is change in template editor? If so, write this explicitly, in a separate sentence. Explain that you expect this method to be called each time the template editor's content is changed. So this is clear that this is not part of the method's responsability. IT does not check "when there is change". On the contrary, something else looks for changes, and call it.
| @@ -301,7 +301,11 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| if (keyCode == KeyEvent.KEYCODE_P) { | |||
| if (event.isCtrlPressed) { | |||
| val currentFragment = currentFragment | |||
| currentFragment?.performPreview() | |||
| if (fragmented) { | |||
There was a problem hiding this comment.
The second commit is nice. But please, make real commit messages. I don't want to have to repeat it for every commit for every PR.
Makes long sentences. Explain everything. Don't worry if you write too much. I prefer to have too much instead of not enough.
So, I want you to explain which fragment you consider. Where it is loaded. And what you mean by "preview"
There was a problem hiding this comment.
@Arthur-Milchior
Oh Sorry, I forget to delete this commit.
Reason: Commit
The above commit will refresh the previewer fragment after one second of text change, so there is no point of loading it manually
| @@ -30,6 +30,7 @@ import com.google.android.material.button.MaterialButton | |||
| import com.google.android.material.card.MaterialCardView | |||
| import com.google.android.material.tabs.TabLayout | |||
| import com.google.android.material.tabs.TabLayout.OnTabSelectedListener | |||
| import com.ichi2.anki.CardTemplateEditor | |||
There was a problem hiding this comment.
Regarding third commit, please edit the commit message. "fragmend" is not a word.
And don't use "it" unless it's clear what it refers too. Here, the only thing it can refer to is the back button. And this back button is not fragmented.
| view.findViewById<MaterialToolbar>(R.id.toolbar).setNavigationOnClickListener { | ||
| requireActivity().onBackPressedDispatcher.onBackPressed() | ||
| val toolbar = view.findViewById<MaterialToolbar>(R.id.toolbar) | ||
| fragmented = requireActivity().javaClass == CardTemplateEditor::class.java |
There was a problem hiding this comment.
I'd expect this can be done during third commit. I don't see a reason to do it later.
| * This line allow us to know in which case we are. | ||
| */ | ||
| fragmented = requireActivity().javaClass == CardTemplateEditor::class.java | ||
| /** |
There was a problem hiding this comment.
/** those two stars are for documentation only. That is, something that explains the meaning of a member, property, variable.
Don't use it inside function please. here, just use comments. With either // or a single star
| */ | ||
| fragmented = requireActivity().javaClass == CardTemplateEditor::class.java | ||
| /** | ||
| * If it's fragmented, we want no navigation icon, otherwise ensure that the arrow request the transmit the "back" action to its parent. |
There was a problem hiding this comment.
Don't use "it" unless it refers to the last noun you used. In this case, it's the start of the sentence, so it does not work.
Worse, the template previewer fragment is never fragmented. Instead, it's part of a fragmented activity. So explain this, that you consider whether this is part of a fragmented activity.
| menu.clear() | ||
| menuInflater.inflate(R.menu.card_template_editor, menu) | ||
| /** | ||
| * This is the common menu which can be used either in template editor or in previewer fragment if [fragmented] |
There was a problem hiding this comment.
"This" seems strange. this has a specific meaning in Kotlin, and I don't think that what you mean here.
As the method's name indicate, the method is setting up the common menu.
So instead, you can state that this method setup the part of the menu that can be used either in template editor or in previewer fragment.
| @@ -544,7 +544,9 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| insertField(bundle.getString(InsertFieldDialog.KEY_INSERTED_FIELD)!!) | |||
| } | |||
| } | |||
| setupMenu() | |||
| if (!templateEditor.fragmented) { | |||
There was a problem hiding this comment.
Regarding the message of the fourth commit, please don't use "it" as often. Clarifies what you refer to
This commit ensures that the menu will be visible in previewer fragment if it is fragmented
It seems you mean that "it" is the preview fragment. But the previewer can't be fragmented. Instead, you can either state "if it belongs to a fragmented activity".
By the way, I think that "when" would be more grammatically correct than "if".
So it will be compatible to use same toolbar on both screens
I've no idea what "it" refers to. I'd expect "it" refers to "Toolbar". But then the end of the sentence makes no sense.
I think you should do shorter sentences. For example, here, one sentence to explain that you want the same kind of toolbar on both previewer and card template editor. And in another sentence, state that you switch from material toolbar to toolbar in the previewer.
SanjaySargam
force-pushed
the
card-template-editor
branch
from
b628699 to
2b0090d
Compare
There was a problem hiding this comment.
It really gets better and better, and it gets easier to review, thank you very much.
I'll almost be ready to tap "approve", if not for the fact that, on phone, for some reason, I get the card editor text area to disappear, which render it unusable.
Everything else can be solved later. But this does not seems an acceptable condition for the app.
| * It loads the contents of previewer fragment when | ||
| * 1. Addition of Card | ||
| * 2. Renaming of Card | ||
| * 2. Deletion of Card |
| ViewCompat.setOnApplyWindowInsetsListener(mainView) { _, insets -> | ||
| val imeIsVisible = insets.isVisible(WindowInsetsCompat.Type.ime()) | ||
| if (imeIsVisible) { | ||
| val isTablet = isTablet(requireContext()) |
There was a problem hiding this comment.
There is still a problem with the length of the navigation bar when it's on top of the keyboard.
There was a problem hiding this comment.
Actually, it seems there is an even worse problem on phone.
Can you please take a look at this ?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
@Arthur-Milchior This problem is solved by setting fitsSystemWindows to false
| context.resources.configuration.screenLayout | ||
| and Configuration.SCREENLAYOUT_SIZE_MASK | ||
| ) | ||
| >= Configuration.SCREENLAYOUT_SIZE_LARGE |
There was a problem hiding this comment.
Your documentation consider "xlarge" but you use the "large" constant. There seems to be an issue here.
Also, it'll probably return true on chromebook, which are not tablet.
I think that, if we need this method, you should just name it isAtLeastXLarge
Also, you may want to add a comment that state that you have the assumption that, anything whose mask is at least SCREENLAYOUT_SIZE_XLARGE will be bigger than x-large.
This seesm to make sense, but I don't see any guarantee in the documentation
| @@ -620,6 +630,11 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| menu.findItem(R.id.action_delete).isVisible = false | |||
| } | |||
|
|
|||
| // Hide preview option if it is fragmented | |||
There was a problem hiding this comment.
Still an issue with a "it" that is not very clear
| @@ -412,14 +415,21 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
|
|
|||
| // Set text change listeners | |||
| val templateEditorWatcher: TextWatcher = object : TextWatcher { | |||
| private var refreshFragmentRunnable: Runnable? = null | |||
There was a problem hiding this comment.
Please document this variable.
| templateEditor.loadTemplatePreviewerFragmentIfFragmented() | ||
| } | ||
| refreshFragmentRunnable = updateRunnable | ||
| refreshFragmentHandler.postDelayed(updateRunnable, 1000) |
There was a problem hiding this comment.
I'd appreciate if you could move this 1000 into a top level constant. And document that this is the time to wait before we refresh the previewer.
This way, if we realize that half a second gives a better user experience, it's clear where to change it.
| @@ -620,6 +630,11 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| menu.findItem(R.id.action_delete).isVisible = false | |||
| } | |||
|
|
|||
| // Hide preview option if it is fragmented | |||
| if (templateEditor.fragmented) { | |||
There was a problem hiding this comment.
It took me some time to understand why this was in the same commit than the auto-refresh. Until I realize that auto-refresh render this pointless.
I'd suggest either splitting it in two commits. Or explaining in the commit message why you put those together
| @@ -73,6 +73,7 @@ class InsertFieldDialog : DialogFragment() { | |||
| REQUEST_FIELD_INSERT, | |||
| bundleOf(KEY_INSERTED_FIELD to textView.text.toString()) | |||
| ) | |||
| (activity as CardTemplateEditor).loadTemplatePreviewerFragmentIfFragmented() | |||
There was a problem hiding this comment.
It seems that inserting a field already cause the text watcher to call afterTextChanged. And so this is redundant with the current line. This cause two reloads, which is not pleasant to see.
I'd suggest either:
- removing this line,
- or, better, ensuring that, in this case,
refreshFragmentRunnableis set to null
SanjaySargam
force-pushed
the
card-template-editor
branch
2 times, most recently
from
f335fae to
cc69459
Compare
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TemplatePreviewerFragment.kt
Show resolved
Hide resolved
| @@ -118,7 +118,7 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| * If true, the view is split in two. The template editor appears on the leading side and the previewer on the trailing side. | |||
| * This occurs when the view is big enough. | |||
| */ | |||
| private var fragmented = false | |||
| var fragmented = false | |||
There was a problem hiding this comment.
I don't believe this needs to be public, revert
| @@ -166,7 +166,7 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
|
|
|||
| slidingTabLayout = findViewById(R.id.sliding_tabs) | |||
| viewPager = findViewById(R.id.card_template_editor_pager) | |||
| setNavigationBarColor(R.attr.appBarColor) | |||
| setNavigationBarColor(R.attr.alternativeBackgroundColor) | |||
There was a problem hiding this comment.
This commit message needs work if we're not squashing this
| @@ -1114,6 +1134,9 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| private const val EDITOR_START_ORD_ID = "ordId" | |||
| private const val CARD_INDEX = "card_ord" | |||
|
|
|||
| // Time to wait before refreshing the previewer, in milliseconds. | |||
| private const val REFRESH_PREVIEW_DELAY = 1000L | |||
There was a problem hiding this comment.
What does Anki Desktop use? This feels slow
| @@ -1114,6 +1134,9 @@ open class CardTemplateEditor : AnkiActivity(), DeckSelectionListener { | |||
| private const val EDITOR_START_ORD_ID = "ordId" | |||
| private const val CARD_INDEX = "card_ord" | |||
|
|
|||
| // Time to wait before refreshing the previewer, in milliseconds. | |||
| private const val REFRESH_PREVIEW_DELAY = 1000L | |||
There was a problem hiding this comment.
But as above, I'd reduce this to xxx.milliseconds
Index: AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt b/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt (revision 82bef8d240ba8ec1315c146975e9dbeb3c6a0838)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/CardTemplateEditor.kt (date 1722228861063)
@@ -67,6 +67,7 @@
import com.ichi2.anki.previewer.TemplatePreviewerFragment
import com.ichi2.anki.snackbar.showSnackbar
import com.ichi2.anki.utils.ext.isImageOcclusion
+import com.ichi2.anki.utils.postDelayed
import com.ichi2.annotations.NeedsTest
import com.ichi2.compat.CompatHelper.Companion.getSerializableCompat
import com.ichi2.libanki.Collection
@@ -89,6 +90,7 @@
import java.util.regex.Pattern
import kotlin.math.max
import kotlin.math.min
+import kotlin.time.Duration.Companion.seconds
/**
* Allows the user to view the template for the current note type
@@ -1134,8 +1136,8 @@
private const val EDITOR_START_ORD_ID = "ordId"
private const val CARD_INDEX = "card_ord"
- // Time to wait before refreshing the previewer, in milliseconds.
- private const val REFRESH_PREVIEW_DELAY = 1000L
+ /** Time to wait before refreshing the previewer */
+ private val REFRESH_PREVIEW_DELAY = 1.seconds
@Suppress("unused")
private const val REQUEST_PREVIEWER = 0
On xlarge screen, display the previewer on the trailing side of the card template editor This commit introduces a new view, card_template_editor.xml, that contains the card template editor (implemented in card_template_editor_activity.xml) and potentially the previewer on xlarge screen. CardTemplateEditor.kt simply set the content view to card_template_editor instead of card_template_editor_activity, and add the previewer if needed.
…fragmented. This commit adds a fragmented variable to TemplatePreviewerArguments, to let the previewer knows whether it belongs to a fragmented view. If so, the previewer's back button is removed, hence only the editor's back button remain.
This commit switch the previewer menu from MateralToolbar to toolbar so that it has the same menu has the editor. It introduces setupCommonMenu and handleCommonMenuItemSelected that add the previewer's menu entry and handle them. They are called from the editor's menu if the view is fragmented, and from the previewer's own menu otherwise.
Rebased one |
This commit updates the previewer when addition, deletion, renaming of cards.
This commit ensures that if we change card on leading side then the previewer fragment will load and shows same card on trailing side
|
Needs de-conflicting as well |
SanjaySargam
force-pushed
the
card-template-editor
branch
from
82bef8d to
0f77591
Compare
|
@SanjaySargam If this is being rebased, please fixup the commit messages After this, I think it will be good to go. I'll do a final review |
|
@SanjaySargam still pending, should be a quick one |
david-allison
added
the
Needs Author Reply
@david-allison As I know, it didn't harm commit messages. Will you please be specific which commit message? |
|
"UI Improvements" needs more detail |
This commit updates the UI to ensure that the previewer theme matches the card template editor theme. CardTemplateEditor - Set navigation bar color to use alternative background color - Update background color attribute TemplatePreviewerFragment - Apply style to TabLayout for consistent appearance
refresh fragment after one second of text changed. And hide preview icon if fragmented as there is no point of having this option because the fragment will auto-refresh the content.
SanjaySargam
force-pushed
the
card-template-editor
branch
from
0f77591 to
f9a2c4f
Compare
SanjaySargam
removed
the
Needs Author Reply
|
@david-allison the commit "UI improvements" has more details. |
david-allison
added
Pending Merge
github-actions
bot
removed
the
Pending Merge
|
Hi there @SanjaySargam! This is the OpenCollective Notice for PRs merged from 2024-08-01 through 2024-08-31 If you are interested in compensation for this work, the process with details is here: Important PLEASE NOTE: The process was updated in August 2024. Re-read the Payment Process page if you have not already. We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding. Thanks! |
Purpose / Description
This feature aims to enhance the CardTemplateEditor by adding a TemplatePreviewerFragment. This will allow users to preview card templates in real-time, improving the usability and efficiency of template editing.
Approach
Steps
How Has This Been Tested?
Emulator
previewer.mp4
Checklist
Please, go through these checks before submitting the PR.