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

Reimplementing the multiple popups handling in the new popup solution. #1454

Merged
merged 11 commits into from Aug 14, 2022

Conversation

kalina559
Copy link
Collaborator

The previous solution was done in NumberPickerFactory class, that has been since removed. Moved the changes to the NumberPopup class, that's now used to create numerical popups.

@kalina559 kalina559 mentioned this pull request Aug 13, 2022
10 tasks
@iSoron
Copy link
Owner

iSoron commented Aug 13, 2022

Looks good to me. Let's just wait for the checks to pass. Do we also need this for the Yes/No popup?

@kalina559
Copy link
Collaborator Author

Oh yeah, I forgot that this changed as well. Previously when double clicking yes/no it just changed between yes/skipped/no and there was no popup.

@kalina559
Copy link
Collaborator Author

kalina559 commented Aug 13, 2022

I gave up with the inheritance in the end, because the dialog classes sometimes inherit from AppCompatDialogFragment, sometimes from AlertDialog and sometimes from nothing at all. They use different methods to be created/dismissed so it was hard to create a universal way to handle it.
I think the HabitsApplication instance is always present when the app is opened, so its companion object shouldn't be ever deallocated by the system (I may be wrong though 😀)

@kalina559
Copy link
Collaborator Author

Maybe I'll look at it as part of the next release to refactor the dialogs and align them a bit?

@iSoron
Copy link
Owner

iSoron commented Aug 13, 2022

Hi @kalina559, thanks for working on this. A few suggestions:

  1. I think we can make currentDialog a top level variable; we don't gain much by putting it inside HabitsApplication; it's still just a global variable.
  2. If we make it a WeakReference<Dialog>, then we don't need to worry about setting it to null.
  3. We can use extension functions to make using it more natural.

In summary, I think we could define, in a new file...

var currentDialog: WeakReference<Dialog> = WeakReference(null)

fun Dialog.dismissCurrentAndShow() {
    currentDialog.get()?.dismiss()
    currentDialog = WeakReference(this)
    show()
}

...then call dialog.dismissCurrentAndShow() instead of dialog.show(). Please let me know if I am missing anything.

@kalina559
Copy link
Collaborator Author

Something like this? My only worry is that after opening and closing a dialog, it stays saved as the currentDialog, and when opening the next one, we try to dismiss a dialog that's already been closed. No error or warning is thrown though.

@kalina559
Copy link
Collaborator Author

kalina559 commented Aug 13, 2022

Nevermind, this doesn't work yet 😀

@kalina559
Copy link
Collaborator Author

kalina559 commented Aug 13, 2022

Okay, now it works. The downside is the delay between showing the the first dialog, closing it and opening the next one after - previously we closed the first one immediately in show(), onCreate() or onCreateDialog() and now we create both dialogs and then check if the second one is not a 'duplicate'.
But on the other hand this is pretty much an edge case - no one in their right mind opens these dialogs that fast xd
I also found another one of these dialogs in EditHabitActivity.

@kalina559
Copy link
Collaborator Author

kalina559 commented Aug 14, 2022

Is there any way to check the ktlint report from Github Actions? Because I have no idea what's causing the checks to fail.
I ran the checks locally and they don't show any violations here.

EDIT: Turned out it was a 'Missing space after //' warning in a comment I left in the code. Not sure why, but the local ktlint check only showed it after I changed to a different branch and back.

@kalina559 kalina559 requested a review from iSoron August 14, 2022 11:46
@iSoron iSoron merged commit 428bf42 into release/2.1.0 Aug 14, 2022
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

2 participants