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

Separate Dart and Flutter code #642

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

Conversation

hacker1024
Copy link
Contributor

The idea here is to make as much code as possible usable in Dart without Flutter, so a Dart-only audio_service-like library can use standard data types and handlers.

Ideally, with these changes, one should be able to write a single handler that can be used both with the audio_service Flutter plugin and a different non-Flutter library.

The changes were made in two stages:

  1. Transfer data classes to independent non-Flutter packages
  2. Transfer audio handlers and mixins to a non-Flutter package

A few API changes had to be made for this to happen:

  • AudioHandler no longer has a private constructor (as the audio_service package needs to extend it), though the documentation encourages use of the base class
  • BaseAudioHandler was stripped of its Flutter-specific code, and transferred. The audio_service package now contains BaseFlutterAudioHandler instead. To prevent incorrect usage of BaseAudioHandler, it must be imported explicitly or else it won't be accessible.
  • The toMessage functions in the message classes are public

Resolves #641.

@ryanheise
Copy link
Owner

This PR does several different things so there's a lot here worth discussing - maybe a good opportunity to try out a voice call on discord.

One thing to consider is that I'd eventually like to use pigeon, once it's more stable and feature complete. Pigeon can create a separate Dart file for all of the messages, but it won't put that into a separate package. I'm not sure if this restructuring would work with Pigeon.

One other comment is that it will also be easier to review a broad set of changes like this if they are separate pull requests. For example, moving Dart code into separate files while also editing the dart code makes it difficult to see which lines of code have changed. I would also hold off on splitting each class into a separate file because it's not related to the goal of this PR, and I'd like to evaluate each of those ideas independently. I'm not convinced of having 100 files.

@hacker1024
Copy link
Contributor Author

This PR does several different things so there's a lot here worth discussing - maybe a good opportunity to try out a voice call on discord.

Why a voice call? I'm open to it, but I generally prefer things in writing because:

  • Messages remain on record for later reference
  • The discussion is open to everybody

One thing to consider is that I'd eventually like to use pigeon, once it's more stable and feature complete. Pigeon can create a separate Dart file for all of the messages, but it won't put that into a separate package. I'm not sure if this restructuring would work with Pigeon.

The example linked in the Pigeon README actually does exactly this:
https://github.com/flutter/plugins/blob/89ccc0eadb2ce715da45af3b88bbc9c86c78ae3d/packages/video_player/video_player/pigeons/messages.dart#L61

One other comment is that it will also be easier to review a broad set of changes like this if they are separate pull requests. For example, moving Dart code into separate files while also editing the dart code makes it difficult to see which lines of code have changed.

I figured that may be the case. The split was more of a byproduct of me breaking down the classes to I could understand and navigate between them better, but I can undo that.

@ryanheise
Copy link
Owner

ryanheise commented Apr 9, 2021

Sorry I should probably explain the situation a bit more, but my medical condition has forced some lifestyle changes upon me. What has happened is that I have a pinched nerve in the neck which is causing nerve pain down my arm. It's something I wouldn't wish on anyone, and I hope other programmers heed the warning to maintain good posture and take regular breaks from sitting at a computer. But as mentioned in the other issue, I am unable to sit at a computer for long stretches, and I was hoping some of you may be open to voice communication for discussion of big issues (this one qualifies).

@hacker1024
Copy link
Contributor Author

Sorry, I didn't understand. Kudos to you for continuing to maintain these wonderful packages with your condition!
I'm happy to do a Discord call. What time is best for you? I'm in the AEST timezone.

@hacker1024 hacker1024 force-pushed the feature/flutter-independence branch from dfcbeca to bae3119 Compare April 9, 2021 03:26
@hacker1024
Copy link
Contributor Author

I've force pushed a new version that doesn't split up any files.
The old attempt is still avaliable on my feature/flutter-independence-old-split-classes branch.

@ryanheise ryanheise changed the base branch from one-isolate to master September 11, 2021 09:27
@ryanheise ryanheise changed the base branch from master to major October 17, 2021 09: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