Skip to content
This repository was archived by the owner on Aug 12, 2022. It is now read-only.

Conversation

@chrisgarrity
Copy link
Contributor

@chrisgarrity chrisgarrity commented Aug 17, 2020

Resolves

Fixes #274

(Replaces https://github.com/LLK/scratchjr/pull/288 which github mysteriously insisted had merge conflicts, but we couldn't actually see any)

Proposed Changes

Many thanks to @yueyuzhao for the first version of upgrading to WKWebview in https://github.com/LLK/scratchjr/pull/283

Reason for Changes

UIWebView is deprecated and new versions that use it are no longer accepted by the App store.

Testing

This replaces the interface between javascript and iOS native calls. It will require extensive manual testing (on iOS and Android to verify that changes did not break anything)

  • creating projects
  • creating and reusing characters in the paint editor
  • creating and reusing backdrops
  • adding multiple pages
  • recording sounds
  • exporting projects (email and airdrop)
  • switching between apps on the device (does it sleep/restore without issues?)
  • Permissions are requested/set correctly
  • can use the camera while editing backdrops and characters (shape is filled, looks like the preview, can replace, switch cameras)
  • UI sounds all play correctly (click, swipe etc)
  • can have multiple sound playing

Upgrade webview to WKWebView with many thanks to @yueyuzhao

* Need to debug a problem saving thumbnails on iOS 9.3.5
* Needs much more testing
@yueyuzhao
Copy link
Contributor

Hi @chrisgarrity , thanks for your hard work and your message.

@benjiwheeler
Copy link
Contributor

I've been testing this on Android and XCode, and everything seems to work well so far!

- (void) callback:(NSString *)res {
NSString *js = nil;
if ([res hasPrefix:@"["] || [res hasPrefix:@"{"]) {
js = [NSString stringWithFormat:@"iOS.resolve('%@', %@);", self.callId, res];
Copy link
Contributor

Choose a reason for hiding this comment

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

So am I right in understanding that we either expect res to be like:

returnStr

  • or -

{someKey: "someVal", someOtherKey: ["str1", "str2", "str3"]}

?

What I wonder is, when there are arrays/objects within an array/object, are we sure the quotes will be right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's one other option:

["someVal", "anotherVal"]

but as far as I can tell, the only function that returns an array is the dir method in IO.m, and I cannot find any places that is used.

In general, I think there's a bigger refactor here where all data passed between javascript and native gets converted to string/base64 encoded consistently. Right now we're relying on the evaluateJavaScript to do the right thing.

@chrisgarrity chrisgarrity marked this pull request as draft August 28, 2020 12:31
@chrisgarrity
Copy link
Contributor Author

@yueyuzhao we're running into an issue where the scratchjr HTML, loaded from the app bundle directory, cannot display assets (svg, png) that have been saved in the Documents folder. This appears to be a security restriction for the wekwebview. BTW, it works fine in the simulator, it only stops working when you run it on an actual iPad. We're exploring different ways to solve it, but since you submitted the original PR, I wanted to give you a head's up on what's taking so long.

@yueyuzhao
Copy link
Contributor

Hi @chrisgarrity , there need a little change in the getAsset function in file IO.js.

src/iPad/IO.js

@yueyuzhao
Copy link
Contributor

My computer was broken several days ago, and I'm setting up my developing environments. Once I confirmed that IO.js is the key, I will let you know.

@yueyuzhao
Copy link
Contributor

Hi @chrisgarrity , there need a little change in the getAsset function in file IO.js.

src/iPad/IO.js

Hi @chrisgarrity , changing these lines in IO.js is not enough, I'm looking into this issue.

@yueyuzhao
Copy link
Contributor

@chrisgarrity , sorry for my testing not covering this. I fixed this issue on my computer, but It's late in China, I will submit a merge request tomorrow after more tests.

@yueyuzhao
Copy link
Contributor

yueyuzhao commented Aug 29, 2020

Hi @chrisgarrity , this is the patch file that fixes assets displaying issue.

patch.diff.txt

Tests:

  • display/create/edit/delete projects
  • create/edit/delete sprites
  • create/edit/delete backgrounds
  • recording/playing sounds
  • GUI sounds
  • taking pictures
  • share project via Airdrop
  • open project via Airdrop

Don’t pass NSNulls to Firebase. Paramaters must be string or integer, so use ‘undefined’ to match current values.
@chrisgarrity chrisgarrity marked this pull request as ready for review September 4, 2020 13:15
@benjiwheeler
Copy link
Contributor

LGTM! Tested on Android 5.0.2 tablet and iOS 9.x tablet.

@chrisgarrity chrisgarrity deleted the issue/274-wkwebview-rebase branch May 11, 2021 14:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to WKWebView on iOS

4 participants