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

Refactoring/convert to swift #228

Open
wants to merge 8 commits into
base: swift-implementation
Choose a base branch
from

Conversation

MohamedAbdallah-14
Copy link
Contributor

@MohamedAbdallah-14 MohamedAbdallah-14 commented Mar 31, 2020

As mentioned in Issue #218
Converted Objective c code to swift
Loading image from cache working
Added Control actions Feature (Play, Pause ... etc) in control center

@ryanheise
Copy link
Owner

Awesome! I'll give it a review in the morning.

@MohamedAbdallah-14
Copy link
Contributor Author

FYI
I tested custom actions on iOS and it's working fine
I think you should update readme

@ryanheise
Copy link
Owner

Hi @MohamedAbdallah-14

I really like your code in particular, and that you've filled in some missing functionality in respecting the controls parameter of setState. Although how difficult would it be to create a cleanroom implementation of the code you have copied from Jørgen Henrichsen's project? Namely, the remote command controller? It will be simpler to accept the PR if your PR corresponds to code that you have written yourself since the application of clause 6 of the GitHub TOS is a bit simpler. (Note that my copyright notice also attributes copyright to the project contributors and thus Jørgen Henrichsen not being the direct contributor isn't covered by my attribution).

Let me know, and otherwise I'd be happy to also have a crack at it.

@MohamedAbdallah-14
Copy link
Contributor Author

MohamedAbdallah-14 commented Apr 2, 2020

Hi @ryanheise
I see the problem but I don't know what is the best approach right now can I have your contact to decide what is the best way to proceed

@DGempler
Copy link
Contributor

DGempler commented Apr 4, 2020

Hello @ryanheise and @MohamedAbdallah-14

I ran into another bug using the existing obj-c implementation and switched to this version to see if it solved it. Unfortunately, it didn't, but here is what I had to add to the obj-c version to fix it. Basically clearing state & such on stopped after clearing nowPlayingInfo:

[MPNowPlayingInfoCenter defaultCenter].nowPlayingInfo = nil;
state = nil;
position = nil;
updateTime = nil;
speed = nil;
artwork = nil;
mediaItem = nil;
queue = nil;
startResult = nil;
result(@YES);

It looks like we will be moving towards swift in the near future, so if this could be added to the "TOS-approved" implementation that'd be great. I'm happy to do that as well once the this initial issue is figured out.

One more thing - print() outputs nothing to VS code. How do you debug (or at least get logs)? According to flutter/flutter#13204, NSLog() can be used instead of print() to get logging output to Flutter.

And @ryanheise if I should create a PR with these changes for the current implementation, let me know.

@ryanheise
Copy link
Owner

Hi @DGempler , thanks for finding another issue. Have you created an issue for it?

Probably what I'll do is create a swift branch to allow me to collaborate with @MohamedAbdallah-14 on the swift implementation, and then in the meantime you could still submit PRs in objective c for the current implementation. If the swift code is merged first, at least your PR would still be a helpful record of what the fix was, so feel welcome to still submit one if you like.

To the logging question, I've been using NSLog() and I believe there are some examples already in the code which seem to successfully show up in the logs.

@DGempler
Copy link
Contributor

DGempler commented Apr 6, 2020

@ryanheise yes just created the issue with linked PR: #237.

Regarding the use of NSLog(), I was suggesting to use it in this swift implementation, as print() outputs nothing (at least for me, on VS code).

@ryanheise
Copy link
Owner

@DGempler Ah, sorry I understand now. Yes, print does not show on the device console, but NSLog does, so I agree it's a good idea to use NSLog. Thanks for making the additional PR also.

@ryanheise ryanheise changed the base branch from master to swift-implementation April 7, 2020 13:03
@diego-lipinski-de-castro

Any preview for when this will be merged into master? I wanted to make updates for the iOS but Im not sure if I should waste time into swift or C

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.

4 participants