-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[GSoC'24] Add NoteEditor to CardBrowser #16764
base: main
Are you sure you want to change the base?
Conversation
5997d1f
to
a49e64c
Compare
a49e64c
to
72cb215
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.
All looks good, cheers!
72cb215
to
241a1f9
Compare
I'm reviewing. Commenting because there is one main issue I've got that I'd want to be fixed before the feature is shipped to user. If you are making any change, in the trailing side, and select a new card, any change you made is lost. The note editor is already able to stop the user from leaving the view if they are unsaved changes. |
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.
Another issue I found. Move to a deck with no card. You can still select the meny entry "copy note". And it'll copy the last selected note. This should become grayed. Same with, for "show toolbar" given that it lead to a crash if there is no note editor.
My main question is: how did you test it.
I understand this is Google Summer of Code, and you're not a quality assurance expert. Still, I'd have expected that you'd test your code with various configuration, such as phone, tablet with fragmented screen, tablet with the fragment hidden, and check all buttons. Or at least, all "note editor" related menu features.
Right now I'll keep reviewing other PR, and wait for you to answer to the remarks I made, I'll come back to it after you replied to them, or, ideally, fixed bugs and applied correction I wanted
* If both conditions are true, assign true to the variable [fragmented], otherwise assign false. | ||
* [fragmented] will be true if the view size is large otherwise false | ||
*/ | ||
fragmented = noteEditorFrame != null && noteEditorFrame!!.visibility == View.VISIBLE |
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.
noteEditorFrame?.visibility == View.VISIBLE
I'm surprised, I thought I already had a recent request of change, but I can't find it
* Provides an instance of NoteEditorLauncher for editing a note | ||
*/ | ||
private val editNoteLauncher: NoteEditorLauncher | ||
get() = NoteEditorLauncher.EditCard(viewModel.currentCardId, Direction.DEFAULT) |
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.
You can remove viewModel.
given that currentCardId
is a getter for viewModel.currentCardId
@@ -1427,6 1468,9 @@ open class CardBrowser : | |||
private fun redrawAfterSearch() { | |||
Timber.i("CardBrowser:: Completed searchCards() Successfully") | |||
updateList() | |||
// Sets the first card as the current card by default after searching for cards | |||
currentCardId = viewModel.cards[0].id |
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'm not really a fan of this line.
If the card selected belongs to the new search result, I'd expect it to still appear on the right side.
By the way, it would make sense to scroll to it, so that you see what's displayed. (Note however that I would like this change only on fragmented screen. I don't think it'd make sense to scroll if no card is displayed)
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.
(Please also update the documentation for currentCardId
if you change the behaviour.
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.
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.
Seems to work the way I expected right now. I won't double check with the past version of the PR
@@ -1427,6 1468,9 @@ open class CardBrowser : | |||
private fun redrawAfterSearch() { | |||
Timber.i("CardBrowser:: Completed searchCards() Successfully") | |||
updateList() | |||
// Sets the first card as the current card by default after searching for cards |
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 am confused by your "by default".
I'm not even sure if it's "Set by default" or "current - by default - card"
I think you should update the comment of currentCardId
. Clearly, it's not only the "card that was clicked". I'd go with
The card to display in the note editor. Either in the trailing fragment or in an opened activity. It is the last card clicked without entering or being in multi select mode. If no card were clicked, then it's the first card of the search result, if any. Thus, it's null iff no cards are displayed.
By the way, I would think that, in multi select mode, we'd still want to display the content of the last touched card on the trailing side of the screen. After all, seeing the note content may be helpful to realize whether it's actually what we wanted to select
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.
By the way, I would think that, in multi select mode, we'd still want to display the content of the last touched card on the trailing side of the screen. After all, seeing the note content may be helpful to realize whether it's actually what we wanted to select
Ok
then no need to write comments here right ?
Sets the first card as the current card by default after searching for cards
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.
Fine with no comment
// Checks if the NoteEditorFragment is not an instance of `SingleFragmentActivity` | ||
// and assign it to the variable inFragmentedActivity. This indicates whether the fragment | ||
// is hosted within a fragmented activity or not. | ||
inFragmentedActivity = requireActivity().javaClass != SingleFragmentActivity::class.java |
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 feel like I already made similar comment while reviewing #16529. But, given the size of the discussion, I don't want to go look to confirm or not.
In my opinion, most of the information provided here should be the documentation of the variable.
The variable could be similar to the other PR.
/**
* Whether this is displayed in a fragment view.
* If true, this fragment is on the trailing side of the card browser.
*/
Also, I'd find it cleaner if, instead of looking for the activity (maybe one day we'll change the activities and get a strange but), you'd just use the bundle you used to open the fragment to decides whether it's fragmented or not. After all, it's an information the opener is certainly able to provide
@@ -466,6 476,11 @@ class NoteEditor : AnkiFragment(R.layout.note_editor), DeckSelectionListener, Su | |||
) | |||
setIconColor(MaterialColors.getColor(requireContext(), R.attr.toolbarIconColor, 0)) | |||
} | |||
// Hide mainToolbar if this fragment is a part of fragmented activity |
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 already added remarks regarding the comment in the template previewer fragment. I'd appreciate if you could apply them here too.
Ideally, I'd state that there is so many part of the code in common that it'd be nice to have some code moved to the parent class. But I don't think that is realistic, so I'll just ask for copy paste of the changes I requested in the other PR
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 don't understand what's wrong with my comment here
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.
Simply that it repeat what the code states. The code already state "If this then that is gone". Usually we try to add comment if either the code is really long/complex and we can summarize, or the reason of the code is not clear to the reader
One fundamental principle of UI design is that the user must have feedback on their action. Also, I'd be interested maybe in having a change of color in the tick. Or maybe a red dot, the same way that the "sync" button let you know you must sync, so it's clear where to click to save changes. |
I'd appreciate if the selected card could always be highlighted. It does not seems to be the case, for example, if you move the current card to another deck, and get a new selected card instead |
Don't do this, we're going to move it to a common codebase at some point, and this will complicate matters |
241a1f9
to
e9b1a0c
Compare
(just closing/reopening as I changed the set of github actions that run / are required, this re-triggers CI with the new set) |
I plan to hide note_editor menu if deck is empty |
e9b1a0c
to
56f2394
Compare
56f2394
to
028d962
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.
I feel like some problem I mentioned above were not resolved:
- save provide little feedback
- Change can be lost
actually, "save" moves to the first card, but the selection still appear to be on the card that was selected, which is a very strange behavior.
I don't feel like we can merge this until those are resolved.
I appreciate that, in case of multi select, we don't have the problem of the huge menu anymore. Thats nice
@david-allison You still have your approval there, do you want to remove it or do you think it's mergeable?
Since the feature is disabled by default, I can hear that it's mergeable. Still, I prefer if we could fix those issues first.
// The card to display in the note editor. Either in the trailing fragment or in an opened activity. | ||
// It is the last card clicked without entering or being in multi select mode. | ||
// If no card were clicked, then it's the first card of the search result, if any. | ||
// Thus, it's null if no cards are displayed. |
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 would have used "if and only if". So that it's clear that it's an equivalence. That there is no way for it to be null when cards are displayed.
@@ -1427,6 1468,9 @@ open class CardBrowser : | |||
private fun redrawAfterSearch() { | |||
Timber.i("CardBrowser:: Completed searchCards() Successfully") | |||
updateList() | |||
// Sets the first card as the current card by default after searching for cards | |||
currentCardId = viewModel.cards[0].id |
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.
Seems to work the way I expected right now. I won't double check with the past version of the PR
@@ -1427,6 1468,9 @@ open class CardBrowser : | |||
private fun redrawAfterSearch() { | |||
Timber.i("CardBrowser:: Completed searchCards() Successfully") | |||
updateList() | |||
// Sets the first card as the current card by default after searching for cards |
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.
Fine with no comment
@@ -466,6 476,11 @@ class NoteEditor : AnkiFragment(R.layout.note_editor), DeckSelectionListener, Su | |||
) | |||
setIconColor(MaterialColors.getColor(requireContext(), R.attr.toolbarIconColor, 0)) | |||
} | |||
// Hide mainToolbar if this fragment is a part of fragmented activity |
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.
Simply that it repeat what the code states. The code already state "If this then that is gone". Usually we try to add comment if either the code is really long/complex and we can summarize, or the reason of the code is not clear to the reader
On xlarge screen, display the NoteEditor on the trailing side of the CardBrowser This commit introduces a new view, cardbrowser.xml, that contains the card browser (implemented in card_browser.xml) and the note_editor on xlarge screen.
This commit ensures that NoteEditor menu will be visible in CardBroswer activity
This commit ensure that NoteEditor frame will be hide is the deck is empty
This commit ensures that when adding note from cardbrowser it will load note editor on trailing side instead of launching NoteEditor on new screen
This commit ensures that the selected card should be highlighted on large screens only. By default first card should be highlighted
Prompt users to save or discard changes dialog if there is unsaved changes in NoteEditor before moving to another note, preventing accidental data loss.
028d962
to
a027b8c
Compare
I changed the status of the PR, but forgot to remove my approval
@Arthur-Milchior Resolved |
Purpose / Description
This feature aims to enhance the CardBrowser by adding a NoteEditor. This will allow users to edit card side by side on large screens
Approach
How Has This Been Tested?
Medium Tablet API 34
Screen_recording_20240711_015502.mp4
Checklist
Please, go through these checks before submitting the PR.