@cyanglaz any chance we can merge this now that the null safe version is finished? 🤔

@cyanglaz @iskakaushik any updates here?

@cyanglaz @iskakaushik @stuartmorgan what do we need to do here to move this forward?

Sorry for the delay; we're still working on triaging the PR backlog. From an initial look this will need tests, per policy, and it will need non-trivial review, so will need to go into the backlog.

To elaborate on the second point:

• As described in the PR comment and the comment in the code this is essentially hacking around an issue we don't actually understand yet, which is not something we generally do until we've done more investigation, and
• It's not clear from the PR how you came to the conclusion that "setting every 500ms does not possibly impact performance" given that it's doing a method channel invocation, which happens on the main thread.

From an initial look this will need tests

@stuartmorgan if you have any idea how to test this, would love to know.

It's not clear from the PR how you came to the conclusion that "setting every 500ms does not possibly impact performance" given that it's doing a method channel invocation, which happens on the main thread.

To be honest, I wrote some of the other code and this is the least impact but it is quite a critical bug (e.g. we cannot use the plugin without this PR).

It would just be nice to move this forward, given we have been on our own fork for 9 months for a 1 line change.

@stuartmorgan your concerns are great but also idealistic.

This is a major bug that has not been addressed for a whole year. Merging this would fix it.

Your concern is valid, however, negligible. The value is updated so often that the single call has 0 measurable effect.

I am all in favor of a better fix. What I would do: merge this and create an issue to clean this up.

And if your concern is quality of code, I am fully with you. If I owned this plugin, the first thing I would do is clean up all of the wanky handling.

The value is not stable at all, which causes this problem in the first place. You cannot trust the value one bit - there are so many ways to bring the value (Dart side) out of sync with the actual underlying video player state (native side).

@stuartmorgan your concerns are great but also idealistic.

I'm willing to listen to explanations for why a native-side fix that has none of those drawbacks would be hard to implement.

Your concern is valid, however, negligible.

You asked me what was necessary to move the PR forward; asserting that my comments don't matter isn't going to move it forward.

And if your concern is quality of code, I am fully with you. If I owned this plugin, the first thing I would do is clean up all of the wanky handling.

Adding hacks instead of addressing root causes directly works against that goal. If the first thing you would do is improve code quality, then I'm not sure why you are arguing against improving this PR as part of the review process.

The value is not stable at all, which causes this problem in the first place. You cannot trust the value one bit - there are so many ways to bring the value (Dart side) out of sync with the actual underlying video player state (native side).

I'm happy to review PRs that address root causes of out-of-sync values, rather than layering hacks over them.

@stuartmorgan

Adding hacks instead of addressing root causes directly works against that goal. If the first thing you would do is improve code quality, then I'm not sure why you are arguing against improving this PR as part of the review process.

I am not at all. What I am trying to say is that I am not familiar with the native side and therefore I cannot move this PR in the direction you pointed out.

Thus, I would merge this and create an issue to fix up the native side once there is someone with the knowledge to do so.

(I would have fixed it on the native side if I knew how - I am sure I have the ability to learn how to do it quickly, but not quickly enough that I have the time to do it, unfortunately)

If the first thing you would do is improve code quality, then I'm not sure why you are arguing against improving this PR as part of the review process.

The issue is that I do not have the capacity to do it. Ideally, I would do it 😕

Based on the comments above it doesn't sound like this PR is going to reach a point where it is landable, so I'm going to close it. If at some point in the future you are interested in revisiting a fix that addresses the comments above, please feel free to open a new PR.

#4331 should be the iOS-side fix 🎉

But it is also lacking unit tests right now..