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.

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

fix: reviewer fetch and XMLHttpRequest issues #16457

Merged
merged 2 commits into from
May 23, 2024

Conversation

BrayanDSO
Copy link
Member

Purpose / Description

#16140 was an experiment to return to the 2.16.5 way of loading media, which worked by using the collection.media directory as base url. The pros were that files load faster and the URL is constant, so localStorage works. The cons were that it needs to accept CORS requests and doesn't work with JS fetch and XMLHttpRequest

For now my goal is just to restore compatibility with all decks. The new performance is still quite good, just ain't the best one anymore. Some ways to improve performance can be investigated later, if necessary, but now compatibility is the priority.

Fixes

Approach

Revert #16140

Doesn't need a backend upgrade

How Has This Been Tested?

  • Seek works on videos
  • JS's fetch works
  • The JS api still works
loading.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

@BrayanDSO BrayanDSO added Needs Review squash-merge The pull request currently requires maintainers to "Squash Merge" labels May 22, 2024
@BrayanDSO BrayanDSO changed the title Fix reviewer fetch and XMLHttpRequest issues issues fix: reviewer fetch and XMLHttpRequest issues May 22, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Hey @BrayanDSO - do you think this is worth picking on to release-2.18?
Seems like it's worth it, to me

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author Queued for Cherry Pick to Stable Branch labels May 23, 2024
@mikehardy mikehardy added this to the 2.18.1 release milestone May 23, 2024
@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels May 23, 2024
@david-allison
Copy link
Member

When we next move on this change, we need better regression test coverage

A @NeedsTest somewhere relevant would be useful, just to collect the failure cases

@mikehardy
Copy link
Member

I'm doing picks now and this fixes regressions, I'm going to be assertive, I'm putting it on the release-2.18 pile

@mikehardy mikehardy merged commit 1246253 into ankidroid:main May 23, 2024
11 checks passed
@github-actions github-actions bot modified the milestones: 2.18.1 release, 2.19 release May 23, 2024
@github-actions github-actions bot removed Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge squash-merge The pull request currently requires maintainers to "Squash Merge" labels May 23, 2024
mikehardy pushed a commit that referenced this pull request May 23, 2024
* feat: handle audio and videos seeking in ViewerResourceHandler

basically restore what e55cae2 had deleted

* fix: use http base url in card viewers

(cherry picked from commit 1246253)
@mikehardy
Copy link
Member

Hmmm 🤔 I'm getting a persistent error in local testing on the release-2.18 branch and on main (./gradlew jacocoAndroidTestReport) on the JS scheduling test:

> Task :AnkiDroid:connectedPlayDebugAndroidTest

com.ichi2.anki.ReviewerTest > testCustomSchedulerWithCustomData[TestingAVD(AVD) - 14] FAILED 
        java.lang.AssertionError:
        Expected: <3000>

That seems suspiciously like the sort of thing that may break with changes in this area. It appears to work in CI though

Local environment is macOS 14.5 (current) with this debug info from emulator:

AnkiDroid Version = 2.19alpha0-debug (e605223cf0f22cf9eb0ee48b6d4e68f47205cc41)

Backend Version = 0.1.38-anki24.04.1 (24.04 429bc9e14cefb597646a0e1beac6ef140f226b6f)

Android Version = 14 (SDK 34)

ProductFlavor = play

Manufacturer = Google

Model = sdk_gphone64_arm64

Hardware = ranchu

Webview User Agent = Mozilla/5.0 (Linux; Android 14; sdk_gphone64_arm64 Build/UE1A.230829.036.A2; wv) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/113.0.5672.136 Mobile Safari/537.36

ACRA UUID = e3ff48c8-1d2f-4017-b648-30585f787ac1

FSRS Enabled = false

Crash Reports Enabled = false

I'm going to hold off on the 2.18.1 release as I'd like to get this in, and maybe it's something trivial ?
If it looks like it will take a while to investigate and fix please just say so - it's fine - and I'll do 2.18.1 without it as that still represents incremental improvement for users, and get a fix in for 2.18.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants