-
-
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] Refactor DeckPicker to Single Menu Layout #16425
Conversation
299cfaf
to
aa849fa
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.
- Could you split out the functional commits (hiding the fragment on onboarding) to make this easier to review and understand
I would have expected that the combination of menus would mean:
- Remove the menu from the fragment
- Have the activity control the menu, and select different menus depending on the fragment state
Is there a reason you didn't pick this implementation?
@@ -353,13 +354,13 @@ fun DeckPicker.shouldFetchMedia(preferences: SharedPreferences): Boolean { | |||
} | |||
|
|||
suspend fun monitorMediaSync( | |||
deckPicker: DeckPicker | |||
context: Context |
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.
Why not make this an Activity
if it's necessary
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.
Not able to access from fragment ( StudyOptionsFragment )
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 sorry @SanjaySargam but I don't understand your answer. What can't access from, and how is StudyOptionsFragment related to this code change?
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.
R.id.action_sync -> {
val actionProvider = MenuItemCompat.getActionProvider(item) as? SyncActionProvider
if (actionProvider?.isProgressShown == true) {
launchCatchingTask {
monitorMediaSync(requireContext())
}
} else {
(activity as DeckPicker).sync()
}
return true
}
@Arthur-Milchior , I called this function from fragment
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.
Actually, you can access the DeckPicker from StudyOptionsFragment.kt. You already do it many time. It's just activity as DeckPicker
So you may not even need to change this function.
I agree with David however. We can just request it to be an Activity. We never actually need it to specifically be a DeckPicker.
If I do so, then the context menu ( three dots ) will show in the middle of the screen. I just thought having the menu at the top-right of the screen would be better so that's why I combined menus in fragment |
I didn't quite understand your comment about splitting out the functional commits. Could you please explain it again or provide more details? |
If I understand correctly David request, that means you'd split your PR into multiple commit. This way, it becomes far easier for us to review each commit, and consider whether it actually does what it is supposed to do |
Now understood very well |
xmlns:ankidroid="http://schemas.android.com/apk/res-auto" > | ||
<item | ||
android:id="@+id/action_undo" | ||
android:id="@+id/action_undo1" |
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, use names that are more meaningful.
In this case action_undo_study_options
and you can rename the other one action_undo_deck_picker
Why do you need to have a different ID, while the items belows seems to be fine sharing the id of deck_picker ?
@@ -46,4 +47,69 @@ | |||
android:id="@+id/action_unbury" | |||
android:title="@string/unbury" | |||
android:visibility = "gone"/> | |||
<group android:id="@+id/allItems" android:visible="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.
My understanding is that this is a copy/paste of deck_picker content, right ?
I don't like to have duplicated code. Because, if one day we update one of the menu, we may forget to update the other one.
Instead, in StudyOptionsFragment.kt
, after toolbar!!.inflateMenu(R.menu.study_options_fragment)
you can add toolbar!!.inflateMenu(R.menu.deck_picker)
, and you'll have the content of deck_picker.xml at the end of study_option_fragment
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.
My understanding is that this is a copy/paste of deck_picker content, right ?
Yes
Instead, in StudyOptionsFragment.kt, after toolbar!!.inflateMenu(R.menu.study_options_fragment) you can add toolbar!!.inflateMenu(R.menu.deck_picker), and you'll have the content of deck_picker.xml at the end of study_option_fragment
I've done this, but the issue which I'm facing rn is search icon is showing in two screens ( DeckPicker & StudyOptionsFragment ) even when I explicitly hide the search icon in the fragment. Also the export menu is already there in fragment so I'm trying to hide export menu which is there in deck_picker.xml but still 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.
I think, Sanjay, that you'll have to learn to give far more context in your text.
"Right now" seems strange to me. I don't know what "right now" refers to the code in your PR? Whether it refers to origin/main.
In all cases, I don't really understand what you mean here. I can't find any way to get two search icons. And I don't see it in your video.
Similarly, "the export menu is already there in fragment". There are two fragments so it was not clear to me which one you referred to, and I had to take a look to see it's in both fragments. It would be easier for me if you stated "both" directly
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.
Instead, in StudyOptionsFragment.kt, after toolbar!!.inflateMenu(R.menu.study_options_fragment) you can add toolbar!!.inflateMenu(R.menu.deck_picker), and you'll have the content of deck_picker.xml at the end of study_option_fragment
@Arthur-Milchior I implemented the way you said. Please review it
I've a small problem right now with your code when I execute it. I think the issue is that you only know whether you need to display one or two views once the collection is loaded. And obviously, it can't be loaded until permissions are given. So, we need to decide what to do when we don't have access to the collection. That is, what to put in the screen before the collection is loaded. Whether there is one or two view by 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.
There are still quite some improvements to do. I admit I really need more information and more context in order to really understand what you tried, what failed, and most importantly, why you did the change you did.
Please don't hesitate to message me on discord if you have questions about my remark.
I still believe we can avoid that much code duplication.
@@ -2072,7 +2087,10 @@ open class DeckPicker : | |||
Timber.i("Updating deck list UI") | |||
hideProgressBar() | |||
// Make sure the fragment is visible | |||
if (fragmented) { | |||
if (fragmented && collectionIsEmpty) { |
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 have a small preference for
if (fragmented)
if (collectionIsEmpty) {
studyoptionsFrame!!.visibility = View.GONE
invalidateOptionsMenu()
} else {
studyoptionsFrame!!.visibility = View.VISIBLE
}
}
I'd find the logic more readable.
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 (fragmented) {
invalidateOptionsMenu()
studyoptionsFrame?.visibility = if (collectionIsEmpty) View.GONE else View.VISIBLE
}
I've done this way, by the considering the case where you have only one deck and you deleted that deck, then the fragment will remove. And when we perform "undo delete deck" , the fragment will show but the optionsMenu is not updating
menu.findItem(R.id.action_rename).isVisible = true | ||
menu.findItem(R.id.action_delete).isVisible = true | ||
menu.findItem(R.id.action_export).isVisible = true | ||
(activity as DeckPicker).setupMediaSyncMenuItem(menu) |
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 we use (activity as DeckPicker)
that much, I suggest we introduce private val deckPicker = activity as DeckPicker
and we use it everywhere. That'll be more readable I expect
@@ -353,13 +354,13 @@ fun DeckPicker.shouldFetchMedia(preferences: SharedPreferences): Boolean { | |||
} | |||
|
|||
suspend fun monitorMediaSync( | |||
deckPicker: DeckPicker | |||
context: Context |
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.
Actually, you can access the DeckPicker from StudyOptionsFragment.kt. You already do it many time. It's just activity as DeckPicker
So you may not even need to change this function.
I agree with David however. We can just request it to be an Activity. We never actually need it to specifically be a DeckPicker.
@@ -46,4 +47,69 @@ | |||
android:id="@+id/action_unbury" | |||
android:title="@string/unbury" | |||
android:visibility = "gone"/> | |||
<group android:id="@+id/allItems" android:visible="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 don't like that "allItems" does not contains all items from the menu. Is there a better way of describing what it contains ?
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 we rename it as "secondaryActions"?
android:title="@string/search_decks" | ||
android:visible="false" | ||
ankidroid:actionViewClass="androidx.appcompat.widget.SearchView" | ||
ankidroid:showAsAction="always|collapseActionView"/> | ||
<group android:id="@+id/allItems"> |
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 we were to move the first element, I'd like the group to be renamed, as it's not "all item" anymore.
Also, I'd appreciate if you could explain in the commit message why you moved this item.
Generally, it'd be nice if you could explain the reason of any non-straightforward change in your commit message. And this is really not a change for which I can guess the meaning.
In particular, it seems that
menu.setGroupVisible(R.id.allItems, optionsMenuState != 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.
Why was this comment ignored? I admit I'm still confused by the name allItems
that don't contain all items.
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 we rename it as "commonItems"?
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.
Did you test your code also on small screen?
Because, we should not change anything on phone. And it seems your commit is doing some change. And actually, the orange icon and the empty line of code cause ankidroid to crash.
Please don't hesitate to ask if you don't know how to debug it. Otherwise, I'd love if you could also have something that still works on android.
Appart from that, I mostly have small remarks concerning the style. It's still hard to review because last commit does a lot of things that are vaguely related. I really want to have some simple commits that do the renaming/introducing a value, that help read the code without any functional change.
And separately THE commit that do the actual functional change, that contains nothing but the functional change.
@@ -363,6 +363,8 @@ class StudyOptionsFragment : Fragment(), ChangeManager.Subscriber, Toolbar.OnMen | |||
icon!!.isAutoMirrored = true | |||
toolbar!!.navigationIcon = icon | |||
toolbar!!.setNavigationOnClickListener { (activity as AnkiActivity).finish() } | |||
} else { |
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 appreciate if your commit message could contains slightly more details, so that I can understand what it does without having to read the code.
In this case, it would indicates that the button had no effect, and should not have been here in the first place.
Unless I'm wrong, this was a mistake in AnkiDroid, that was not related to the current PR.
I'd be fine putting it as a separate PR that could be merged very quickly.
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 was a mistake in AnkiDroid. I'll make separate PR
updateUndoLabelFromState(menu.findItem(R.id.action_undo), undoLabel) | ||
updateSyncIconFromState(menu.findItem(R.id.action_sync), this) | ||
menu.findItem(R.id.action_scoped_storage_migrate).isVisible = shouldShowStartMigrationButton | ||
setupMigrationProgressMenuItem(menu, mediaMigrationState) | ||
} | ||
} | ||
|
||
private fun updateSearchFromState(menu: Menu) { |
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.
Maybe, rename to updateSearchVisibilityFromState
to be more specific.
Because, when I read the part of the code where the method is used, I was wondering whether it was updating the search result, the search field or anything else.
@@ -81,6 +81,10 @@ class StudyOptionsFragment : Fragment(), ChangeManager.Subscriber, Toolbar.OnMen | |||
private lateinit var textTotal: TextView | |||
private var toolbar: Toolbar? = null | |||
|
|||
@VisibleForTesting | |||
var createMenuJob: Job? = null | |||
private lateinit var deckPicker: DeckPicker |
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.
My bad, I gave you a wrong snippet in the previous remark.
What I was suggesting was
/** Current activity.*/
private val deckPicker: DeckPicker
get() = activity as DeckPicker
I'd consider putting it in its own commit.
The last commit is long enough, removing from the diff all lines where you use this value would simplify reviewing it
@@ -1,7 +1,7 @@ | |||
<menu xmlns:android="http://schemas.android.com/apk/res/android" | |||
xmlns:ankidroid="http://schemas.android.com/apk/res-auto" > | |||
<item | |||
android:id="@+id/action_undo" | |||
android:id="@+id/action_undo_study_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.
Similarly, to simplify reviewing the commit, I'd considering renaming the id in its own commit. So that I can concentrate on the important part of the review without having to see all those changes.
You can do multiple renaming in a single commit.
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.
It is not possibble to rename in separate commit . If I do so, we will get an error because action_undo
id contains in deck_picker menu and studyoptions menu
android:title="@string/search_decks" | ||
android:visible="false" | ||
ankidroid:actionViewClass="androidx.appcompat.widget.SearchView" | ||
ankidroid:showAsAction="always|collapseActionView"/> | ||
<group android:id="@+id/allItems"> |
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.
Why was this comment ignored? I admit I'm still confused by the name allItems
that don't contain all items.
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 of my remarks are related to documentation/comment and can be handled later.
I tested on smartphone and tablet simulator, and it seems to work as expected. So I'm fine approving it
return true | ||
} | ||
else -> return false | ||
// item was handled by the Deck Picker, no need to handle it 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.
I'm sorry, but this comment is not right anymore.
It was right with the code snippet I gave above. Where we first tried deckPicker.onOptionsItemSelected(item)
. This comment explained what we were doing, and why we did an early return.
I'm totally fine with your new code. But you need to update the comment, to explain that the entry is not specific to study options, and so we delegate it to the deck picker
menu.findItem(R.id.action_rename).isVisible = true | ||
menu.findItem(R.id.action_delete).isVisible = true | ||
menu.findItem(R.id.action_export).isVisible = true | ||
|
||
// Add export collection item in export menu and remove it from menu |
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 think you should add quote around "export collection", because it's not part of the normal grammar of the sentence, and it's not clear you are referring to a string.
More generally, this is a place that may be confusing to someone who has no idea what's going on, who didn't follow this work. So I think it really would be nice to give far more context. Explaining that the "export collection" action appears in the menu directly when we only display the deck picker, but, when we display the splitted view, we want to move it to a submenu that already contains "export deck".
@@ -81,6 +81,13 @@ class StudyOptionsFragment : Fragment(), ChangeManager.Subscriber, Toolbar.OnMen | |||
private lateinit var textTotal: TextView | |||
private var toolbar: Toolbar? = null | |||
|
|||
@VisibleForTesting | |||
var createMenuJob: Job? = 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.
Please add a comment. Explain why it's necessary to have this value. How you intend to use it in test. That's not clear to me
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.
How you intend to use it in test.
@Arthur-Milchior
Really don't know, I just copied it from deckpicker . And I see createMenuJob
is not used in any of the test.
Should I remove @VisibleForTesting
?
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.
Yes please remove it. Either it'll break the build, and we'll understand why it was here. Or, more probably, it'll clean up some code
|
||
deckPicker.setupMediaSyncMenuItem(menu) | ||
deckPicker.updateMenuFromState(menu) | ||
createMenuJob = launchCatchingTask { |
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.
Same here. It'd be nice to write down while you save this job in a member. It's not directly clear from reading the code
I think it's unusual that the menus for the fragmented view are the responsibility of the fragment on the right, as it adds complexity to the logic I would expect the menu to be extracted out of the fragments, and extended to the full width of the activity (with the activity being responsible) You would then have two menu files for the DeckPicker (fragmented/non), and one for the Options (non-fragmented), and not need to worry about conditionally applying text, or moving some of the processing to the fragment Is there a technical reason why my hunch won't work out? |
@david-allison Would it be acceptable to handle this as a TODO and merge as it is? I will work on this during GSOC period only |
It would, add a to-do and let's get this in |
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 by me. Only a very small nit. I'd be fine merging it as-is
@@ -81,6 +81,13 @@ class StudyOptionsFragment : Fragment(), ChangeManager.Subscriber, Toolbar.OnMen | |||
private lateinit var textTotal: TextView | |||
private var toolbar: Toolbar? = null | |||
|
|||
@VisibleForTesting | |||
var createMenuJob: Job? = 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.
Yes please remove it. Either it'll break the build, and we'll understand why it was here. Or, more probably, it'll clean up some code
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.
Cheers
Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR. Read more about updating strings on the wiki, |
Hi there @SanjaySargam! This is the OpenCollective Notice for PRs merged from 2024-07-01 through 2024-07-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 PR aims to
Approach
Moved menu items from DeckPicker screen to StudyOptionsFragment
How Has This Been Tested?
Emulator
deck_picker.mp4
Chromebook
Screen recording 2024-05-22 11.08.56.webm
Checklist
Please, go through these checks before submitting the PR.