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

feat!: event channel #1352

Merged
merged 227 commits into from
Apr 4, 2023
Merged

feat!: event channel #1352

merged 227 commits into from
Apr 4, 2023

Conversation

Gustl22
Copy link
Collaborator

@Gustl22 Gustl22 commented Dec 22, 2022

Description

  • Replace Platform Method Channels (platform -> lib) with Event Channels
  • Combined Event stream for player (PlayerEvent) and global (GlobalEvent) events
  • Add methods create, dispose and getEventStream to AudioplayersPlatformInterface
  • Add method getGlobalEventStream to GlobalAudioplayersPlatformInterface
  • Add log stream player.onLog() and AudioPlayer.global.onLog()
  • Example for using Logger
  • Throw PlatformException on Method Error via Method Channel (lib -> platform)
  • Emit PlatformException on Event Error via Event Channel (platform -> lib) player.eventStream.listen().onError() and AudioPlayer.global.eventStream.listen().onError()
  • Add docs

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs:, chore: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation and added dartdoc comments with ///, where necessary.
  • I have updated/added relevant examples in example.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Migration instructions

audioplayers:

Before After
deprecated AudioPlayer.global.changeLogLevel(LogLevel.info) Logger.logLevel = LogLevel.info
deprecated AudioPlayer.global.logLevel Logger.logLevel
deprecated AudioPlayer.global.log() Logger.log() or Logger.error()
deprecated AudioPlayer.global.info() Logger.log()
deprecated AudioPlayer.global.error() Logger.error()
ForPlayer<> removed

audioplayers_platform_interface:

Before After
LogLevel moved to audioplayers package
StreamsInterface removed
ForPlayer<> removed

Related Issues

Closes #106
Closes #151
Closes #1266
Can handle error of #1260
Related to #208 and #933, but cannot hide Android MediaPlayer logs

This would bypass: flutter/flutter#69103

@Gustl22
Copy link
Collaborator Author

Gustl22 commented Jan 1, 2023

@luanpotter @spydon we could also think about unifying the logger and the error stream and handle the logging on the dart side to gain more control. But don't know if it would hit on performance, but I don't think so...

@Gustl22 Gustl22 changed the title feat: error stream (#1266) feat!: log stream (#1266) Jan 8, 2023
@Gustl22
Copy link
Collaborator Author

Gustl22 commented Jan 8, 2023

@luanpotter @spydon I unified the error and the logs in a single stream, or more specific: a stream for each player and a global stream to log stuff outside of player scope.

I just implemented it for Android, but wanted you to review before rewriting the other platforms. Thank you in advance for your feedback.

@spydon
Copy link
Member

spydon commented Jan 8, 2023

@luanpotter @spydon I unified the error and the logs in a single stream, or more specific: a stream for each player and a global stream to log stuff outside of player scope.

I just implemented it for Android, but wanted you to review before rewriting the other platforms. Thank you in advance for your feedback.

Looks like a great improvement to me! I have too little knowledge about any potential downsides to do a proper review, but I'm sure Luan will chime in.

_onLogStreamSubscription = _onLog.listen(logHandler);
}

static void setGlobalLogHandler(void Function(Log log)? logHandler) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm also considering to handle both streams in the Logger class and then be able to set only one logging handler in there, which would make it easier to handle all logs at once and would allow the preprocessing by the log level. Downside is, that you cannot differentiate between player logs and global logs anymore.

Copy link
Collaborator Author

@Gustl22 Gustl22 Mar 16, 2023

Choose a reason for hiding this comment

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

Now I solved it by exposing the log stream (per player and global), but also allow the user to disable logs/errors via Logger.logLevel. The log level now is not attached to the AudioPlayer anymore, as it can/should handle logs / errors from anywhere (like global ones, too).

@Gustl22 Gustl22 mentioned this pull request Jan 11, 2023
2 tasks
…stream

# Conflicts:
#	packages/audioplayers/example/test/logger_test.dart
#	packages/audioplayers_platform_interface/lib/global_platform_interface.dart
#	packages/audioplayers_platform_interface/lib/method_channel_audioplayers_platform.dart
@Gustl22 Gustl22 requested a review from wolfenrain March 24, 2023 11:35
@Gustl22
Copy link
Collaborator Author

Gustl22 commented Mar 24, 2023

I think the work is done now. I tried to split independent changes as Merge Requests beforehand, but this still is a bulky Merge Request. As I'm definitely no expert in C / C++ and Swift, it would be nice if someone can have a look into these. Or if having any concerns elsewhere in these changes.

@@ -4,6 +4,7 @@ publish_to: none

dependencies:
audioplayers: ^3.0.1
audioplayers_platform_interface: ^4.0.0
Copy link
Member

Choose a reason for hiding this comment

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

should the main audioplayers package expose anything that users my want to access? I think no one should have to depend on platform_interface directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. I decided to test the integration tests of audioplayers_platform_interface (which weren't present before) alos in the audioplayers package, to avoid having and maintaining two example apps. So therefore this dependency is listed here (not relevant for the actual example app). But I changed it to a dev dependency now.

@@ -219,10 +223,10 @@ class SoundPoolManager {
.setAudioAttributes(attrs)
.setMaxStreams(maxStreams)
.build()
Logger.info("Create SoundPool with $attrs")
ref.handleGlobalLog("Create SoundPool with $attrs")
Copy link
Member

Choose a reason for hiding this comment

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

should this call the player-specific log?

Copy link
Collaborator Author

@Gustl22 Gustl22 Apr 3, 2023

Choose a reason for hiding this comment

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

Creating a SoundPool is a global action in my opinion, as it can be used by multiple players. Although it is initiated by one of these. (And it would be a bit quirky to bring in the player scope in this area, as the specific player is not available here)

Copy link
Member

Choose a reason for hiding this comment

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

gotcha, agreed. forgot the soundpool was shared

}

// See: https://github.com/flutter/packages/blob/12609a2abbb0a30b9d32af7b73599bfc834e609e/packages/video_player/video_player_android/test/android_video_player_test.dart#L270
void createNativeEventStream({
Copy link
Member

Choose a reason for hiding this comment

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

for a followup: between this, MapParser/MethodCall extensions, OverridePrint, and more, there seems to be a lot of generic testing utils that could be extracted to their own package.
I think I could use them on gamepads for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah nice, didn't noticed you started this lib 🎉 Sure, if it's more practical. Feel free to move any generic testing stuff :)

Copy link
Member

@luanpotter luanpotter left a comment

Choose a reason for hiding this comment

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

Excellent refactor @Gustl22 🚀 👏 ❤️

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Great job! Would be good to add a paragraph to the migration doc so that the user knows what changes they have to do to migrate from the previous version.

packages/audioplayers/lib/src/global_audio_scope.dart Outdated Show resolved Hide resolved
packages/audioplayers/lib/src/log_level.dart Show resolved Hide resolved
@Gustl22
Copy link
Collaborator Author

Gustl22 commented Apr 3, 2023

Great job! Would be good to add a paragraph to the migration doc so that the user knows what changes they have to do to migrate from the previous version.

I would also do this in a separate MR, as it concerns the version migration and not this specific MR, where the relevant steps are in the description. The migration also includes other MR like #1443

I also wonder how you handle migration in flame. Maybe the Changelog is sufficient. At least we have to make more sections in the migration guide for the different versions. But maybe that's overengineered. Or we have to ditch stuff from the migration from v0 to v1. Whats are your proposals @spydon @luanpotter ? Thank you :D

@spydon
Copy link
Member

spydon commented Apr 3, 2023

I would also do this in a separate MR, as it concerns the version migration and not this specific MR, where the relevant steps are in the description. The migration also includes other MR like #1443

I also wonder how you handle migration in flame. Maybe the Changelog is sufficient. At least we have to make more sections in the migration guide for the different versions. But maybe that's overengineered.

In Flame we have the migration instructions in the bottom of the version in the changelog, so we could do the same here, and maybe mention that in the existing migration doc?

@Gustl22 Gustl22 mentioned this pull request Apr 3, 2023
1 task
@Gustl22 Gustl22 merged commit c9fd6a7 into main Apr 4, 2023
@Gustl22 Gustl22 deleted the gustl22/1266-error-stream branch April 4, 2023 10:06
Gustl22 added a commit that referenced this pull request Apr 4, 2023
# Description

- Upgrade min Dart to v2.7.0
- Upgrade min Flutter to v3.0.0
- Adapt some code to new Dart features

## Related Issues

#1352
Gustl22 added a commit that referenced this pull request Apr 9, 2023
# Description

As some of the classes introduced in #1352 are quite generic and could
conflict with other packages, we should rename them to be more specific
to the package:

- Refactor `GlobalEvent` to `GlobalAudioEvent`
- Refactor `PlayerEvent` to `AudioEvent`
- Refactor `Logger` to `AudioLogger`
- Refactor `LogLevel` to `AudioLogLevel`

PlayerEvent was renamed to have the same scheme as `GlobalAudioEvent`
and so that it is more applicable to Audio and not to all players in
general (e.g. media or video player).

## Breaking Change

This is a breaking change regarding the current master channel, but not
the published versions.

**audioplayers**
| Before | After |
|---|---|
| `GlobalEvent` | `GlobalAudioEvent` |
| `PlayerEvent` | `AudioEvent` |
| `Logger` | `AudioLogger` |
| `LogLevel` | `AudioLogLevel` |

**audioplayers_platform_interface**
| Before | After |
|---|---|
| `GlobalEvent` | `GlobalAudioEvent` |
| `PlayerEvent` | `AudioEvent` |

## Related Issues
#1352
Gustl22 added a commit that referenced this pull request Aug 16, 2023
# Description

Closes #1453.

### Migration instructions

See #1352 and #1443
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.

feat: how to handle platform error? option to disable logging Error handler never called
4 participants