Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

Feature: Allow changing deck name in statistics screen

Open criticalAY opened this issue 11 months ago • 17 comments

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

image test-stats.webm

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

criticalAY avatar Feb 24 '24 21:02 criticalAY

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

criticalAY avatar Feb 25 '24 04:02 criticalAY

I feel this significantly reduces vertical space, which is what people want from this screen

  1. Could we move it into the menu (as the main action)?
  2. Could you send a screenshot of the spinner to see if we can make it work? The inconsistency is unusual

david-allison avatar Feb 25 '24 05:02 david-allison

image-1.png

Updated screenshot

criticalAY avatar Feb 27 '24 00:02 criticalAY

This likely will need more feedback rounds than I have availability for. Leaving to the other reviewers

BrayanDSO avatar Feb 27 '24 21:02 BrayanDSO

@BrayanDSO Is this ok? I made some changes

criticalAY avatar Feb 28 '24 11:02 criticalAY

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

github-actions[bot] avatar May 05 '24 17:05 github-actions[bot]

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

github-actions[bot] avatar May 20 '24 18:05 github-actions[bot]

It is there but I suspect not visible due to the toolbar color will override it

criticalAY avatar May 21 '24 20:05 criticalAY

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

github-actions[bot] avatar Jun 05 '24 08:06 github-actions[bot]

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):

lukstbit avatar Jun 19 '24 14:06 lukstbit

I feel I'm missing something

Where do you select a deck in the screenshots? I'd have expected it in the app bar

david-allison avatar Jun 19 '24 14:06 david-allison

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).

lukstbit avatar Jun 19 '24 14:06 lukstbit

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.

image

user1823 avatar Jun 19 '24 15:06 user1823

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).

lukstbit avatar Jun 19 '24 15:06 lukstbit

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.

user1823 avatar Jun 19 '24 15:06 user1823

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

lukstbit avatar Jun 20 '24 11:06 lukstbit

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

mikehardy avatar Aug 22 '24 16:08 mikehardy

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

mikehardy avatar Sep 16 '24 16:09 mikehardy

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.

user1823 avatar Sep 16 '24 16:09 user1823

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.

mikehardy avatar Sep 16 '24 16:09 mikehardy

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!

github-actions[bot] avatar Oct 01 '24 19:10 github-actions[bot]