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

Catty-77: Migrate UIWebView to WKWebView #1292

Merged
merged 1 commit into from Apr 26, 2020

Conversation

neel-makhecha
Copy link
Contributor

@neel-makhecha neel-makhecha commented Apr 10, 2020

Updated UIWebView to WKWebView in HelpWebViewController.swift along with all delegates.

Your checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

  • Include the name of the Jira ticket in the PR’s title
  • Verify that the Jira ticket is in the status Ready for Development
  • Include a summary of the changes plus the relevant context
  • Choose the proper base branch (develop)
  • Confirm that the changes follow the project’s coding guidelines
  • Verify that the changes generate no compiler or linter warnings
  • Perform a self-review of the changes
  • Verify to commit no other files than the intentionally changed ones
  • Include reasonable and readable tests verifying the added or changed behavior
  • Confirm that new and existing unit tests pass locally
  • Check that the commits’ message style matches the project’s guideline
  • Stick to the project’s git workflow (rebase and squash your commits)
  • Verify that your changes do not have any conflicts with the base branch
  • After the PR, verify that all CI checks have passed
  • Post a message in the #catty Slack channel and ask for a code reviewer

@neel-makhecha neel-makhecha changed the title Catty Catty-77: Migrate UIWebView to WKWebView Apr 10, 2020
@m-herold
Copy link
Member

Jenkins, retest this please.

Copy link
Member

@m-herold m-herold left a comment

Choose a reason for hiding this comment

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

Thanks! I left a few (minor) suggestions directly at the affected lines of code. One major thing we need to address:

  • Click on "Help"
  • Click on the "hamburger" menu button
  • Click on Featured
  • Select any project
  • Click on "Download the project"
  • The app is crashing: *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Completion handler passed to -[Pocket_Code.HelpWebViewController webView:decidePolicyForNavigationAction:decisionHandler:] was called more than once'

src/Catty/ViewController/Help/HelpWebViewController.swift Outdated Show resolved Hide resolved
src/Catty/ViewController/Help/HelpWebViewController.swift Outdated Show resolved Hide resolved
@neel-makhecha
Copy link
Contributor Author

Hi Michael
I have implemented changes as suggested. Please review it.

@m-herold
Copy link
Member

Were you able to reproduce the NSInternalInconsistencyException as mentioned above?

@neel-makhecha
Copy link
Contributor Author

Were you able to reproduce the NSInternalInconsistencyException as mentioned above?

Yes, I was getting that exception but with a different reason
'NSInternalInconsistencyException', reason: 'Modifications to the layout engine must not be performed from a background thread after it has been accessed from the main thread.'

The exception with the reason you mentioned is fixed with a return statement in decidePolicyFor navigationAction , where it was calling completion handler more than once in certain conditions.

Another exception that I mentioned above is fixed with various changes in the way progressBar gets hidden and unhidden, when the activityIndicator is visible.

@neel-makhecha
Copy link
Contributor Author

Hi Michael,
I have made changes as per your suggestions and have made some more where required. Please review it.

@neel-makhecha
Copy link
Contributor Author

I'm sorry, I have a very bad habit of force unwrapping optionals. I have updated tests again fixing that.

@neel-makhecha
Copy link
Contributor Author

Hi Michael,
I have made changes as per your suggestions. Please review it.

@neel-makhecha neel-makhecha force-pushed the CATTY-77 branch 2 times, most recently from d1a7a20 to ca48be5 Compare April 18, 2020 18:24
Copy link
Member

@m-herold m-herold left a comment

Choose a reason for hiding this comment

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

When you test your implementation having internet turned off (or when running into a timeout), the corresponding alert is never shown and the loading view keeps spinning for ever. For this time please do not directly squash your commits but please add another commit (makes review of the large class easier). Thanks!

Copy link
Member

@m-herold m-herold left a comment

Choose a reason for hiding this comment

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

The app is still crashing after downloading a project from within the WKWebView. See one of my previous comments on how to reproduce (stay in the view for another ~10 seconds or so without doing anything).

@m-herold
Copy link
Member

@neel-makhecha any news on that pull request? thanks!

@neel-makhecha
Copy link
Contributor Author

@neel-makhecha any news on that pull request? thanks!

Yes! The first issue where the loading view kept spinning forever is resolved. I had implemented an incorrect delegate method. So that's solved. But I am still trying to figure out the the crash. I am currently working on that, it is occasional and don't happen every time. But hopefully will figure out that too very soon!

@neel-makhecha
Copy link
Contributor Author

Hi Michael,
I have done the changes addressing those issues. Please review.

@m-herold
Copy link
Member

Thank you, looks very good! Please squash your commits, then this pull request is ready to merge.

- Removed UIWebView from storyboard and added UIView for WKWebView
- Renamed FileManager to CBFileManager to avoid conflicts
- HelpWebViewController updated with new delegate methods for WKWebView
- Added a new test class HelpWebViewControllerTests
@neel-makhecha
Copy link
Contributor Author

Done!

@m-herold m-herold merged commit edb0aef into Catrobat:develop Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants