-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nits
Thank you so much for being patient and going the extra mile to complete this PR!
...er/video_player/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerOptions.java
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_platform_interface/CHANGELOG.md
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_platform_interface/pubspec.yaml
Outdated
Show resolved
Hide resolved
74a37b4
to
b074e04
Compare
pushed with requested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. CI seems failing.
Yes the CI is failing because it cannot find the new class I added that is in the Interface package. Because it installs the latest released interface package. How to deal with such a situation? |
I see! You might want to create a separate PR for the interface change. And we land the interface PR, then we can land the plugin PR. |
I opened a PR with only changes for the interface package here: #2927 |
b074e04
to
d4cb25f
Compare
@wwwdata The interface change has been published |
d4cb25f
to
491c61e
Compare
hm, iOS podspec linting seems to be broken. What is causing it? I cannot really make sense of the error messages, can you help? |
|
hm, I did that in the
|
@wwwdata That's weird. I'm kicking off a re-run. |
hmm, seems to be the same issue. Is there anybody else that can help? I don't really see the concrete error message in the build log output as well. So I don't know how to fix it right now, unfortunately. |
@@ -4,7 +4,7 @@ description: Flutter plugin for displaying inline video with other Flutter | |||
# 0.10.y+z is compatible with 1.0.0, if you land a breaking change bump | |||
# the version to 2.0.0. | |||
# See more details: https://github.com/flutter/flutter/wiki/Package-migration-to-1.0.0 | |||
version: 0.10.11+2 | |||
version: 0.10.12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to depend on the newly published platform interface plugin here?
It's a bunch of null check static analyzer warnings, the line numbers are pointing to the right spot. Instead of
|
@jmagman interesting. Fixing all the lint "warnings" actually made the check green. What is confusing me about this is that the code was generated from code generation inside the Do those linting errors now pop up because of some linter rule changes maybe? It will be an issue the next time somebody runs the code auto-generation again. So I guess manually fixing it should not be the solution. |
f7db541
to
ec9e8bc
Compare
Ok, I think I got it now and the tests should become green. So It seems that the auto-generated code must have been manually edited. So I went over the diff and undid everything the code generator incorrectly generated. |
I just want to say thank you so much for working on this because I've been waiting on this to be pushed publicly for about a year now. Once this is published, I won't have to keep my fork of this going on. |
@gaaclarke Might know. |
@gaaclarke Looks like |
I spawned an issue for the analysis issue: flutter/flutter#63966 Thanks =) |
Offline discussion with @gaaclarke. We are ok with manually editing the auto-generated code to pass CI for now, and fix pigeon later. |
Master started timing out after this commit. I'm going to take a quick look and see if reverting it resolves the timeout. |
This reverts commit 77fbfcd.
Looks like this is not the cause of the time out. Relandded in #2939 |
…lutter#2938)" (flutter#2939) This reverts commit d4750ea.
…lutter#2938)" (flutter#2939) This reverts commit d4750ea.
Fixed flutter/flutter#66799 as well |
…lutter#2938)" (flutter#2939) This reverts commit d4750ea.
…lutter#2938)" (flutter#2939) This reverts commit d4750ea.
…lutter#2938)" (flutter#2939) This reverts commit d4750ea.
…lutter#2938)" (flutter#2939) This reverts commit d4750ea.
This is a replacement PR for #1174 because I lost access to the original repository I created the PR from.
How do I make the tests work? Unfortunately, I also had to modify the interface package.
@cyanglaz Pinging you like promised and maybe you can also help me out making the tests green and stuff :) But I have to say this new pigeon method of communicating with the native side is pretty awesome 👍
The only "dirty" thing was that when I created the new dart files, I lost some API test files that apparently were committed to the auto-generated file. I guess that is something that should be moved into a file that is not generated 😄
edited by cyanglaz: fixes flutter/flutter#30438