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

[fix] NotificationPermissionController fix an override method signature #2132

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jun 18, 2024

> Task :OneSignal:notifications:compileReleaseKotlin FAILED
w: Argument -module-name is passed multiple times. Only the last value will be used: com.onesignal.notifications
e: /Users/nanli/Documents/GitHub/OneSignal-Android-SDK/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt: (90, 17): 'onFocus' overrides nothing
e: /Users/nanli/Documents/GitHub/OneSignal-Android-SDK/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt: (91, 35): No value passed for parameter 'firedOnSubscribe'

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':OneSignal:notifications:compileReleaseKotlin'.
> A failure occurred while executing org.jetbrains.kotlin.compilerRunner.GradleCompilerRunnerWithWorkers$GradleKotlinCompilerWorkAction
   > Compilation error. See log for more details

This change is Reviewable

@nan-li nan-li requested review from jkasten2 and emawby June 18, 2024 19:54
Copy link
Member

@jkasten2 jkasten2 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 CI never failed due to these branches were running parallel and there was never a PR where both change existed at the same time.

@nan-li
Copy link
Contributor Author

nan-li commented Jun 18, 2024

Oh, right, that was added as part of the cold start PR , d'oh.

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Actually looks like there are some more spots this param needs to be added.

* This code was originally created in #2112
* A parallel PR made a change to the method signature of `onFocus`
@nan-li nan-li force-pushed the fix/NotificationPermissionController_methods branch from 8930137 to 9482a93 Compare June 18, 2024 20:17
@nan-li nan-li requested a review from jkasten2 June 18, 2024 20:28
@nan-li nan-li merged commit e367bd9 into main Jun 18, 2024
2 checks passed
@nan-li nan-li deleted the fix/NotificationPermissionController_methods branch June 18, 2024 20:54
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.

2 participants