Anki-Android
Anki-Android copied to clipboard
Feature: Allow changing deck name in statistics screen
Purpose / Description
Reintroduce Deck Selector to the Statistics Page
Fixes
- Fixes #15197
Approach
Use M3 button to allow user to change deck, not using the spinner here as the deck name is already displayed in the text field
How Has This Been Tested?
Google emulator API 34 Tested the orientation change too
Checklist
Please, go through these checks before submitting the PR.
- [x] You have a descriptive commit message with a short title (first line, max 50 chars).
- [x] You have commented your code, particularly in hard-to-understand areas
- [x] You have performed a self-review of your own code
- [x] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
- [ ] UI Changes: You have tested your change using the Google Accessibility Scanner
Ohh that's because the deck name is already shown in the page why to have it at top would that be a good ui ? I did mention that in my pr template,
- other reasons action bar spinner has a sublist too, why would we need that here?,
- note editor spinner doesn't look good in the action bar, I already tried it
I feel this significantly reduces vertical space, which is what people want from this screen
- Could we move it into the menu (as the main action)?
- Could you send a screenshot of the spinner to see if we can make it work? The inconsistency is unusual
Updated screenshot
This likely will need more feedback rounds than I have availability for. Leaving to the other reviewers
@BrayanDSO Is this ok? I made some changes
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
It is there but I suspect not visible due to the toolbar color will override it
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
I've fixed the conflicts and made some changes to implement the feature(basically, just change the current selected deck and reload the embedded WebView). The new behavior follows the desktop behavior where selecting a deck in the statistics screen doesn't change the html input at the top.
Some images(note that I added a background for the decks spinner area to differentiate it a bit from the rest of the content, I think it looks better):
I feel I'm missing something
Where do you select a deck in the screenshots? I'd have expected it in the app bar
Where do you select a deck in the screenshots? I'd have expected it in the app bar
It's the Spinner at the bottom(in the picture there was a deck B selected).
Besides the desktop UI that places the deck selector at the bottom it was much easier to implement this way(I would prefer to keep the title as it is).
The new behavior follows the desktop behavior where selecting a deck in the statistics screen doesn't change the html input at the top.
But, does it mean that changing the deck in the Stats page changes the current deck? If so, it will affect the reviewer if the user goes to the Stats page and changes the deck there in the middle of doing reviews.
From the issue description, I quote:
If we set the Current Deck in the Statistics deck picker, it will result in the reintroduction of this bug: https://github.com/ankidroid/Anki-Android/issues/13442, which we have resolved for now precisely by not changing the Current Deck.
In https://github.com/ankidroid/Anki-Android/issues/15197#issuecomment-1936853509, David wrote:
Personally: if I switch decks in the reviewer or the Card Browser, I don't want the current review session's deck to change
(Here, "decks in the reviewer" should be read as "decks in the Stats page" IMO.)
In this Anki forums post, Damien wrote:
The other Anki clients change the current deck when the user selects a different deck. If you don’t want to do that, you’ll need to send through a PR that exposes a JS function to update the search with “deck:…”.
So, Damien seems to support the idea of updating the search when selecting a deck.
Also, design-wise, I liked the design by criticalAY.
But, does it mean that changing the deck in the Stats page changes the current deck? If so, it will affect the reviewer if the user goes to the Stats page and changes the deck there in the middle of doing reviews.
My quick test just now showed the same behavior on desktop. Do you see a different behavior? If it is the same I don't think we should go another way(although the behavior is not nice).
Well, what you are suggesting here is exactly what I suggested in https://github.com/ankidroid/Anki-Android/issues/15197#issuecomment-1925761823. If we go that route, we will also need to add code for refreshing the reviewer when the user returns to the reviewer, though (see the linked comment).
However, I later got convinced by David's comment that users won't want their reviews to be affected by changing the deck in Stats page. Also, see that Damien also supports changing the search query and is welcoming a PR to do that in Anki.
I made some changes:
- reverted to the popular design
- the deck is changed/statistics are shown without changing the selected deck for the rest of the app
Note for me mainly as I look to merge all things merge-able and I scan this, only outstanding thread is this, hoping for a PR or issue to mark the code generating the form we twiddle here upstream so we get a note if they modify it: https://github.com/ankidroid/Anki-Android/pull/15648#discussion_r1727502723
Current status: I think we're really close here. Only sticking point is cross-repo programmatic usage of the web form and that will resolve but it may take an iteration or so
I think that this PR can be merged now as it is and the change from "first text box" to ID can be made in a subsequent PR.
Reasoning: Even if dae merges the linked PR today, we likely won't get the change in AnkiDroid until the next stable release of Anki, which seems to be at least a month away. In my opinion, it isn't wise to block this PR just for that small change, which we can easily make once the backend update is available.
Obviously, this is just my opinion and the maintainers are free to disagree.
That's a reasonable point @user1823 I'll merge this and peel off the cross-repo compatibility thing to a separate issue so the bulk of this (which is good!) is unblocked.
Hi there @criticalAY! This is the OpenCollective Notice for PRs merged from 2024-09-01 through 2024-09-30
If you are interested in compensation for this work, the process with details is here:
https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid
[!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!