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

Feat[Multimedia]: render svg using webview #17035

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

criticalAY
Copy link
Contributor

@criticalAY criticalAY commented Sep 7, 2024

Purpose / Description

Render SVG instead of showing a warning text.

Blocked by

Fixes

Approach

Use a webview to render an SVG in it

How Has This Been Tested?

Tested on OnePlus Nord CE:
image

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

Copy link
Contributor

github-actions bot commented Sep 7, 2024

Message to maintainers, this PR contains strings changes.

  1. Before merging this PR, it is best to run the "Sync Translations" GitHub action, then make and merge a PR from the i18n_sync branch to get translations cleaned out.
  2. Then merge this PR, and immediately do another translation PR so the huge change made by this PR's key changes are all by themselves.

Read more about updating strings on the wiki,

@criticalAY criticalAY added Blocked by dependency Currently blocked by some other dependent / related change Strings and removed Strings labels Sep 7, 2024
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Looks GREAT to me

@BrayanDSO BrayanDSO added the Needs Second Approval Has one approval, one more approval to merge label Sep 8, 2024
@BrayanDSO
Copy link
Member

Question: does this work for other formats?

Because animated PNGs are static in the ImageView, but animated when the card is opened

@criticalAY
Copy link
Contributor Author

Question: does this work for other formats?

No this method would require workarounds for different images, we can directly use webview to preview images

@BrayanDSO
Copy link
Member

we can directly use webview to preview images

So, is it possible to use the WebView instead of an ImageView to preview all the images?

That should handle all the images that can be rendered in a flashcard

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.

this does seem like a fantastic opportunity to have a single way to do things vs conditionals

what if all images went through the webview? can this PR be altered to do that?

also would be nice to move this along by removing the dependency blocker - I think that other PR is about ready to go so the first commit can drop here ?

@criticalAY
Copy link
Contributor Author

I will update it to use webview then /will rebase once the blocker goes in /

@mikehardy
Copy link
Member

this one is free / unblocked now 🥳

@mikehardy mikehardy removed the Blocked by dependency Currently blocked by some other dependent / related change label Oct 2, 2024
@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Oct 4, 2024
@criticalAY criticalAY force-pushed the feat-render-svg branch 2 times, most recently from 3380af6 to 654e231 Compare October 12, 2024 17:26
@criticalAY criticalAY added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Oct 12, 2024
@criticalAY criticalAY requested a review from mikehardy October 12, 2024 17:27
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.

This is looking really good. Love the "use webview for all the things" direction if we're going to be using a WebView at all

Was testing this and took a crash here - granted, unlikely perhaps in the wild but on an emulator, boom

  • add note (basic forward/reversed)
  • click paperclip
  • select gallery
10-12 12:58:40.367  4867  4867 D MultimediaImageFragment: MultimediaImageFragment:: Opening gallery
10-12 12:58:40.391   661   963 D CompatibilityChangeReporter: Compat change id reported: 182734110; UID 10193; state: ENABLED
10-12 12:58:40.392   509   608 D audioserver: FGS Logger Transaction failed
10-12 12:58:40.392   509   608 D audioserver: -129
10-12 12:58:40.399   661   963 I ActivityTaskManager: START u0 {act=android.intent.action.PICK dat=content://media/...} with LAUNCH_MULTIPLE from uid 10193 result code=-91
10-12 12:58:40.399  4867  4867 D AndroidRuntime: Shutting down VM
--------- beginning of crash
10-12 12:58:40.404  4867  4867 E AndroidRuntime: FATAL EXCEPTION: main
10-12 12:58:40.404  4867  4867 E AndroidRuntime: Process: com.ichi2.anki.debug, PID: 4867
10-12 12:58:40.404  4867  4867 E AndroidRuntime: android.content.ActivityNotFoundException: No Activity found to handle Intent { act=android.intent.action.PICK dat=content://media/... }
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at android.app.Instrumentation.checkStartActivityResult(Instrumentation.java:2239)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at android.app.Instrumentation.execStartActivity(Instrumentation.java:1878)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at android.app.Activity.startActivityForResult(Activity.java:5589)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at androidx.activity.ComponentActivity.startActivityForResult(ComponentActivity.kt:704)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at androidx.core.app.ActivityCompat.startActivityForResult(ActivityCompat.java:244)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at androidx.activity.ComponentActivity$activityResultRegistry$1.onLaunch(ComponentActivity.kt:225)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at androidx.activity.result.ActivityResultRegistry$register$2.launch(ActivityResultRegistry.kt:137)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at androidx.fragment.app.Fragment$10.launch(Fragment.java:3637)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at androidx.activity.result.ActivityResultLauncher.launch(ActivityResultLauncher.kt:37)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at com.ichi2.anki.multimedia.MultimediaImageFragment.openGallery(MultimediaImageFragment.kt:328)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at com.ichi2.anki.multimedia.MultimediaImageFragment.handleSelectedImageOptions(MultimediaImageFragment.kt:288)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at com.ichi2.anki.multimedia.MultimediaImageFragment.handleImageUri(MultimediaImageFragment.kt:280)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at com.ichi2.anki.multimedia.MultimediaImageFragment.onViewCreated(MultimediaImageFragment.kt:272)
10-12 12:58:40.404  4867  4867 E AndroidRuntime: 	at androidx.fragment.app.Fragment.performViewCreated(Fragment.java:3152)

The "take photo, crop it, see it in the preview both times" case worked well though
I did not test the "unsupported image type" case or the actual SVG case (yet)

@criticalAY
Copy link
Contributor Author

I did test it with svg, gif and normal pictures

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Has Conflicts labels Nov 20, 2024
@david-allison

This comment was marked as resolved.

@mikehardy
Copy link
Member

This'll take a conflict when #17467 merges regardless (I know because I had to rebase this on on that one in order to test some of the pathways, like even proceeding past the "do you want to compress?" dialog)

@criticalAY

This comment was marked as resolved.

@criticalAY criticalAY added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 23, 2024
@criticalAY
Copy link
Contributor Author

@david-allison ping for re-review

Comment on lines 609 to 608
return try {
context?.contentResolver?.openInputStream(uri)?.use { inputStream ->
inputStream.bufferedReader().readText()
}
} catch (e: Exception) {
Timber.w(e, "Error reading SVG from URI")
null
}
Copy link
Member

Choose a reason for hiding this comment

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

possibly higher performance (using StringBuffer etc) to use existing util

fun InputStream.convertToString(): String {

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.

If we're going to handle the WebView choking on large (okay: extremely huge) files separately that's fine by me. It did not crash which was good

So I'm +1 on this with a mild preference to delegate stream handling to centralized methods, as suggested in comment

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'd like to see Mike's change included

Mine are optional

Treat this as an approve when:

  • Mike's change is implemented
  • The change is tested

@criticalAY
Copy link
Contributor Author

@mikehardy @david-allison Done!

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.

REALLY clean, cheers!

@david-allison 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 Review labels Dec 5, 2024
@david-allison
Copy link
Member

@david-allison david-allison added this pull request to the merge queue Dec 6, 2024
Merged via the queue into ankidroid:main with commit e8c1e97 Dec 6, 2024
9 checks passed
Copy link
Contributor

github-actions bot commented Dec 6, 2024

Maintainers: Please Sync Translations to produce a commit with only the automated changes from this PR.

Read more about updating strings on the wiki,

@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Dec 6, 2024
@github-actions github-actions bot added this to the 2.21 release milestone Dec 6, 2024
Copy link
Contributor

github-actions bot commented Jan 2, 2025

Hi there @criticalAY! This is the OpenCollective Notice for PRs merged from 2024-12-01 through 2024-12-31

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!

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

Successfully merging this pull request may close these issues.

Display SVGs in the image import preview
5 participants