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

Fix/9905 crash inflate exception binary xml file line #10279

Closed

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented Jul 25, 2019

Fixes #9905

The main approach for a potential fix is to increment android material library from 1.0.0 to 1.1.0-rc01.

Reason for that was because the codebase from the library was much improved.

They changed a logic around crashing line from old release and general logic how drawables can be set. https://github.com/material-components/material-components-android/blob/cd59e98f7e2185ddb075ff0fc91f29765d562968/lib/java/com/google/android/material/textfield/TextInputLayout.java#L344
(log -> https://sentry.io/share/issue/9eaadccac3be4c259943d72c46427792/).

Thanks to @mzorz comment 👉 #9905 (comment) I got inspired to make one more step in this fix.

The second approach is to use our own xml which represents show/hide password icon in order to avoid a potential crash if icon can't be found in runtime.

Note: The new version of android material library forced some compile time issues :

Screen Shot 2019-07-23 at 3 43 16 PM

⬆️ this was resolved with adding !! and as I am not experienced in Kotlin, if someone has some better approach I am open for suggestions.

Screen Shot 2019-07-23 at 3 44 25 PM

To test:

It's very hard to reproduce crash from the logs. However, one should check for potential regressions.

In order to achieve that one should open and check every screen where TextInputLayout or any other component from material library is used. EspeciallyEnter your WordPress password screen should be checked, where password visibility icon (show/hide password) shod work as before (expected).

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 25, 2019

Warnings
⚠️ PR is not assigned to a milestone.
Messages
📖

This PR contains changes in the subtree libs/login/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPress-Login-Flow-Android. Follow these handy steps!
WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history!

  1. cd WordPress-Android
  2. git checkout fix/9905_Crash_InflateException_Binary_XML_file_line
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/10279
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/10279 and open a new PR.

Generated by 🚫 dangerJS

@marecar3 marecar3 assigned marecar3 and unassigned marecar3 Jul 25, 2019
@marecar3 marecar3 requested a review from daniloercoli July 25, 2019 02:19
@planarvoid
Copy link
Contributor

... this was resolved with adding !! and as I am not experienced in Kotlin, if someone has some better approach I am open for suggestions.

I think you're keeping the previous functionality by this approach (it would've crashed even before if the fragmentManager were null).

If you wanna add a non-null check, you can do something like this:

fragmentManager?.let { nonNullFragmentManager -> 
      dialog.show(nonNullFragmentManager, it)
}

I'm not sure it's worth the extra effort

@mzorz
Copy link
Contributor

mzorz commented Jul 29, 2019

I think you're keeping the previous functionality by this approach (it would've crashed even before if the fragmentManager were null).

Exactly - I think Marko's code in that regard looks ok as is, as not having a non-null FragmentManager at any point is probably good to let it crash

@marecar3 marecar3 requested a review from theck13 August 9, 2019 17:02
@planarvoid planarvoid removed their request for review September 25, 2019 08:05
@marecar3
Copy link
Contributor Author

Closing this one in favor of #11172

@marecar3 marecar3 closed this Jan 27, 2020
@khaykov khaykov mentioned this pull request Feb 4, 2020
3 tasks
…L_file_line

# Conflicts:
#	WordPress/build.gradle
#	WordPress/src/main/java/org/wordpress/android/ui/main/MainBottomSheetFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/pages/PagesFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/reader/SubfilterBottomSheetFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/sitecreation/siteinfo/SiteCreationSiteInfoFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/sitecreation/verticals/SiteCreationVerticalsFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/widget/configuration/StatsWidgetConfigureFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/widget/configuration/StatsWidgetSiteSelectionDialogFragment.kt
#	WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/widget/minified/StatsMinifiedWidgetConfigureFragment.kt
#	WordPress/src/main/res/values/styles.xml
#	libs/editor/WordPressEditor/build.gradle
#	libs/login/WordPressLoginFlow/build.gradle
#	libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/widgets/WPLoginInputRow.java
@marecar3 marecar3 reopened this May 21, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 21, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@planarvoid planarvoid removed their request for review May 21, 2020 15:18
@khaykov khaykov removed their request for review May 22, 2020 03:48
marecar3 added 5 commits May 22, 2020 14:13
…L_file_line

# Conflicts:
#	libs/login/WordPressLoginFlow/src/main/res/drawable-xhdpi/ic_password_visibility.png
@marecar3 marecar3 requested review from khaykov and oguzkocer and removed request for maxme, malinajirka, nbradbury, mzorz, daniloercoli, theck13 and hypest May 22, 2020 13:31
@marecar3 marecar3 closed this May 22, 2020
@marecar3 marecar3 deleted the fix/9905_Crash_InflateException_Binary_XML_file_line branch June 26, 2020 10:43
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.

Crash: InflateException: Binary XML file line #26: Error inflating class org.wordpress.android.l...
8 participants