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

Fixes NullPointerExceptions due to callbacks in MainActivity #1039

Merged
merged 2 commits into from Mar 10, 2019
Merged

Fixes NullPointerExceptions due to callbacks in MainActivity #1039

merged 2 commits into from Mar 10, 2019

Conversation

sonusourav
Copy link
Contributor

Fixes #891 #895

Issues #891 and #895 have a common root to callbacks which gives NullPointerExceptions.
Changes:

  1. Added a function named CheckNull.
  2. CheckNull checks for NullPointerException for views in MainActivity.

Screenshots
checknull

@soloturn
Copy link
Contributor

soloturn commented Mar 2, 2019

is this overlapping with #1038 ?

Copy link
Contributor

@soloturn soloturn left a comment

Choose a reason for hiding this comment

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

checks for null are there. if i view such code i understand @abdulwd suggesting to migrate to kotlin ;)

@soloturn
Copy link
Contributor

soloturn commented Mar 2, 2019

@sonusourav @abdulwd @prajurock is there a design problem as we need such null pointer checks all over the place? i fell over a comment saying:
"Once setContentView() has been called, you will never get a null View provided you are looking in the correct layout and the View exists in that layout."

https://stackoverflow.com/questions/16437050/checking-if-a-view-null-before-setting-onclicklistener

Copy link
Contributor

@harshika-kashyap harshika-kashyap left a comment

Choose a reason for hiding this comment

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

Changes look good except one point for discussion.


public boolean checkNull(View view) {
return view != null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, encapsulating such small functionality in a separate function is overkill. You are anyways going to write if(checkNull(view_name)){}. Why not just if(view_name != null). Every function call has an overhead and hence I am not in favor of it. If readability is your main intention, then we might leave it like you have done but I find explicit check equally readable.

@harshika-kashyap
Copy link
Contributor

@soloturn Hi, So I went through the codebase a bit more after going through the Stackoverflow link. I must say that I might be wrong since I started yesterday only and hence am relatively new to this project.

I think the problem is indeed a design issue.

  1. We open the SettingsActivity to change settings.
  2. We change a few things which requires us to restart the MainActivity. The onActivityResult(Line 1417 - MainActivity.java) method handles the callback using this snippet
case REQUEST_PREFERENCES:
        if (resultCode == RESULT_RESTART) {
          startActivity(new Intent(MainActivity.this, MainActivity.class));
          finish();
        }
        ... more code.
  1. The Activity is recreated. While the Activity is getting recreated(not yet finished), the KiwixWebChromeClient uses the callback to update the view using methods such as webViewProgressChanged. Now because the views are still being recreated, we get a NPE and hence crash.

@soloturn
Copy link
Contributor

soloturn commented Mar 3, 2019

@harshika-arya thanks for having a look! how would you recommend to implement this?

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

Successfully merging this pull request may close these issues.

None yet

4 participants