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

[Push] Allow UP distributor selection #1146

Merged
merged 23 commits into from
Dec 4, 2024

Conversation

ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ commented Nov 28, 2024

Purpose

Give the user the option to select their preferred push distributor when multiple are available.

Short description

When the unified push distributor setting is tapped, a new dialog is shown with the distributors available.

Choose endpoint Dist. selected but no endpoint Endpoint ready
distributors-selected no-endpoint Screenshot_20241128_084945

It can happen that a distributor has been selected, but it has still not provided any endpoint. If that's the case, a new text is displayed: "Awaiting endpoint from ..."

There's a new preference now that stores the currently selected distributor, as well as the existing one that stores the push endpoint.

Note: Right now, if you have the app open, and install a new distributor, it won't show up immediately, you need to exit from settings, and enter again, no need to restart the app.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ linked an issue Nov 28, 2024 that may be closed by this pull request
@ArnyminerZ ArnyminerZ self-assigned this Nov 28, 2024
@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Nov 28, 2024
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@rfc2822 rfc2822 requested a review from sunkup November 29, 2024 11:29
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and looks very good!

As I understand it, we shouldn't "do" too much in the UI, so the actual UnifiedPush logic should be in the model.

Also let's call the variables etc. "pushDistributors" instead of "distributors" because distributors could also mean something else.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 November 29, 2024 13:29
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@rfc2822 rfc2822 force-pushed the 1143-push-provide-ui-to-select-the-up-distributor branch from 897106d to 083145e Compare November 30, 2024 10:27
@rfc2822
Copy link
Member

rfc2822 commented Nov 30, 2024

I have also cleaned up the tasks settings in AppSettings, please merge main-ose first

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
…he-up-distributor' into 1143-push-provide-ui-to-select-the-up-distributor

# Conflicts:
#	app/src/main/kotlin/at/bitfire/davdroid/ui/AppSettingsModel.kt
#	app/src/main/kotlin/at/bitfire/davdroid/ui/AppSettingsScreen.kt
…stributor

# Conflicts:
#	app/src/main/kotlin/at/bitfire/davdroid/ui/AppSettingsScreen.kt
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 1, 2024 16:37
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what should happen when there's only one distributor – should the selection popup even appear or should the only one be automatically registered, like it's done currently?

Also CC @devvv4ever

@devvv4ever
Copy link
Member

I'm wondering if this should be an explicit user interaction. There could be the case that people don't want any Push at all and then they should have an option like: "Don't use any distributor" or just an option "None / Disabled".

I'd go for an auto-select if there is any distributor available, but also add an option to disable it at all on user wish. So the dialog would be nice to have in any case.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ
Copy link
Member Author

I'd go for an auto-select if there is any distributor available, but also add an option to disable it at all on user wish. So the dialog would be nice to have in any case.

Done it 😄

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 2, 2024 07:28
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're already done.

There's however a crash:

  1. I have only ntfy installed.
  2. Start DAVx5, app settings, UnifiedPush, select None, OK.
  3. Back, Back to close DAVx5.
  4. Start DAVx5 again, choose app settings.
  5. It crashes:
FATAL EXCEPTION: DefaultDispatcher-worker-1
Process: at.bitfire.davdroid, PID: 11179
java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.Object kotlinx.coroutines.flow.MutableStateFlow.emit(java.lang.Object, kotlin.coroutines.Continuation)' on a null object reference
	at at.bitfire.davdroid.ui.AppSettingsModel.loadPushDistributors(AppSettingsModel.kt:139)
	at at.bitfire.davdroid.ui.AppSettingsModel.access$loadPushDistributors(AppSettingsModel.kt:33)
	at at.bitfire.davdroid.ui.AppSettingsModel$1.invokeSuspend(AppSettingsModel.kt:43)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:101)
	at kotlinx.coroutines.internal.LimitedDispatcher$Worker.run(LimitedDispatcher.kt:113)
	at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:89)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:589)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:823)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:720)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:707)
	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@39af90e, Dispatchers.IO]

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ
Copy link
Member Author

The UnifiedPush website is back online. This is the example they give:

import org.unifiedpush.android.connector.UnifiedPush.*
/* ... */

// Check if a distributor is already registered
getAckDistributor(context)?.let {
    // Re-register in case something broke
    // Options:
    // "instance" to handle multiple registrations
    // "features" empty array or [FEATURE_BYTES_MESSAGE]
    //    to be sure the distributor handles non-UTF-8 input
    // "messageForDistributor" that may be displayed by the distrib.
    registerApp(context)
    return
}
// Get a list of distributors that are available
val distributors = getDistributors(context)
// select one or show a dialog or whatever
val userDistrib = yourFunc(distributors)
// save the distributor
saveDistributor(context, userDistrib)
// register your app to the distributor
// Options:
// "instance" to handle multiple registrations
// "features" empty array or [FEATURE_BYTES_MESSAGE]
//    to be sure the distributor handles non-UTF-8 input
// "messageForDistributor" that may be displayed by the distrib.
registerApp(context)

I mean, regarding the getAckDistributor call

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@rfc2822
Copy link
Member

rfc2822 commented Dec 3, 2024

java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.Object kotlinx.coroutines.flow.MutableStateFlow.emit(java.lang.Object, kotlin.coroutines.Continuation)' on a null object reference

I think the correct way is to set value = (like in this example) instead of using emit. Can you try to reproduce the problem and does that make it go away?

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ
Copy link
Member Author

Can you try to reproduce the problem and does that make it go away?

I was able to reproduce, I think it was because we were calling init before declaring the variable, and even though it was in a coroutine, _pushDistributor wasn't initialized yet. I've moved it to the bottom and now it works correctly.

@ArnyminerZ ArnyminerZ requested review from rfc2822 and removed request for rfc2822 and sunkup December 3, 2024 13:11
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The init block position was a surprise… 👍🏻 Now looks good and works.

I have replaced the emit() by .value = although it does the same thing and shouldn't matter. However it seems more logical to set the value in the model (because we always use the value) and it's done like that in the example.

@rfc2822 rfc2822 changed the title Added UP distributor selector [Push] Allow UP distributor selection Dec 4, 2024
@rfc2822 rfc2822 merged commit 9e060f6 into main-ose Dec 4, 2024
9 checks passed
@rfc2822 rfc2822 deleted the 1143-push-provide-ui-to-select-the-up-distributor branch December 4, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Push] Provide UI to select the UP distributor
3 participants