[video_player] Improve DartDocs and test coverage #2286
Conversation
| /// For example, assume that the entire video is 4 minutes long. If [end] has a | ||
| /// duration of two minutes, this will return `0.5` since the DurationRange | ||
| /// ends 50% of the way through the video's total length. |
There was a problem hiding this comment.
Maybe use a similar example with the startFraction so it's easier to understand. 0.5 is kind of ambitious.
maybe 1 min -> 0.75 -> 75%
| /// Whether or not [errorDescription] is null. | ||
| bool get hasError => errorDescription != null; | ||
|
|
||
| /// Returns [size.width] / [size.height] when size is non-null, or `1.0.` when |
There was a problem hiding this comment.
I think in this specific case (unlike the other two) this is actually explaining what the getter does in a way that's hard to simplify. An aspect ratio really is either width / height or height / width, and I think it's useful to know what happens in strange edge cases where there aren't any dimensions loaded yet.
| /// This finishes as soon as the "play" command has been sent to the platform, | ||
| /// not when playback is complete. |
There was a problem hiding this comment.
maybe: This method returns as soon as the playback has started, not when it is complete.
There was a problem hiding this comment.
This is referring specifically to the future. It completes once "play" itself starts, not when playback is complete. I thought that was potentially confusing so I mentioned it. I'll clear this up to say "The future that is returned from this method finishes as soon as..."
| /// controlled by [controller]. | ||
| /// | ||
| /// Defaults will be used for everything except [controller] if they're not | ||
| /// provided. [allowScrubbing] defaults to false, and [padding] will default |
There was a problem hiding this comment.
nits: The [allowingScrubing]...
I think we don't start sentences with small letter characters
There was a problem hiding this comment.
It's weird to start off a sentence with "[a", but I think this is actually correct here for a few reasons.
Using "the" would imply that allowScrubbing is significant somehow--that it's a specific allowScrubbing out of multiple possible allowScrubbings out there, or maybe somehow very important. Similarly if I were to use "a/an" I would be implying a random allowScrubbing out of the various allowScrubbings available. But In this case there is only one parameter called allowScrubbing, so it doesn't make sense to use either article. I'm referring to the one and only allowScrubbing throughout, and it's just a regular parameter like any other one.
And then after that normally sentences do start with upper case letters. This is where it gets into extremely confusing territory because prescriptive grammarians haven't really caught on to coding documentation yet, but here I'm basically using [allowScrubbing] as a name that is also lower cased. That's unusual for English, but when it does happen it is generally considered to be correct to just use the proper name as it's normally capitalized instead of upper casing it, the same if I were talking about iPhones. See https://english.stackexchange.com/a/68665 for some discussion on it.
That said, English doesn't really have real grammar rules so this is all very subjective.
Also adds a lint to prevent undocumented APIs going forward.
Also adds a lint to prevent undocumented APIs going forward.
3 participants
Description
Also adds a lint to prevent undocumented APIs going forward.
Related Issues
Fixes flutter/flutter#44405.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?