-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
@SanjaySargam I'm going to want to get this in, probably for 2.21. I've been the cause of a number of conflicts and should be taking responsibility to get this over the finish line What was the resolution logic which you defined for "user has made a change, and wants to change selected card" |
Hey @david-allison, I'll resume working on this |
Let me know if you want me to de-conflict. My responsibility, as I caused a bunch of breakages |
Yeah. I'd be great |
@SanjaySargam please ping on Monday, I'll probably forget. Got a lot on right now |
Ok |
0c3483a
to
05d7e71
Compare
De-conflicted.
|
This comment was marked as resolved.
This comment was marked as resolved.
I force pushed, you want to After that, ktLint should be working with a pre-commit hook, but you can I attempted to go through each commit and ensure it passed ktLint |
@david-allison The logic is that when a user makes any unsaved note editor changes while changing the card, the dialog will prompt to save or discard changes. |
05d7e71
to
3d58bd1
Compare
@SanjaySargam To confirm: is this ready for review? |
Yes |
You probably want to rebase my 'DA' commit into something which lets us |
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.
Partial review due to the number of spacing changes which need to be reverted
// 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 is null if and only if no cards are displayed. | ||
override var currentCardId |
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.
This is a non-null value in the ViewModel, so the comment is incorrect
And we should inline this variable so it lives in the ViewModel
cardsAdapter.focusedRow = id | ||
if (viewModel.isInMultiSelectMode) { | ||
val wasSelected = viewModel.selectedRows.contains(id) | ||
viewModel.toggleRowSelection(id) | ||
// Load NoteEditor on trailing side if card is selected | ||
if (wasSelected) { | ||
currentCardId = id.toCardId(viewModel.cardsOrNotes) | ||
loadNoteEditorFragmentIfFragmented(editNoteLauncher) | ||
} | ||
} else { | ||
val cardId = viewModel.queryDataForCardEdit(id) | ||
openNoteEditorForCard(cardId) |
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.
We should move this logic to the ViewModel and test it
currentCardId = id.toCardId(viewModel.cardsOrNotes) | ||
cardsAdapter.focusedRow = id | ||
// click on whole cell triggers select | ||
if (viewModel.isInMultiSelectMode && viewModel.lastSelectedId != null) { | ||
viewModel.selectRowsBetween(viewModel.lastSelectedId!!, id) | ||
} else { | ||
viewModel.toggleRowSelection(id) | ||
} | ||
loadNoteEditorFragmentIfFragmented(editNoteLauncher) |
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.
We should move this logic to the VIewModel and test it
val launchOptions = | ||
intent?.toCardBrowserLaunchOptions() // must be called after super.onCreate() |
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.
spacing change (probably my fault)
// must be called once we have an accessible collection | ||
viewModel = createViewModel(launchOptions) | ||
|
||
setContentView(R.layout.card_browser) | ||
setContentView(R.layout.cardbrowser) |
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 still don't understand why you didn't add two layouts named card_browser
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 want to reuse the @layout/card_browser
layout in both normal and x-large layouts
I implemented in similar way like how DeckPicker is implemented
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 understand, but can we keep the original name of the file?
@@ -419,6 +480,68 @@ open class CardBrowser : | |||
registerOnForgetHandler { viewModel.queryAllSelectedCardIds() } | |||
} | |||
|
|||
private fun showSaveChangessDialog(launcher: NoteEditorLauncher) { | |||
DiscardChangesDialog.showDialog( |
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 should be a 'cancel' here. Both save and discard are 'destructive'
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 mean 3 buttons ?
- Save
- Discard
- Cancel
FYI, The dialog will be dismissed if the user clicks anywhere outside of it.
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.
Yep. I know it's against material spec, but some users won't know about the functionality, and I don't want to give the user a choice of two destructive actions
return if (frag is NoteEditor) { | ||
frag | ||
} else { | ||
null | ||
} |
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.
can this be simplified to as? NoteEditor
if (!noteEditorFrame!!.isVisible) { | ||
noteEditorFrame!!.isVisible = true | ||
} |
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.
set it to true unconditionally
@@ -498,13 +621,14 @@ open class CardBrowser : | |||
Timber.d("search state: %s", searchState) | |||
notifyDataSetChanged() | |||
when (searchState) { | |||
SearchState.Initializing -> { } | |||
SearchState.Initializing -> {} |
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.
spacing
@@ -89,4 +89,6 @@ | |||
<!--Keyboard shortcuts dialog--> | |||
<string name="edit_tags_dialog" comment="Description of the shortcut that opens the edit tags dialog">Edit tags dialog</string> | |||
<string name="show_order_dialog" comment="Description of the shortcut that shows the order dialog">Show order dialog</string> | |||
|
|||
<string name="save_changes_message">Do you want to save changes?</string> |
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.
Anki has 'please save your changes first', it may be usable. Implementer's choice
deckConfigPleaseSaveYourChangesFirst
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. Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com>
This commit ensures that NoteEditor menu will be visible in CardBroswer activity
This commit ensures that when adding note from cardbrowser it will load note editor on trailing side instead of launching NoteEditor on new screen
Prompt users to save or discard changes dialog if there is unsaved changes in NoteEditor before moving to another note, preventing accidental data loss.
This commit ensure that NoteEditor frame will be hide is the deck is empty
This commit ensures that the selected card should be highlighted on large screens only. By default first card should be highlighted Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com>
3d58bd1
to
d2df834
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.
Still got a number of spacing changes in CardBrowser
if (neutralButtonText != null) { | ||
neutralButton(text = neutralButtonText) { neutralMethod() } | ||
} |
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.
If a user supplies a method without text, it is unused.
I'd make the neutral method nullable
// must be called once we have an accessible collection | ||
viewModel = createViewModel(launchOptions) | ||
|
||
setContentView(R.layout.card_browser) | ||
setContentView(R.layout.cardbrowser) |
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 understand, but can we keep the original name of the file?
} else { | ||
viewModel.toggleRowSelection(id) | ||
launchCatchingTask { | ||
viewModel.currentCardId = id.toCardId(viewModel.cardsOrNotes) |
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.
Move to ViewModel & test
@@ -336,8 +356,15 @@ open class CardBrowser : | |||
@VisibleForTesting | |||
fun onTap(id: CardOrNoteId) = | |||
launchCatchingTask { | |||
cardsAdapter.focusedRow = 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.
move to ViewModel & test
|
||
noteEditorFrame = findViewById(R.id.note_editor_frame) | ||
|
||
if (!sharedPrefs().getBoolean("split_cardbrowser", false)) { |
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'd remove the preference. We're making a lot of CardBrowser changes in 2.21, let's go live with it
* [fragmented] will be true if the view size is large otherwise false | ||
*/ | ||
fragmented = | ||
(noteEditorFrame?.visibility == View.VISIBLE).apply { |
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.
on second thought, just Timber.i
using fragmented
:
Timber.i("Using split Browser: %b", fragmented)
@@ -505,6 +609,7 @@ open class CardBrowser : | |||
searchItem!!.expandActionView() | |||
} | |||
} | |||
|
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.
spacing changes need reverting
viewModel.endMultiSelectMode() | ||
viewModel.currentCardId = cardId | ||
// Load NoteEditor on trailing side if in fragmented mode | ||
if (fragmented) { |
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.
Probably ViewModel & react to a flow
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.
@david-allison Would you guide me here?
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.