This repository was archived by the owner on Aug 12, 2022. It is now read-only.
Split iOS and Android interfaces #284
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisgarrity
commented
Aug 6, 2020
src/editor/ScratchJr.js
Outdated
| } | ||
| // // XXX: does this ever happen? | ||
| // // I'm pretty sure this is dead code -TM | ||
| // static saveProjectState () { |
Contributor
Author
There was a problem hiding this comment.
I was sure enough that it's dead code to just comment it out. Probably should just remove it completely.
Contributor
There was a problem hiding this comment.
Yeah, I can't find anything that points to it, I say go for it and delete!
Refactor the native interface in preparation for switching iOS to use WKWebview. * Finally rename the folder for device specific interfaces as `tablet` instead of `iPad` * Update `import` statements to use the new name * Create new `iOS.js` and `Android.js` based on previous `iPad/iOS.js` to separate the interfaces * Add new `OS.js` class to manage the class variables, initialize the device interface delegate methods to the correct interface. * refactor how `utils/lib` detects the current platform based on `navigtor.userAgent` based on https://stackoverflow.com/questions/37591279/detect-if-user-is-using-webview-for-android-ios-or-a-regular-browser. previous method relied on the Android interface being loaded or not. It can be difficult to detect the difference between in a browser and in a webview, but for now ScratchJr doesn’t need to worry about running in a browser
chrisgarrity
force-pushed
the
issue/274-refactor-os-interface
branch
from
0827c6b to
1420a59
Compare
chrisgarrity
commented
Aug 10, 2020
| } | ||
| var me = this; | ||
| var url = (MediaLib.keys[name]) ? MediaLib.path + name : (name.indexOf('/') < 0) ? iOS.path + name : name; | ||
| var url = (MediaLib.keys[name]) ? |
Contributor
Author
There was a problem hiding this comment.
this was just fixing a lint error
Android Studio added some more config files.
src/tablet/OS.js
Outdated
| tabletInterface.getmedia(file, fcn); | ||
| } | ||
|
|
||
| // static getmediadata (key, offset, len, fcn) { |
Contributor
There was a problem hiding this comment.
similar comment to the one I left above: could you add a note about when it would be appropriate to delete this code, so it doesn't stay in limbo forever?
|
|
||
| // // Called on the Objective-C side. The argument is a base64-encoded .SJR file, | ||
| // // to be unzipped, processed, and stored. | ||
| // static loadProjectFromSjr (b64data) { |
src/tablet/iOS.js
Outdated
|
|
||
| export default class iOS { | ||
|
|
||
| // waitForInterface is defined in OS, and should make sure that the correct |
|
|
||
| // // Web Wiew delegate call backs | ||
| // | ||
| // static pageError (desc) { |
| // safari = /safari/.test( userAgent ), // currently do not need to detect browser vs webview | ||
| // android = /android/.text.(userAgent); | ||
|
|
||
| if( ios ) { |
Contributor
There was a problem hiding this comment.
I think this would be more consistent with our style:
Suggested change
| if( ios ) { | |
| if (ios) { |
Contributor
|
LGTM, with a few suggestions that are minor! |
removing dead code that was just commented in the previous commit,
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves
Preparation for #274
Proposed Changes
Refactor the native interface in preparation for switching iOS to use WKWebview.
tabletinstead ofiPadimportstatements to use the new nameiOS.jsandAndroid.jsbased on previousiPad/iOS.jsto separate the interfacesOSconstant toutils/libthat records the current platform based onnavigtor.userAgentbased on https://stackoverflow.com/questions/37591279/detect-if-user-is-using-webview-for-android-ios-or-a-regular-browserIt can be difficult to detect the difference between in a browser and in a webview, but for now ScratchJr doesn’t need to worry about running in a browser
Reason for Changes
Having all the device dependent files named
iPadandiOSeven when they applied to Android has been a source of confusion for a very long time. The way that iOS WKWebview and Android webview handle communications between javascript and the native side is quite different. Having them be two completely separate interfaces allows us to handle them differently as needed - for example, deciding when things need to beJSON.stringifiedand when the interface will take care of it.Test Coverage
Manual testing:
*able to create projects, add pages, add sprites, add backgrounds, add sound, add text, use camera in paint editor, export via email