"Deck Search" display now same as the Deck Viewer #10389

viciousAegis · 2022-02-24T07:34:54Z

Pull Request template

Purpose / Description

As described in #10378, the way decks are currently displayed in the "Deck Search" subscreen is not intuitive nor easy to understand, especially with multiple subdeck levels. This PR aims to make it more user friendly by indenting decks the same way as the main "Deck Viewer" screen.

EDIT: Also added a test for the same.

I feel the code can be better formatted and commented.

Fixes

Fixes #10378

Approach

instead of displaying the entire deck path, now only the subdeck name is displayed along with appropriate indentation. Deck and subdeck creation works just like before.

Before

After

Video showing creation of subdecks:

Video showing search capability:

WhatsApp.Video.2022-02-24.at.1.01.13.PM.mp4

How Has This Been Tested?

Emulator:
Pixel 5 API 32

Device:
Xaomi Redmi Note 8 Pro API 28 (Android 9)

Checklist

Please, go through these checks before submitting the PR.

You have not changed whitespace unnecessarily (it makes diffs hard to read)
You have a descriptive commit message with a short title (first line, max 50 chars).
Your code follows the style of the project (e.g. never omit braces in if statements)
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

instead of displaying the entire deck path, now only the subdeck name is displayed along with appropriate indentation. Deck and subdeck creation works just like before.

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

david-allison

You asked for a thorough review. Hope this helps!

My only other comment would be that it'd be great to get a unit test on .displayName, optional, but nice to have.

david-allison · 2022-02-24T11:58:40Z

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

        constructor(deckId: Long, name: String) {
            this.deckId = deckId
            this.name = name
+            this.displayName = getDisplayName(name)


We typically want to avoid performing work in a constructor. I'd advise having displayName as a get() property

Potentially use lazy { } so it's only evaluated once: https://kotlinlang.org/docs/delegated-properties.html#lazy-properties

Is using get() like this fine?

val displayName: String get() { return getDisplayName(name) }

Yes, the only issue is that displayName is calculated more than once if it's called more than once

I tried writing this as:

val displayName: String by lazy { getDisplayName(name)}

But, it gave me an error: Variable 'name' must be initialized, but name is already initialised, so I don't know what's wrong.

Lazy option: Go with get() and leave a comment that it should be a lazy

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

viciousAegis · 2022-02-24T12:24:39Z

You asked for a thorough review. Hope this helps!

My only other comment would be that it'd be great to get a unit test on .displayName, optional, but nice to have.

Yep, it does! I'll make the changes soon. For the unit test, are there any docs I could look at to see how to get those? Don't know a lot about them.

krmanik · 2022-02-24T13:02:15Z

I had also difficulty in implementing test earlier but now I can implement test.
I have learned by viewing existing test class in AnkiDroid source code.

My general workflow for implementing test class

Create test class if not exist in src/test/java/com/ichi2/ with name end with Test
E.g. for TypeAnswer
src/main/java/com/ichi2/anki/cardviewer/TypeAnswer.kt

src/test/java/com/ichi2/anki/cardviewer/TypeAnswerTest.java
Add function name to test with @Test annotation, then use functions in MatcherAssert to test if result is equal to expected.

    @Test
    fun testMediaIsNotExpected() {
        // #0096 - Anki Desktop did not expect media.
        val input = "ya[sound:36_ya.mp3]<div><img src=\"paste-efbfdfbff329f818e3b5568e578234d0d0054067.png\" /><br /></div>"
        val expected = "ya"
        val actual: String = cleanCorrectAnswer(input)
        MatcherAssert.assertThat(actual, Matchers.equalTo(expected))
    }

Docs
https://junit.org/junit5/docs/current/user-guide/
https://developer.android.com/training/testing/local-tests
https://www.vogella.com/tutorials/JUnit/article.html

May be I missed something above, so google it or chat in discord.

viciousAegis · 2022-02-24T13:18:32Z

@krmanik That is really helpful! Thank you so much! I will let you know if I have other doubts.

refactored comments as suggested. Optimized function `getDisplayName()` as suggested. `displayName` is now a val not a var, and is a `get()` property. to do next: add test.

viciousAegis · 2022-02-25T19:47:03Z

Resolved all the changes suggested. PR not final, will add a test in the next commit. (Or, can add a test in another PR, either way fine with me. )

david-allison

Beautiful

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

viciousAegis · 2022-02-25T20:48:05Z

Started working on the test, I think I'm making good progress. Will address the comments along with it. It should be done by tomorrow. If so, we can add it do this PR, otherwise, will let you know! :)

viciousAegis · 2022-02-26T05:37:14Z

I made a test file, which looks like this:

@RunWith(AndroidJUnit4::class)
class DeckSelectionDialogTest: RobolectricTest() {

    @Test
    fun verifyDeckDisplayName() {
        val input = "deck::sub-deck::sub-deck2::sub-deck3"
        val expected = "\t\t\t\t\t\tsub-deck3"

        val deck = SelectableDeck(1234, input)
        val actual: String = deck.displayName

        assertThat(actual, Matchers.equalTo(expected))
    }
}

When I try to run this test (or any other test) like ./gradlew AnkiDroid:testPlayDebugUnitTest --tests com.ichi2.anki.dialogs.DeckPickerContextMenuTest, the build is failing with the error message:

com.ichi2.anki.dialogs.DeckPickerContextMenuTest > delete_deck_is_last_in_menu_issue_10283 FAILED
    java.lang.UnsatisfiedLinkError: /private/var/folders/5b/1q9_lqrs79ng7n9hjm_xl4cm0000gn/T/librsdroid2032698931526253765.dylib: dlopen(/private/var/folders/5b/1q9_lqrs79ng7n9hjm_xl4cm0000gn/T/librsdroid2032698931526253765.dylib, 0x0001): tried: '/private/var/folders/5b/1q9_lqrs79ng7n9hjm_xl4cm0000gn/T/librsdroid2032698931526253765.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64e')), '/usr/lib/librsdroid2032698931526253765.dylib' (no such file)

Is this a problem with my machine? How should I run these tests otherwise?

david-allison · 2022-02-26T20:55:51Z

@viciousAegis This is a bug on our side. We do not build io.github.david-allison-1:anki-android-backend-testing for M1 Macs.

I have an M1 Mac arriving sometime late March, so I'll be personally invested in testing

As a workaround:

Remove @RunWith(AndroidJUnit4::class)
Remove :RobolectricTest()
This will only work because the test does not depend on the Anki Database

As a workaround, please enable GitHub actions on your GitHub clone of Anki-Android and test there

Properly fixing this

Issue

We have a library written in rust: librsdroid. This is compiled for Android
Robolectric doesn't run Android, it runs tests via the JVM on your PC
Therefore, librsdroid needs to be compiled for native platforms
We have a separate library: anki-android-backend-testing which contains the dlls and dylibs needed
When this was written, M1 macs didn't exist

Please see: https://github.com/ankidroid/Anki-Android-Backend/tree/main/rsdroid-testing

First of all, this isn't well documented. Please submit a PR with better documentation if you want
Most of the work is performed via GitHub actions
Feel free to submit a pull request to the GitHub actions get this working.
- Compile the dylib for arm64e (I have no idea how to do this, but it should be possible)
  - The code should be: https://github.com/ankidroid/Anki-Android-Backend/blob/9302b3f89e09643e95c42166743f8563a3822ddc/rsdroid-testing/build.gradle#L184-L202
- Include two dylibs in the jar
- Fix this code to select the correct dylib based on your architecture:
  - https://github.com/ankidroid/Anki-Android-Backend/blob/9302b3f89e09643e95c42166743f8563a3822ddc/rsdroid-testing/src/main/java/net/ankiweb/rsdroid/testing/RustBackendLoader.java#L55-L64

mikehardy · 2022-02-26T22:16:23Z

Alternatively: run the test somehow under Rosetta2?

Github Actions does not have any M1 runners unfortunately, and no concrete plans to add them.

I have an M1 mac though and can likely generate the libraries if it had to build on an M1 mac.

I think cross-compiling is possible though, to an M1 mac target (arm64e as you mention) from an intel macos github action runner assuming it has the right toolchain stuff setup, that's what the code link above seems to do, and it should be a good path forward

Not sure if there are better ways to do this but here's a shell way to detect architecture (and Rosetta2 execution status, if needed), may help

if [ "$(uname)" == "Darwin" ]; then
  # We do not want to run under Rosetta 2, brew doesn't work and compiles might not work after
  arch_name="$(uname -m)"
  if [ "${arch_name}" = "x86_64" ]; then
    if [ "$(sysctl -in sysctl.proc_translated)" = "1" ]; then
      echo "Running on Rosetta 2"
      echo "This is not supported. Run \`env /usr/bin/arch -arm64 /bin/bash --login\` then try again"
      exit 1
    else
      echo "Running on native Intel"
    fi
  elif [ "${arch_name}" = "arm64" ]; then
    echo "Running on ARM"
  else
    echo "Unknown architecture: ${arch_name}"
  fi
fi

david-allison · 2022-02-27T01:35:23Z

We should be able to cross-compile Rust for arm64e. This /should/ be doable by adding aarch64-apple-darwin in:

https://github.com/ankidroid/Anki-Android-Backend/blob/9302b3f89e09643e95c42166743f8563a3822ddc/rsdroid-testing/build.gradle#L109-L114

and installing the target platform: https://github.com/ankidroid/Anki-Android-Backend/blob/9302b3f89e09643e95c42166743f8563a3822ddc/.github/scripts/install_rust_robolectric_targets.sh

And then:

An option in the .java to detect an M1 and pull the correct dylib
A fix to to check_robolectric_assets.sh allow 2 dylibs in the .jar

Note that aarch64-apple-darwin is a Tier 2 target in Rust.

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt

viciousAegis · 2022-02-27T06:40:45Z

I think this PR should be complete now, will move onto figuring out implementing tests on the m1 mac. Please review one final time for the displayName test, and let me know if any changes are needed!

david-allison

Awesome!

Akshay0701 · 2022-02-27T14:32:38Z

Nice!

mikehardy · 2022-04-10T00:38:29Z

Hi there! FEBRUARY OpenCollective Notice

Just a friendly notice that we try to process OpenCollective payments monthly - it's time for February 2022 submissions (I'll do my best to follow this with the same process for March in a week)

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

(I only post one comment per person to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for in the month of January)

Thanks!

"Deck Search" display updated

898bdfc

instead of displaying the entire deck path, now only the subdeck name is displayed along with appropriate indentation. Deck and subdeck creation works just like before.

krmanik reviewed Feb 24, 2022

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt Outdated Show resolved Hide resolved

krmanik approved these changes Feb 24, 2022

View reviewed changes

krmanik added the Needs Second Approval Has one approval, one more approval to merge label Feb 24, 2022

david-allison requested changes Feb 24, 2022

View reviewed changes

mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Second Approval Has one approval, one more approval to merge labels Feb 24, 2022

Optimized function and refactored comments

122a157

refactored comments as suggested. Optimized function `getDisplayName()` as suggested. `displayName` is now a val not a var, and is a `get()` property. to do next: add test.

david-allison approved these changes Feb 25, 2022

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt Show resolved Hide resolved

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt Outdated Show resolved Hide resolved

david-allison added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Author Reply Waiting for a reply from the original author labels Feb 25, 2022

david-allison reviewed Feb 27, 2022

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/DeckSelectionDialog.kt Outdated Show resolved Hide resolved

viciousAegis added 2 commits February 27, 2022 11:53

code refactoring

a13268a

Added test for displayName

145381f

viciousAegis requested a review from david-allison February 27, 2022 06:40

david-allison approved these changes Feb 27, 2022

View reviewed changes

david-allison removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Feb 27, 2022

david-allison added this to the 2.16 release milestone Feb 27, 2022

david-allison merged commit 5959c1e into ankidroid:master Feb 27, 2022

viciousAegis deleted the deckSearchUpdate branch February 28, 2022 06:44

mikehardy mentioned this pull request Feb 28, 2022

[Bug] Apple Silicon test failures with x86_64 JDK on macOS #10403

Closed

4 tasks

viciousAegis mentioned this pull request Mar 16, 2022

Added delete note button in the Previewer (Help Wanted) #10538

Closed

7 tasks

mikehardy mentioned this pull request Mar 16, 2022

AnkiDroid unit test suite currently not working on arm64 JDKs on macOS ankidroid/Anki-Android-Backend#164

Closed

viciousAegis mentioned this pull request Jun 25, 2022

[GSoC] Filter Bottom Sheet implementation #11663

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Deck Search" display now same as the Deck Viewer #10389

"Deck Search" display now same as the Deck Viewer #10389

viciousAegis commented Feb 24, 2022 •

edited

Loading

david-allison left a comment •

edited

Loading

david-allison Feb 24, 2022

viciousAegis Feb 25, 2022

david-allison Feb 25, 2022 •

edited

Loading

viciousAegis Feb 25, 2022 •

edited

Loading

david-allison Feb 25, 2022

viciousAegis commented Feb 24, 2022

krmanik commented Feb 24, 2022

viciousAegis commented Feb 24, 2022

viciousAegis commented Feb 25, 2022

david-allison left a comment

viciousAegis commented Feb 25, 2022

viciousAegis commented Feb 26, 2022

david-allison commented Feb 26, 2022

mikehardy commented Feb 26, 2022 •

edited

Loading

david-allison commented Feb 27, 2022 •

edited

Loading

viciousAegis commented Feb 27, 2022

david-allison left a comment

Akshay0701 commented Feb 27, 2022

mikehardy commented Apr 10, 2022

"Deck Search" display now same as the Deck Viewer #10389

"Deck Search" display now same as the Deck Viewer #10389

Conversation

viciousAegis commented Feb 24, 2022 • edited Loading

Pull Request template

Purpose / Description

Fixes

Approach

Before

After

How Has This Been Tested?

Checklist

david-allison left a comment • edited Loading

Choose a reason for hiding this comment

david-allison Feb 24, 2022

Choose a reason for hiding this comment

viciousAegis Feb 25, 2022

Choose a reason for hiding this comment

david-allison Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

viciousAegis Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

david-allison Feb 25, 2022

Choose a reason for hiding this comment

viciousAegis commented Feb 24, 2022

krmanik commented Feb 24, 2022

viciousAegis commented Feb 24, 2022

viciousAegis commented Feb 25, 2022

david-allison left a comment

Choose a reason for hiding this comment

viciousAegis commented Feb 25, 2022

viciousAegis commented Feb 26, 2022

david-allison commented Feb 26, 2022

Properly fixing this

mikehardy commented Feb 26, 2022 • edited Loading

david-allison commented Feb 27, 2022 • edited Loading

viciousAegis commented Feb 27, 2022

david-allison left a comment

Choose a reason for hiding this comment

Akshay0701 commented Feb 27, 2022

mikehardy commented Apr 10, 2022

viciousAegis commented Feb 24, 2022 •

edited

Loading

david-allison left a comment •

edited

Loading

david-allison Feb 25, 2022 •

edited

Loading

viciousAegis Feb 25, 2022 •

edited

Loading

mikehardy commented Feb 26, 2022 •

edited

Loading

david-allison commented Feb 27, 2022 •

edited

Loading