Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature: Allow changing deck name in statistics screen #15648

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Feature: Allow changing deck name in statistics screen #15648

wants to merge 1 commit into from
+165 −8

Conversation

criticalAY
Copy link
Contributor

Purpose / Description

Reintroduce Deck Selector to the Statistics Page

Fixes

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.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • 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

david-allison
david-allison reviewed Feb 25, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you didn't go with the spinner in the ActionBar? I'd expect this UI to be at the top of the screen

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Feb 25, 2024
@criticalAY
Copy link
Contributor Author

criticalAY commented Feb 25, 2024 •
edited

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

@david-allison
Copy link
Member

david-allison commented Feb 25, 2024 •
edited

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

@criticalAY
Copy link
Contributor Author

image-1.png

Updated screenshot

david-allison
david-allison requested changes Feb 27, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Visually, this looks awesome, thank you!

AnkiDroid/src/main/java/com/ichi2/anki/widgets/DeckDropDownAdapter.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/pages/Statistics.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/pages/Statistics.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/StatisticsActivity.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/StatisticsActivity.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/res/layout/page_fragment.xml Outdated Show resolved Hide resolved
AnkiDroid/src/main/res/layout/page_fragment.xml Outdated Show resolved Hide resolved
@criticalAY criticalAY force-pushed the stats-deck-change branch 2 times, most recently from efada90 to bde8a9f Compare February 27, 2024 14:28
BrayanDSO
BrayanDSO reviewed Feb 27, 2024
View reviewed changes
AnkiDroid/src/main/java/com/ichi2/anki/pages/Statistics.kt Outdated
Comment on lines 99 to 112
private fun changeDeck(selectedDeck: String) {
val javascriptCode = """
var textBox = [].slice.call(document.getElementsByTagName('input'), 0).filter(x => x.type == "text")[0];
textBox.value = "deck:$selectedDeck";
textBox.dispatchEvent(new Event("input", { bubbles: true }));
textBox.dispatchEvent(new Event("change"));
""".trimIndent()
webView.evaluateJavascript(javascriptCode, null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if possible, add an AndroidTest to get any backend breakages

@BrayanDSO
Copy link
Member

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

@criticalAY
Copy link
Contributor Author

@BrayanDSO Is this ok? I made some changes

@criticalAY criticalAY force-pushed the stats-deck-change branch 2 times, most recently from 31453a4 to 39f5818 Compare February 28, 2024 11:36
@david-allison david-allison added Needs Review Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Author Reply Waiting for a reply from the original author labels Mar 4, 2024
@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Review Needs reviewer reply Waiting for a reply from another reviewer labels Apr 16, 2024
@criticalAY criticalAY force-pushed the stats-deck-change branch 2 times, most recently from 9574e50 to 545b02b Compare April 21, 2024 15:34
@david-allison david-allison self-requested a review May 20, 2024 19:08
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author Stale labels May 20, 2024
david-allison
david-allison requested changes May 21, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The control in the action bar needs a "down caret" so it appears selectable

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels May 21, 2024
@criticalAY
Copy link
Contributor Author

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 5, 2024

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 github-actions bot added the Stale label Jun 5, 2024
@criticalAY criticalAY added Keep Open avoids the stale bot and removed Stale labels Jun 5, 2024
@lukstbit lukstbit added Needs Review Needs Second Approval Has one approval, one more approval to merge and removed Keep Open avoids the stale bot Has Conflicts Needs Author Reply Waiting for a reply from the original author labels Jun 19, 2024
@lukstbit
Copy link
Member

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

@david-allison
Copy link
Member

I feel I'm missing something

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

@lukstbit
Copy link
Member

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

david-allison
david-allison approved these changes Jun 19, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a 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 it design wise, but it's better than it was. Let's do it

@user1823
Copy link
Contributor

user1823 commented Jun 19, 2024 •
edited

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: #13442, which we have resolved for now precisely by not changing the Current Deck.

In #15197 (comment), 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

@lukstbit
Copy link
Member

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

@user1823
Copy link
Contributor

user1823 commented Jun 19, 2024 •
edited

Well, what you are suggesting here is exactly what I suggested in #15197 (comment). 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.

@criticalAY @lukstbit
feat: Allow deck selection in statistics screen
65c9a35 
A Spinner was introduced in the statistics screen top bar which can be
used by the user to change the current selected deck and update the
WebView with the statistics for the new selected deck.

Note: the deck selection mechanism is decoupled from the general
DeckSpinnerSelection/DeckSelectionDialog system so changing the deck
in the statistics screen doesn't modify the selected deck for other parts
of the app.
@lukstbit
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrayanDSO BrayanDSO BrayanDSO left review comments

@david-allison david-allison david-allison approved these changes

Assignees
No one assigned
Labels
Needs Second Approval Has one approval, one more approval to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reintroduce Deck Selector to Statistics Page
5 participants
@criticalAY @david-allison @BrayanDSO @lukstbit @user1823