DeckOption request the user to confirm whether to discard only if there are local changes #17315

Arthur-Milchior · 2024-10-27T22:45:29Z

NF: Simplify the usage of bridgeCommand in PageFragment
bridgeCommand is the general process by which Anki's webview send
command to the webview's owner. In Anki, it's interpreted by
Python. In AnkiDroid it must be interpreted by Kotlin.

Currently, the only bridgeCommand we listen to are in
CongratsPage. And I'm really thankful @BrayanDSO for creating this
first usage!

When anki 24.10 beta 2 is used in ankidroid, the DeckOptions will also
send bridgeCommand. And, I expect that when we'll use more webview,
more bridgeCommand will arrive.

So, I decided to generalize Brayan's code and ensure that there is a
simple map that any PageFragment can override that would allow to
declare its bridgeCommand without needing to add copy
CongratsPage's code.

I tested this manually, and the CongratsPage's button still works.

mikehardy

love the idea, implementation seems good but will defer to anyone better at Kotlin for taste and to Brayan for idea-consistency with what he was thinking

david-allison · 2024-10-28T07:32:20Z

@voczi FYI

BrayanDSO

It works for me, but I agree with David's comments.

And, I expect that when we'll use more webview, more bridgeCommand will arrive.

About that, @dae himself said that he wants to avoid bridgeCommand in new code.

If this is new code and still uses bridgeCommand, I'm honestly disappointed.

Arthur-Milchior · 2024-10-28T15:40:48Z

I was referring to https://github.com/ankitects/anki/pull/3410/files , which introduces changes in svelte I wanted to use (and then I realize that, even if it’s available in ankidroid-backend, it’s not in ankidroid yet).

It does not introduce new bridgeCommand (i recalled it did, I was wrong). It still uses the deckOptionsReady to state that the webview is available to let python knows that it can send request to the webview about whether the data was modified.
I would have appreciated to use it.

But I guess I can just ignore this bridge command, assume that when the page is loaded, it can accept request. And any result that is not a boolean means the page was not loaded so we can’t close it.

Still, given that bridgeCommand already exists and does not seems to be going to be deleted soon, I still suspect this PR is useful.

I’ll apply David’s change when I’m on my home computer

Arthur-Milchior · 2024-11-08T14:50:58Z

oh, I finally figured out why I was confused. New bridge commands were added in https://github.com/ankitects/anki/pull/3495/files
And I need it to fix a bug in ankidroid

Arthur-Milchior · 2024-11-08T15:49:25Z

So I applied @david-allison 's requested change, and then added an extra commit to solve the deck option issue.

If we eventually move away from the bridge, this will need to be updated.

Arthur-Milchior · 2024-11-08T15:50:38Z

Given the extra commit, I removed @mikehardy 's approval.
The nice thing here being that this show why the first two commit is useful. It's not only for code cleanliness

Arthur-Milchior · 2024-11-09T04:01:29Z

Does anyone know whether PageFragments actually load their webcontent in RobolectricTest ?
Trying to run the test with the debugger, it seems clear that the BridgeCommand is not executed. I don't know whether it is because we should wait more or because the js part is not executed.

I tried adding


            runBlocking{
                delay(2000)
            }

after pressing back, and it does not solve anything. So I assume it's a problem with the JS part

mikehardy · 2024-11-09T13:37:51Z

As discussed on Discord, the WebView is a "shadow" (that is: fake) implementation in robolectric.
The APIs are there but they largely do nothing. No content is loaded, javascript is definitely not executed.
For WebView-related tests where the WebView is expected to actually do things, it needs to be an androidTest on device

stale review

Arthur-Milchior · 2024-11-11T01:37:03Z

I removed the unit test then. There is no relevant way to do them. Unless I I start to mock the webview behavior, and that does not seems interesting things to do. In particular because this would break as soon as the deck option's TS code change

For some reason I don't understand, when I am in the deck picker, with instrumented tests, long click fail to open the menu. I don't know whether it just entirely fail to be registered, or whether the instrumented test don't open the menu for some reason.
In any case, opening the menu of actions from deck is too hard, so I would prefer not to block this useful feature, unless someone is competent enough with espresso/instrumented test to help me understand what's wrong with it

Arthur-Milchior · 2024-11-11T08:16:48Z

Rebased on top of #17419 so that I can reuse some deck picker instrumented tests function

BrayanDSO · 2024-11-11T19:36:47Z

Please add some around my GitHub nickname in the NF: Simplify the usage of bridgeCommand in PageFragment
commit description, so I don't get a ping every time you force push. i.e. use @BrayanDSO, not @BrayanDSO

Arthur-Milchior · 2024-11-11T20:45:49Z

Done, sorry

david-allison

Main question is regarding functionality + manual testing

And potential issues two classes with the same JavaScriptInterface, I'd avoid this by renaming the "ankidroid" interface we add in DeckOptions

Really appreciate the effort which went into re-testing this, thanks!!

AnkiDroid/src/androidTest/java/com/ichi2/anki/DeckOptionsTest.kt

david-allison · 2024-11-11T21:35:09Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/DeckOptionsTest.kt

+
+    @Before
+    fun before() {
+        assumeFalse("Test flaky in CI - #9282, skipping", wasBuiltOnCI())


This issue is closed

I reopened it. There are still multiple disabled tests referring to this issue

AnkiDroid/src/androidTest/java/com/ichi2/anki/DeckOptionsTest.kt

david-allison · 2024-11-11T21:37:18Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/DeckOptionsTest.kt

+    }
+
+    @Test
+    fun testDeckOptionsWithChangeShowAlert() {


This test has no assertions, and is therefore difficult to understand the intention

Actually there are assertions in openDeckOptions.
But the most important one is that the click can occurs, which means the buttons indeed exists. I don't know exactly how to state this with assertion

david-allison · 2024-11-11T21:38:07Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/DeckOptionsTest.kt

+
+        // Close without change
+        pressBack()
+        assertWebViewDoesNotExists()


Don't you want to assert that either: the fragment/activity is no longer loaded, or the user has gone back to the previous screen.

The WebView not existing doesn't seem to be a good proxy for this

I tried that at first. But somehow searching for the presence of a view whose id is R.layout.page_fragment does not work. It is never found. Instead, the log displaying the current view on the screen just shows me a WebView without PageFragment anywhere

david-allison · 2024-11-11T21:40:51Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/DeckPicker.kt

+ * You should have received a copy of the GNU General Public License along with         *
+ * this program.  If not, see <http://www.gnu.org/licenses/>.                           *
+ ****************************************************************************************/
+package com.ichi2.anki


This feels like it should be in a utility package

Done. Actually it was done in the PR #17419 that I use in two PRs.

david-allison · 2024-11-11T21:43:46Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/tests/InstrumentedTest.kt

+        try {
+            check(viewAssertion)
+            return
+        } catch (e: Throwable) {


Q: is this a bug. I would expect a ViewNotFound to be retried, but an assertion failure should exit the loop

May I suggest moving the discussion to #17419. As this is the commit that is common to two PRs where I need to do instrumented test.

This function is not changed at all. If it must be corrected, I think this is out of scope here.
What mattered to me is that I could use it without having to be in a subclass of InstrumentedTest

AnkiDroid/src/main/java/com/ichi2/anki/pages/DeckOptions.kt

david-allison · 2024-11-11T21:48:15Z

AnkiDroid/src/androidTest/java/com/ichi2/anki/DeckOptionsTest.kt

+import org.junit.Rule
+import org.junit.Test
+
+class DeckOptionsTest : InstrumentedTest() {


⚠️

These tests don't seem to handle either of the following cases. Did you manually test these:

User navigated to the Anki Manual via a link, and pressed 'back'

WebView should navigate back to options (changes are persisted), options should remain open

User opened up a modal on the svelte page and pressed 'back'

modal should close, options should remain open

Currently html generated by svelte has no id. It's pretty hard to select something at least if I want to be sure that random change in svelte code won't break the test.
As far as I can tell, it's not even tested upstream.

I manually tested it. When the manual or a link from it is opened, and I press back, either on the screen or the phone button, the deck options remain open and the manual get closed.

I asked about test upstream. Maybe I'll have to ask permission to add id. Right now, I don't think writing those test is worth it.

AnkiDroid/src/main/java/com/ichi2/anki/pages/PageFragment.kt

I also remove `OnPageFinishedCallback` because I don't really see the point in this list in Kotlin. I keep the interface as it's used in AbstractFlashcardViewer.

The names were far from clear

`bridgeCommand` is the general process by which Anki's webview send command to the webview's owner. In Anki, it's interpreted by Python. In AnkiDroid it must be interpreted by Kotlin. Currently, the only `bridgeCommand` we listen to are in CongratsPage. And I'm really thankful `@BrayanDSO` for creating this first usage! When anki 24.10 beta 2 is used in ankidroid, the DeckOptions will also send `bridgeCommand`. And, I expect that when we'll use more webview, more `bridgeCommand` will arrive. So, I decided to generalize Brayan's code and ensure that there is a simple map that any PageFragment can override that would allow to declare its `bridgeCommand` without needing to add copy `CongratsPage`'s code. I tested this manually, and the CongratsPage's button still works.

This use a recent change introduced upstream. Unit test can't run the webview. So it has to be replaced by instrumented tests. Test fails on CI while working locally, so I have to assume it's not CI.

Arthur-Milchior requested a review from BrayanDSO October 27, 2024 22:45

mikehardy approved these changes Oct 27, 2024

View reviewed changes

mikehardy added Needs Second Approval Has one approval, one more approval to merge Anki Ecosystem Compatibility labels Oct 27, 2024

This comment was marked as resolved.

Sign in to view

david-allison added the Needs Author Reply Waiting for a reply from the original author label Oct 28, 2024

BrayanDSO reviewed Oct 28, 2024

View reviewed changes

Arthur-Milchior force-pushed the bridgeCommand branch from efa539c to 761263b Compare November 8, 2024 15:48

Arthur-Milchior changed the title ~~NF: Simplify the usage of bridgeCommand in PageFragment~~ DeckOption request the user to confirm whether to discard only if there are local changes Nov 8, 2024

Arthur-Milchior requested a review from mikehardy November 8, 2024 15:50

Arthur-Milchior removed Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge labels Nov 8, 2024

Arthur-Milchior force-pushed the bridgeCommand branch 2 times, most recently from c02bb90 to 90cdf03 Compare November 9, 2024 03:55

Arthur-Milchior force-pushed the bridgeCommand branch from 90cdf03 to d56a2ac Compare November 10, 2024 04:39

david-allison added Needs Author Reply Waiting for a reply from the original author Needs Review labels Nov 10, 2024

Arthur-Milchior force-pushed the bridgeCommand branch from d56a2ac to 26752dd Compare November 11, 2024 01:32

Arthur-Milchior force-pushed the bridgeCommand branch 2 times, most recently from 66092be to 38bbf5d Compare November 11, 2024 08:16

Arthur-Milchior removed the Needs Author Reply Waiting for a reply from the original author label Nov 11, 2024

Arthur-Milchior force-pushed the bridgeCommand branch from 38bbf5d to d815373 Compare November 11, 2024 16:25

Arthur-Milchior force-pushed the bridgeCommand branch 2 times, most recently from 06efeab to 7a57ae0 Compare November 11, 2024 21:05

david-allison requested changes Nov 11, 2024

View reviewed changes

github-actions bot added the Has Conflicts label Nov 11, 2024

Arthur-Milchior force-pushed the bridgeCommand branch from 7a57ae0 to 4902897 Compare November 12, 2024 01:33

Arthur-Milchior added 4 commits November 15, 2024 03:06

NF: PageWebViewClients onPageFinished can take multiple callbacks.

6f7ab27

I also remove `OnPageFinishedCallback` because I don't really see the point in this list in Kotlin. I keep the interface as it's used in AbstractFlashcardViewer.

NF: rename and document the callback

d5af08b

The names were far from clear

Deck Options request confirmation iff there were changes

488031b

This use a recent change introduced upstream. Unit test can't run the webview. So it has to be replaced by instrumented tests. Test fails on CI while working locally, so I have to assume it's not CI.

Arthur-Milchior force-pushed the bridgeCommand branch from 4902897 to 488031b Compare November 15, 2024 02:06

Arthur-Milchior removed the Has Conflicts label Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DeckOption request the user to confirm whether to discard only if there are local changes #17315

DeckOption request the user to confirm whether to discard only if there are local changes #17315

Arthur-Milchior commented Oct 27, 2024

mikehardy left a comment

This comment was marked as resolved.

david-allison commented Oct 28, 2024

BrayanDSO left a comment •

edited

Loading

Arthur-Milchior commented Oct 28, 2024

Arthur-Milchior commented Nov 8, 2024

Arthur-Milchior commented Nov 8, 2024

Arthur-Milchior commented Nov 8, 2024

Arthur-Milchior commented Nov 9, 2024

mikehardy commented Nov 9, 2024

Arthur-Milchior commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

BrayanDSO commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

david-allison left a comment

david-allison Nov 11, 2024

Arthur-Milchior Nov 11, 2024

david-allison Nov 11, 2024

Arthur-Milchior Nov 11, 2024

david-allison Nov 11, 2024

Arthur-Milchior Nov 11, 2024

david-allison Nov 11, 2024

Arthur-Milchior Nov 12, 2024

david-allison Nov 11, 2024

Arthur-Milchior Nov 12, 2024

david-allison Nov 11, 2024 •

edited

Loading

Arthur-Milchior Nov 11, 2024

DeckOption request the user to confirm whether to discard only if there are local changes #17315

Are you sure you want to change the base?

DeckOption request the user to confirm whether to discard only if there are local changes #17315

Conversation

Arthur-Milchior commented Oct 27, 2024

mikehardy left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

david-allison commented Oct 28, 2024

BrayanDSO left a comment • edited Loading

Choose a reason for hiding this comment

Arthur-Milchior commented Oct 28, 2024

Arthur-Milchior commented Nov 8, 2024

Arthur-Milchior commented Nov 8, 2024

Arthur-Milchior commented Nov 8, 2024

Arthur-Milchior commented Nov 9, 2024

mikehardy commented Nov 9, 2024

Arthur-Milchior commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

BrayanDSO commented Nov 11, 2024

Arthur-Milchior commented Nov 11, 2024

david-allison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrayanDSO left a comment •

edited

Loading

david-allison Nov 11, 2024 •

edited

Loading