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

#94 fixed event bus related app crushing issue on Android 5.0 Lollipop #96

Merged
merged 3 commits into from Sep 15, 2020

Conversation

bluetoothfx
Copy link
Contributor

@bluetoothfx bluetoothfx commented Aug 22, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

This PR is related to DEV android app. If the application open from android version 5.0 (Lollipop) it crashes immediately & failed to perform application normal activity & shows Error Message "Unfortunately DEV stop working".

What was the error:
I have observed that the project was failing to initiate Event Bus. To properly initiate Event Bus I have followed the guideline
to implement it. Which is available here.

I need to add annotation processor which was lacking in this project. Which is highly recommended by Greenrobot team.
Which is mentioned here.
So I have made an application class and put them centrally & found that application working as expected.

Related Tickets & Documents

Please check following ticket to get details.
#94

Screenshots/Recordings (if there are UI changes)

N/A

[optional] What gif best describes this PR or how it makes you feel?

alt_text

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@fdocr fdocr left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @bluetoothfx and sorry for the delay in reviewing.

While testing this I saw the crash reported on #98 and also forem#9809. Apparently when autofill takes control (during login) it calls the observeNetwork() method, which in turn calls EventBus.getDefault().register(this) and crashes (see screenshot below).

I think this can easily be fixed by calling EventBus.getDefault().unregister(this) right before EventBus.getDefault().register(this) inside the observeNetwork() method from CustomWebViewClient.kt. Since this PR refactors the EventBus implementation can you please include this one line for the fix as well? Thanks again for the help.

Screen Shot 2020-09-14 at 13 14 22

@@ -0,0 +1,25 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this file needs to be added, it can even be included in .gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added on .gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove this .idea/codeStyles/Project.xml also? I have updated kotlin and it appeared.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that we already had that one in source control. I think for now it's best if we do a separate and thorough PR for working out the files to include/ignore

@bluetoothfx
Copy link
Contributor Author

@fdoxyz should I check before unregister or just straight unregister ?

if(EventBus.getDefault().isRegistered(this)) 
    EventBus.getDefault().unregister(this)

@fdocr
Copy link
Contributor

fdocr commented Sep 15, 2020

I think the check would work great 👍

Copy link
Contributor

@fdocr fdocr left a comment

Choose a reason for hiding this comment

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

Tested this and works great, including the suggested bug fix. Thank you again for the contribution @bluetoothfx

@fdocr fdocr merged commit abd21b4 into forem:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants