-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fixes #3639 (Fix Save State implementation of CheckBoxTriState ) #3686
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 #3639 (Fix Save State implementation of CheckBoxTriState ) #3686
Conversation
* Removed custom save state implementatio of CheckBoxTriState * Save/Restore CheckBox state in NearbyParentFragment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add tests to make sure this change works as expected?
crashes when restoring the state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't reproduce the issue, even with mentioned developer option is set.
click on the tricheckstate button a few times, apologies, should have mentioned more explicitly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java.lang.NullPointerException: Attempt to invoke interface method 'void fr.free.nrw.commons.nearby.CheckBoxTriStates$Callback.filterByMarkerType(java.util.List, int, boolean, boolean)' on a null object reference
at fr.free.nrw.commons.nearby.CheckBoxTriStates.setState(CheckBoxTriStates.java:93)
at fr.free.nrw.commons.nearby.fragments.NearbyParentFragment.onViewCreated(NearbyParentFragment.java:228)
After @macgills 's warnings, I also could reproduce the issue :/
Thanks for testing the PR @neslihanturan & @macgills. I have added the fix for the exception, tested the app with a few clicks here and there. Please review the PR again. |
Codecov Report
@@ Coverage Diff @@
## 2.13-release #3686 +/- ##
=================================================
- Coverage 6.95% 6.94% -0.01%
+ Complexity 291 289 -2
=================================================
Files 256 256
Lines 11733 11713 -20
Branches 959 960 +1
=================================================
- Hits 816 814 -2
+ Misses 10846 10828 -18
Partials 71 71
Continue to review full report at Codecov.
|
@ashishkumar468 still crashing with same repro steps
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did not failed for me this time
Did I missed some steps to reproduce it again? will check the issue page instead of relying on my memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, after playing wit checkbox states I also still can reproduce it:
java.lang.NullPointerException: Attempt to invoke interface method 'void fr.free.nrw.commons.nearby.CheckBoxTriStates$Callback.filterByMarkerType(java.util.List, int, boolean, boolean)' on a null object reference
at fr.free.nrw.commons.nearby.CheckBoxTriStates.setState(CheckBoxTriStates.java:93)
at fr.free.nrw.commons.nearby.fragments.NearbyParentFragment.onViewCreated(NearbyParentFragment.java:228)
@macgills @neslihanturan Just to confirm, the exact steps to reproduce are turn on don't keep activities, open the app, press home->come back go to nearby, play with the checkbox tri-state?. Did I miss anything? |
You should play with checkboxtristate at beginning. Press the home button of the phone, not home of the app, then return back to the app. |
…called before onViewRestored sometimes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After your last commit, it does not crash for me anymore:)
It isn't exactly good practice but we can just not restore the state, create a tech debt ticket and move on with the release @misaochan |
@macgills I think I might be able to preserve the checkbox state and solve the crash, working on it, will push the changes soon |
I seem to have found the fix, @macgills @neslihanturan. Please verify, sorry for the trouble :) |
Fair point @macgills, I am okay to remove the state saving, let's have some more insights @neslihanturan @misaochan @maskaravivek |
I am personally okay with not saving the state for now and just fixing the crash for the release. It is better than having filters that don't actually affect the pins on the map, and better than users crashing. Let us wait for @neslihanturan 's input, and if she is okay with it we can proceed. |
I also think not saving the state for now as soon as the issue is fixed. |
Hi, @neslihanturan @macgills I have pushed the discussed changes, please have a look |
.idea/codeStyles/codeStyleConfig.xml
Outdated
@@ -1,5 +1,6 @@ | |||
<component name="ProjectCodeStyleConfiguration"> | |||
<state> | |||
<option name="USE_PER_PROJECT_SETTINGS" value="true" /> | |||
<option name="PREFERRED_PROJECT_CODE_STYLE" value="Default" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://youtrack.jetbrains.com/issue/IDEA-207303#focus=streamItem-27-3299338.0-0 so I think this comment has the solution to this line appearing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixing this
@@ -118,8 +118,9 @@ | |||
|
|||
public class NearbyParentFragment extends CommonsDaggerSupportFragment | |||
implements NearbyParentFragmentContract.View, | |||
WikidataEditListener.WikidataP18EditListener, LocationUpdateListener { | |||
WikidataEditListener.WikidataP18EditListener, LocationUpdateListener ,CheckBoxTriStates.Callback{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect zero changes to this file, if we just remove the state restoration in CheckboxTriState that is the minimal change to solve this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macgills We have removed the manual state restoration thing, but the android system also restores the view right so if we don't set the callback in such cases, it will throw an NPE, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called on master after onViewCreated
checkBoxTriStates.setCallback((o, state, b, b1) -> presenter.filterByMarkerType(o,state,b,b1));
There was an internal problem where the checkbox will throw an NPE if you don't set a click listener to it and it processes a click event, that should not be the case. That problem arose because the state restoration was happening before the click listener was set and your version of state restoration didn't use the isRestoring
flag to prevent the state restoration being interpreted as human input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still halfway through my morning coffee, that wasn't a great explanation...
I'll perk up in a bit if you have any questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, done
Thanks @ashishkumar468 ! |
* #3624 DateTimeFormat wrong - match pattern returned from servers (#3625) * Revert "Fixes: #3179 Make category search non case-sensitive (#3326)" (#3636) Simply lower casing the name of the category sent to the server doesn't result in the server doing a case insensitive category search. In fact, it reduces the category search space as only categories that has a lower case character is searched even if the search text contains upper case characters. The test case did not catch this issue as the first character of the title is case insensitive[1]. So, revert the changes done in commit afdeaae. See further disucssion in the issue thread of #3179 starting from [2]. [1]: https://www.mediawiki.org/wiki/Manual:Page_title [2]: #3179 (comment) * Bugfix/security exception (#3627) * Fixes #3626 * Check is file is actually created before writing to file, file picker android * Handle Security exception * Fixes #3436 and #2881: Media Detail design Overhaul (#3505) * ic_map_dark_24dp: map icon for white background * ic_info_outline_dark_24dp: info icon for dark background * MediaDetailFragment: update the spacer as per image aspect ratio * fragment_media_detail: design overhaul * fragment_media_detail: remove redundant background color statements * make requested changes * add dark mode support * minor ui tweak * white map icon in dark mode * make rquested changes * make requested changes to layout * fix misalignment of category list * subtle amendments * convert comments to javadocs * minor amendments * minor changes * add styles for media detail * Media detail fragment refactored * make suggested changes * minor name fix * fix the delete button border * Fixes #3639 (Fix Save State implementation of CheckBoxTriState ) (#3686) * Add #3723 and #3721 to 2.13 release, fix conflicts Conflicts were caused by merging #3723 before #3721 , so I just rolled both into one commit. * Fix NullPointer when clicking on image in MediaDetailFragment (#3730)… (#3739) * Update changelog.md * Versioning for v2.13 * Fixes #3705 (Crash when viewing pic I just uploaded) (#3782) * Fixes #3705 * Let the MediaDetailPager fragment know when the contributions have been updated * Handle NPE, null check on adapter in MediaDetailPagerFragment * Fixed BookmarkLocationsDao DB migration (#3793) * Fixes #3725 (#3795) * Downgraded okhttp version to support Api 19 devices * Handled null CompoundDrawable[2] in etTitle-> UploadMediaDetailsFragment (#3828) * DownSample Upload image to be shown in UploadMediaDetailFragment to handle OOM, Bitmap Too large exception (#3830) * Fixes #3829 * DownSample Upload image to be shown in UploadMediaDetailFragment to handle OOM, Bitmap Too large exception * removed unused imports, handled possible exceptions * Let Fresco handle the downsampling of image * invalidate in onTransformEnd * Expose an interface TransformationListener in ZoomableDraweeView to listen to transformation change end * removed photoView dependency * removed unused imports in ZoomableActivity * Bugfix, expand/collapse * changed functio name * Bugfix/p18 uploads (#3869) * updated gradle plugin version * BugFix #3856 * Do not use preference for deciding acceptable lat long for nearby uploads, instead save the corresponding location in the Contribution via UploadItem * Marshall contribution's hasInvalidLocation * reset un-related changes * Fixed test cases * Minor code formatting and docs * Fixes #3882 (#3883) * Make hasInvalidLocation non-null integer with default value 0 Co-authored-by: Ashish Kumar <ashish@Ashishs-MacBook-Air.local> * Fixes #3766, Added OPENSTREET attribution (#3889) * Fixes #3766 * Added OPENSTREET attribution in nearby * Added custom text attribution in Nearby * Deleted unused class CustomBorderTextView * review suggested changes * modified telemetry summary string * Versioning and changelog for v2.13.1 (#3908) * Update changelog.md * Versioning for v2.13.1 * Fixes #3914 (#3915) * Verify user login before setting upload count * fixed compile-time error * fix erros * delete emptied files * remove empty file CategoriesModel.java Co-authored-by: Seán Mac Gillicuddy <seantheappdev@gmail.com> Co-authored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Co-authored-by: Kshitij Bhardwaj <44129798+kbhardwaj123@users.noreply.github.com> Co-authored-by: Vitaly V. Pinchuk <vetal.978@gmail.com> Co-authored-by: Josephine Lim <josephinelim86@gmail.com> Co-authored-by: Ashish Kumar <ashish@Ashishs-MacBook-Air.local>
* commons-app#3624 DateTimeFormat wrong - match pattern returned from servers (commons-app#3625) * Revert "Fixes: commons-app#3179 Make category search non case-sensitive (commons-app#3326)" (commons-app#3636) Simply lower casing the name of the category sent to the server doesn't result in the server doing a case insensitive category search. In fact, it reduces the category search space as only categories that has a lower case character is searched even if the search text contains upper case characters. The test case did not catch this issue as the first character of the title is case insensitive[1]. So, revert the changes done in commit afdeaae. See further disucssion in the issue thread of commons-app#3179 starting from [2]. [1]: https://www.mediawiki.org/wiki/Manual:Page_title [2]: commons-app#3179 (comment) * Bugfix/security exception (commons-app#3627) * Fixes commons-app#3626 * Check is file is actually created before writing to file, file picker android * Handle Security exception * Fixes commons-app#3436 and commons-app#2881: Media Detail design Overhaul (commons-app#3505) * ic_map_dark_24dp: map icon for white background * ic_info_outline_dark_24dp: info icon for dark background * MediaDetailFragment: update the spacer as per image aspect ratio * fragment_media_detail: design overhaul * fragment_media_detail: remove redundant background color statements * make requested changes * add dark mode support * minor ui tweak * white map icon in dark mode * make rquested changes * make requested changes to layout * fix misalignment of category list * subtle amendments * convert comments to javadocs * minor amendments * minor changes * add styles for media detail * Media detail fragment refactored * make suggested changes * minor name fix * fix the delete button border * Fixes commons-app#3639 (Fix Save State implementation of CheckBoxTriState ) (commons-app#3686) * Add commons-app#3723 and commons-app#3721 to 2.13 release, fix conflicts Conflicts were caused by merging commons-app#3723 before commons-app#3721 , so I just rolled both into one commit. * Fix NullPointer when clicking on image in MediaDetailFragment (commons-app#3730)… (commons-app#3739) * Update changelog.md * Versioning for v2.13 * Fixes commons-app#3705 (Crash when viewing pic I just uploaded) (commons-app#3782) * Fixes commons-app#3705 * Let the MediaDetailPager fragment know when the contributions have been updated * Handle NPE, null check on adapter in MediaDetailPagerFragment * Fixed BookmarkLocationsDao DB migration (commons-app#3793) * Fixes commons-app#3725 (commons-app#3795) * Downgraded okhttp version to support Api 19 devices * Handled null CompoundDrawable[2] in etTitle-> UploadMediaDetailsFragment (commons-app#3828) * DownSample Upload image to be shown in UploadMediaDetailFragment to handle OOM, Bitmap Too large exception (commons-app#3830) * Fixes commons-app#3829 * DownSample Upload image to be shown in UploadMediaDetailFragment to handle OOM, Bitmap Too large exception * removed unused imports, handled possible exceptions * Let Fresco handle the downsampling of image * invalidate in onTransformEnd * Expose an interface TransformationListener in ZoomableDraweeView to listen to transformation change end * removed photoView dependency * removed unused imports in ZoomableActivity * Bugfix, expand/collapse * changed functio name * Bugfix/p18 uploads (commons-app#3869) * updated gradle plugin version * BugFix commons-app#3856 * Do not use preference for deciding acceptable lat long for nearby uploads, instead save the corresponding location in the Contribution via UploadItem * Marshall contribution's hasInvalidLocation * reset un-related changes * Fixed test cases * Minor code formatting and docs * Fixes commons-app#3882 (commons-app#3883) * Make hasInvalidLocation non-null integer with default value 0 Co-authored-by: Ashish Kumar <ashish@Ashishs-MacBook-Air.local> * Fixes commons-app#3766, Added OPENSTREET attribution (commons-app#3889) * Fixes commons-app#3766 * Added OPENSTREET attribution in nearby * Added custom text attribution in Nearby * Deleted unused class CustomBorderTextView * review suggested changes * modified telemetry summary string * Versioning and changelog for v2.13.1 (commons-app#3908) * Update changelog.md * Versioning for v2.13.1 * Fixes commons-app#3914 (commons-app#3915) * Verify user login before setting upload count * fixed compile-time error * fix erros * delete emptied files * remove empty file CategoriesModel.java Co-authored-by: Seán Mac Gillicuddy <seantheappdev@gmail.com> Co-authored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Co-authored-by: Kshitij Bhardwaj <44129798+kbhardwaj123@users.noreply.github.com> Co-authored-by: Vitaly V. Pinchuk <vetal.978@gmail.com> Co-authored-by: Josephine Lim <josephinelim86@gmail.com> Co-authored-by: Ashish Kumar <ashish@Ashishs-MacBook-Air.local>
* commons-app#3624 DateTimeFormat wrong - match pattern returned from servers (commons-app#3625) * Revert "Fixes: commons-app#3179 Make category search non case-sensitive (commons-app#3326)" (commons-app#3636) Simply lower casing the name of the category sent to the server doesn't result in the server doing a case insensitive category search. In fact, it reduces the category search space as only categories that has a lower case character is searched even if the search text contains upper case characters. The test case did not catch this issue as the first character of the title is case insensitive[1]. So, revert the changes done in commit afdeaae. See further disucssion in the issue thread of commons-app#3179 starting from [2]. [1]: https://www.mediawiki.org/wiki/Manual:Page_title [2]: commons-app#3179 (comment) * Bugfix/security exception (commons-app#3627) * Fixes commons-app#3626 * Check is file is actually created before writing to file, file picker android * Handle Security exception * Fixes commons-app#3436 and commons-app#2881: Media Detail design Overhaul (commons-app#3505) * ic_map_dark_24dp: map icon for white background * ic_info_outline_dark_24dp: info icon for dark background * MediaDetailFragment: update the spacer as per image aspect ratio * fragment_media_detail: design overhaul * fragment_media_detail: remove redundant background color statements * make requested changes * add dark mode support * minor ui tweak * white map icon in dark mode * make rquested changes * make requested changes to layout * fix misalignment of category list * subtle amendments * convert comments to javadocs * minor amendments * minor changes * add styles for media detail * Media detail fragment refactored * make suggested changes * minor name fix * fix the delete button border * Fixes commons-app#3639 (Fix Save State implementation of CheckBoxTriState ) (commons-app#3686) * Add commons-app#3723 and commons-app#3721 to 2.13 release, fix conflicts Conflicts were caused by merging commons-app#3723 before commons-app#3721 , so I just rolled both into one commit. * Fix NullPointer when clicking on image in MediaDetailFragment (commons-app#3730)… (commons-app#3739) * Update changelog.md * Versioning for v2.13 * Fixes commons-app#3705 (Crash when viewing pic I just uploaded) (commons-app#3782) * Fixes commons-app#3705 * Let the MediaDetailPager fragment know when the contributions have been updated * Handle NPE, null check on adapter in MediaDetailPagerFragment * Fixed BookmarkLocationsDao DB migration (commons-app#3793) * Fixes commons-app#3725 (commons-app#3795) * Downgraded okhttp version to support Api 19 devices * Handled null CompoundDrawable[2] in etTitle-> UploadMediaDetailsFragment (commons-app#3828) * DownSample Upload image to be shown in UploadMediaDetailFragment to handle OOM, Bitmap Too large exception (commons-app#3830) * Fixes commons-app#3829 * DownSample Upload image to be shown in UploadMediaDetailFragment to handle OOM, Bitmap Too large exception * removed unused imports, handled possible exceptions * Let Fresco handle the downsampling of image * invalidate in onTransformEnd * Expose an interface TransformationListener in ZoomableDraweeView to listen to transformation change end * removed photoView dependency * removed unused imports in ZoomableActivity * Bugfix, expand/collapse * changed functio name * Bugfix/p18 uploads (commons-app#3869) * updated gradle plugin version * BugFix commons-app#3856 * Do not use preference for deciding acceptable lat long for nearby uploads, instead save the corresponding location in the Contribution via UploadItem * Marshall contribution's hasInvalidLocation * reset un-related changes * Fixed test cases * Minor code formatting and docs * Fixes commons-app#3882 (commons-app#3883) * Make hasInvalidLocation non-null integer with default value 0 Co-authored-by: Ashish Kumar <ashish@Ashishs-MacBook-Air.local> * Fixes commons-app#3766, Added OPENSTREET attribution (commons-app#3889) * Fixes commons-app#3766 * Added OPENSTREET attribution in nearby * Added custom text attribution in Nearby * Deleted unused class CustomBorderTextView * review suggested changes * modified telemetry summary string * Versioning and changelog for v2.13.1 (commons-app#3908) * Update changelog.md * Versioning for v2.13.1 * Fixes commons-app#3914 (commons-app#3915) * Verify user login before setting upload count * fixed compile-time error * fix erros * delete emptied files * remove empty file CategoriesModel.java Co-authored-by: Seán Mac Gillicuddy <seantheappdev@gmail.com> Co-authored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Co-authored-by: Kshitij Bhardwaj <44129798+kbhardwaj123@users.noreply.github.com> Co-authored-by: Vitaly V. Pinchuk <vetal.978@gmail.com> Co-authored-by: Josephine Lim <josephinelim86@gmail.com> Co-authored-by: Ashish Kumar <ashish@Ashishs-MacBook-Air.local>
Description (required)
Fixes #3639
What changes did you make and why?
CheckBox's custom saved state was not being Parcealized properly
Tests performed (required)
Tested betaDebug on Samsung S7 with "Don't Keep Activities" enabled in developer mode with the steps mentioned in the issue