-
Notifications
You must be signed in to change notification settings - Fork 551
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: EditProfile discard changes dialog shown on back press #1307
Conversation
7c1e320
to
8a80e79
Compare
bb97b30
to
fe24593
Compare
The solution presented is very brittle and can fail in many ways |
Can you guide me towards an efficient solution? |
Why you haven't just overriden onBackPressed in the fragment? |
As far as I checked onBackPressed cannot be overriden in fragments. That is why I took this route. |
* Called by EditProfileFragment to go to previous fragment | ||
*/ | ||
fun onSuperBackPressed() { | ||
super.onBackPressed() |
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.
Useless as it's just calling onBackPressed. OnBackPressed can be called directly
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 use this method to call the direct onBackPressed from the EditProfileFragment. If i would have called the onBackPressed then it would have called the overriden method which would again call the handleBackPress() method causing a cycle
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.
Ok
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.
Any changes I should make?
@iamareebjamal I have updated the branch. Please review |
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.
You need to check if edit text and image field are clicked, then only show an alert dialog.
|
||
// Calls the handleBackPress method in EditProfileFragment | ||
val hostFragment = supportFragmentManager.findFragmentById(R.id.frameContainer) as? NavHostFragment | ||
(hostFragment?.childFragmentManager?.fragments?.get(0) as? EditProfileFragment)?.handleBackPress() |
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.
Too complex.
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.
Should I split these up into more lines of code or do you mean some other way should be used? Because I could not find any other way
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.
You just need to call the method in edit profile fragment. Can't you do it by taking the fragment as object?
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.
How do I get a reference to the Fragment instance? It is done using FragmentManager only right? If I used any other method to obtain the fragment the app was crashing
@@ -110,7 +111,7 @@ class EditProfileFragment : Fragment() { | |||
.observe(this, Observer { | |||
Snackbar.make(rootView.editProfileCoordinatorLayout, it, Snackbar.LENGTH_LONG).show() | |||
if (it == USER_UPDATED) { | |||
activity?.onBackPressed() | |||
(activity as? MainActivity)?.onSuperBackPressed() |
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.
Do not cast.
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.
How do I use this method from MainActivity then without casting?
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.
Use smart cast instead.
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.
Changes made
This is done in the following code
|
@@ -110,7 +111,7 @@ class EditProfileFragment : Fragment() { | |||
.observe(this, Observer { | |||
Snackbar.make(rootView.editProfileCoordinatorLayout, it, Snackbar.LENGTH_LONG).show() | |||
if (it == USER_UPDATED) { | |||
activity?.onBackPressed() | |||
activity?.let { act -> if (act is MainActivity) act.onSuperBackPressed() } | |||
} |
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.
@liveHarshit I have done it this way instead of using if(activity is MainActivity) activity.onSuperBackPressed()
because the following error occurred for this method
Smart cast to 'MainActivity' is impossible, because 'activity' is a property that has open or custom getter
Using let
worked
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.
Then use a value - val thisActivity = activity
then cast for thisActivity.
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.
okay! I tried that also but thought this was cleaner. will implement this
* Handles back press when up button or back button is pressed | ||
*/ | ||
fun handleBackPress() { | ||
val activity = activity as? MainActivity |
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.
Don't cast like that.
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.
okay
83ecebe
to
d67fbae
Compare
@iamareebjamal @liveHarshit any other changes that I should make in this? |
Fixes Issue fossasia#1292. The behaviour for up button has been replicated for the back button. So now if changes are made in the EditProfileFragment and the back button or the up button is pressed, an AlertDialog is displayed which warns the user of the unsaved changes
@iamareebjamal Please review. |
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 very hacky. Features which require such hacky code are generally not worth it
Fixes Issue #1292. The behaviour for up button has been replicated for the back button. So now if changes are made in the EditProfileFragment and the back button or the up button is pressed, an AlertDialog is displayed which warns the user of the unsaved changes
Changes:
The MainActivity class has been modified to handle the backPress for the EditProfileFragment. It in turn calls a method in EditProfileFragment which handles the backPress. When the back button is finally meant to be pressed from EditProfileFragment to go back, then super.onBackPressed is called using a new method.
Please suggest a better method to implement this if any.
Screenshots for the change: