-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Donation dialog does not show button on standard #371
Donation dialog does not show button on standard #371
Conversation
7b8a8e7
to
d3697e2
Compare
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
d3697e2
to
d872f68
Compare
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 guess it's neglegtible right now, since there is not anything else going on in ICSx5 at the moment, but in general it would be better to have a seperate PR for the datastore migration, which then gets merged before the issue PR. and It also makes the review easier -> less potential bugs.
Maybe the migration to DataStore is not mandatory for this fix, but I think it's useful, and may solve any future issues.
I think we should do it without datastore for now and try to keep ICSx5 as compatible with DAVx5 as possible. I saw you proposed it for DAVx5, but until it's actually implemented in DAVx5 could be a long time and we don't know Rickis plans on when to merge ICSx5 or to migrate to Datastore really. There could be reasons to merge ICSx5 first and then we don't want to be forced to implement datastore in DAVx5 first.
It's definetly useful and I saw it used in another app already myself. Would be cool to have it, but yeah maybe later?
EDIT:
Nevermind, Ricki says it's nice to try out here in ICSx5 already and should not cause complications when incorporating into DAVx5 :)
Perfect. In any case, there isn't much stored there, so really shouldn't matter for migration to DAVx5 |
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.
See the comments. Works well 👍
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>
Aaaand now tests are failing |
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
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.
See comments
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>
Should be ready :) |
Somethings seems to not work with the CI emulator tests, but the change is little. Looks good :) |
Purpose
The ose version of ICSx5 shows a donation dialog every once in a while, which does have a correct working donation button, but once you hide it, it doesn't pop up again, and can't be invoked in any other way. If the one in the info activity is pressed, the same is shown both in gplay and standard, which doesn't contain any donation button to comply with Google Play's policy.
This dialog should be the same as the one that pops up automatically in standard.
Short description
The preferences have been migrated to
DataStore
since otherwise there's no easy way to access them from outside theDonateDialogService
for example.There's a migration function in
SubscriptionsModel
even though I don't think it's even worth it for what it migrates.Now in
InfoActivity
the services (ComposableStartupService
) are also initialized. If there's a service with theFLAG_DONATION_DIALOG
it's shown instead of the default "text dialog".Note
If we eventually migrate to Compose Navigation services won't be needed to be initialized twice since there would be only one Activity.
Maybe the migration to DataStore is not mandatory for this fix, but I think it's useful, and may solve any future issues.
Checklist