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

macOS Support #450

Merged
merged 1 commit into from
Sep 6, 2020
Merged

macOS Support #450

merged 1 commit into from
Sep 6, 2020

Conversation

hacker1024
Copy link
Contributor

This is my initial attempt at implementing macOS support. The changes mainly involved switching some UIKit components to their AppKit counterparts, and I also had to not use a separate isolate due to a missing macOS engine feature linked in #445.

The idea is that since the iOS and macOS implementations are so similar, I've used a few preprocessor directives to modify the few parts that need to change. I've also hardlinked the classes in the iOS and macOS directories on my system.

This means that the macOS and iOS classes are identical, and should preferably stay that way. Unfortunately, git doesn't acknowlege hardlinks, so they stay as two distinct files in this repo.

All that's missing now is custom action support. I'm waiting on that flutter issue to implement the isolate (which will fix custom actions), but in the meantime, perhaps streams could do the trick.

(Also, this is my first time writing Objective-C, so a more experienced iOS/macOS developer may want to check this before it is merged.)

@ryanheise
Copy link
Owner

Nice work! I also intend to look at the web PR and decide which one to merge first. I think probably it makes sense to merge the simpler one first (which may be the web one). In the meantime, what I was suggesting was to use symlinks rather than hard links. This is the approach recommended to me by the Google staff to share the iOS and macOS implementations.

@hacker1024
Copy link
Contributor Author

Flutter didn't seem to play nice with symlinks for me - it refused to build the app with symlinks.

That could be my machine though, so perhaps someone else can try.

@hacker1024
Copy link
Contributor Author

Also, would you then like me to rebase on the web PR? There will be one minor merge conflict after merging the web PR if I don't do so, and I'd also take advantage of the custom action code (I should be able to use some of the web PR's logic with macOS, too.)

@ryanheise
Copy link
Owner

Whatever way you approach the web PR probably won't affect things since I'll do a squash and merge if there are no conflicts.

For symlinks, this should work. You might need to either open the project in xcode to regenerate some indexes/indices or else maybe try removing some generated iOS files and get flutter to regenerate them. Otherwise, you can share the error you're getting here. By the way, the way I did this in just_audio was to have a separate directory called darwin/Classes and the symlinks pointed to there on the individual .m files but not the .h files.

@hacker1024
Copy link
Contributor Author

With symlinks, this is what I get with pod install:

Errno::ENOENT - No such file or directory @ realpath_rec - /Users/<USER>/IdeaProjects/<PROJECT>/audio_service/macos/Classes/ios

Something about the symlinking is breaking the path logic.

@hacker1024
Copy link
Contributor Author

I've added an AudioService.usesIsolate property. I need this, because if it's true I uses a SendPort to send data to my service, and if it's false I use a stream in a global variable.

@ryanheise
Copy link
Owner

Regarding the error with symlinks, I'd probably need to see what you attempted to do exactly. Perhaps you could create another branch with symlinks for me to look at?

@hacker1024
Copy link
Contributor Author

hacker1024 commented Sep 5, 2020

You can check out the PR branch at this commit: 0c06a24

I also tried the exact same darwin structure from just_audio, with similar errors.

@hacker1024 hacker1024 closed this Sep 5, 2020
@hacker1024 hacker1024 reopened this Sep 5, 2020
@hacker1024
Copy link
Contributor Author

Oops, accidentally hit close

@hacker1024
Copy link
Contributor Author

Other than the symlinking, everything seems to be working at this point, in both the example app and the one I'm developing. Once we solve the symlink issue, I'll promote this from draft status.

@ryanheise
Copy link
Owner

I just tried the symlink approach I described above and it worked. You can take a look at how I did it for both just_audio and more recently audio_session. Note the creation of a separate darwin directory with both ios and macos linking into it.

Aside from creating a separate darwin directory, I noticed that you also have two lines that differ from my macOS podspec for audio_session and just_audio:

  s.public_header_files = 'Classes/**/*.h'
  s.platform = :osx, '10.11'

On the second line, I'm not sure if you require a higher platform version. Anyway, please use the approach I took for audio_session and just_audio since that is the approach advised to me by Google.

@hacker1024
Copy link
Contributor Author

hacker1024 commented Sep 6, 2020

Turns out the problem was how I was actually running ln -s. Originally, I was in the root directory, and running:
ln -s darwin/Classes/*.m macos/Classes/

Apparently, I needed to actually do this:

cd macos/Classes/
ln -s ../../darwin/Classes/*.m .

It works fine now.

As for the platform version, much of the media stuff was only added in 10.12.2. It may be possible to get it to run on earlier versions by disabling a lot of stuff, but I haven't yet considered that. Are there enough people using a version lower than or equal to 10.11 (which is five years old) to make it an issue?

@hacker1024
Copy link
Contributor Author

Okay, marking as ready for review. Note that this is built on the web PR, and therefore includes all its changes as well. This is because it builds on some code added in the web PR.

@hacker1024 hacker1024 marked this pull request as ready for review September 6, 2020 04:05
@hacker1024
Copy link
Contributor Author

Rebased and squashed. The old, full commit history is now on my macos-old branch.

@ryanheise ryanheise merged commit 1b0a201 into ryanheise:master Sep 6, 2020
@ryanheise
Copy link
Owner

Awesome! I've now just merged it. And thanks for also spotting and fixing some important bugs along the way. I haven't fully tested it, but as I mentioned on the web PR, it is probably the best thing to do to just get it the PR out there and get more eyes on it.

@hacker1024
Copy link
Contributor Author

I'm noticing UI freezes when adding four songs at a time to a ConcatenatingAudioSource. Is this effecting anyone else? It wouldn't effect the iOS version because that runs in its own isolate + background task.

@ryanheise
Copy link
Owner

That's interesting. Does it freeze completely or momentarily, and if the latter, how long? It would be good if you can submit a bug report on just_audio.

@hacker1024
Copy link
Contributor Author

It's just momentarily, for a few seconds. I got confused; I'll see if I can rule out the possibility of my app's code causing the freezes and then I'll make an issue at just_audio.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants