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

Make caption dialog non-cancellable (issue #4939) #4942

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

svenjacobs
Copy link
Contributor

I just made the dialog non-cancellable. Actually I don't know why it was cancellable when there also is

dialog?.setCanceledOnTouchOutside(false)

Fixes #4939

@connyduck
Copy link
Collaborator

This is a good start, but I think we will still need another dialog to prevent accidental cancel with the cancel button

@Ember-ruby
Copy link

Actually I don't know why it was cancellable when there also is

this is what introduced it svenjacobs@9e66ccf, and the only change there that looks like it had something to do with Close dialog on back button is setting this to true, so just setting it to false may cause an unintended regression?

@svenjacobs
Copy link
Contributor Author

svenjacobs commented Feb 24, 2025

So instead of making it non-cancellable, we need a warning dialog when text was entered and someone either presses Cancel, dismissed the dialog by clicking outside or uses the back navigation.

@connyduck
Copy link
Collaborator

So instead of making it non-cancellable, we need a warning dialog when text was entered and someone either presses Cancel, dismissed the dialog by clicking outside or uses the back navigation.

No, the non-cancellable thing makes sense and can stay. Just add another confirmation to the cancel button.

@svenjacobs
Copy link
Contributor Author

No, the non-cancellable thing makes sense and can stay. Just add another confirmation to the cancel button.

So are you fine with the back navigation not doing anything when this dialog is opened?

@connyduck
Copy link
Collaborator

connyduck commented Feb 24, 2025

So are you fine with the back navigation not doing anything when this dialog is opened?

Well we don't want accidental cancels, so yes? Back button should either do nothing, or show the warning dialog as well. But definitely not close the dialog silently.

@svenjacobs
Copy link
Contributor Author

svenjacobs commented Feb 25, 2025

Sorry guys but I currently struggle with this a bit as I have only coded in Jetpack Compose for the past few years and it seems I forgot how to handle Fragments 😂

I tried to implement the onCancel() method hoping I can intercept a cancellation and show a confirmation dialog both for back navigation and clicking outside the dialog but unfortunately this method is called after the dialog has already been cancelled. So I guess we have to disable all cancellation and handle it manually, meaning we have to also intercept the back navigation event.

@connyduck
Copy link
Collaborator

we have to also intercept the back navigation event

Yes probably

@Lakoja
Copy link
Collaborator

Lakoja commented Mar 5, 2025

A few observations:

  • onDismiss() is called before the dialog is closed (cancelled)
  • In order to get rid of the text button "Cancel" the "setNegativeButton()" in "onCreateDialog()" needs to be removed
  • The "isCancelable = false" (then) only prohibits the closing with the back button
  • (Detail: That can also be placed in "onCreateDialog" - not sure which is the better location. I prefer onCreateDialog a bit.)

However: I would say that we could stay with normal UI behavior here:

  • It is cancelable; in any normal way: Cancel button, back button, tap outside
  • This would require however an "Are you sure?" dialog everytime the text was changed in comparisson to the initial one ("arguments?.getString(EXISTING_DESCRIPTION_ARG)")

@connyduck
Copy link
Collaborator

@svenjacobs Do you still want to work on this? If not I can just merge it like this and do the remaining stuff myself.

@svenjacobs
Copy link
Contributor Author

Sorry, some other stuff caught up. Please go ahead and merge it. I think you will solve this in half the time than me struggling with the legacy Android view system 😉

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.

Alt text lost when tapping outside of the input box
4 participants