This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Channel buffers #12167
Channel buffers #12167
Changes from all commits
12c4f89
8a8343d
cff180e
31a845b
c28999d
4253c1c
e19f59c
63f7b8a
4e87cb0
24607ab
c4bec51
ea3823f
afc09b6
0a3c609
9df8b23
f32af48
97be8ca
d041d62
1533428
42c742d
652a822
4d84d1b
33c21e9
afa7bb0
6b6dbb9
bcc5ea7
3ff518c
8d4a1e4
8e6ee3b
3e3c38d
a4204c9
1527c3b
837ed7e
23a9ce9
007edb1
741981b
571594e
478520f
8569f21
96761f8
adcd641
24cf927
67aecdb
b13a3dc
282bff0
a0a4c0e
26d9deb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
A per channel, fixed-size circular queue.
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.
There isn't anything about the implementation that requires this to be used for channels. It's a generic RingBuffer that could be used anywhere.
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.
doc
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.
done
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.
This doesn't say anything :) https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-useless-documentation
Something like
Add a callback when previously queued messages are being dropped due to overflow. The message instance is returned in the callback.
.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.
That seemed redundant since the docstring for _dropItemCallback says all of that. The intent was to point them to that documentation and not repeat it.
I found examples elsewhere and they put the ivar next to the setter and getter and it gets collapsed in the generated documentation so I don't have to repeat it, so done
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.
Yup, this works.
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.
Doc. Who is supposed to use it. Use See also: for references.
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.
Done.
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.
This should be once a callback is setup per channel on the BinaryMessenger now right?
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.
Yep, the companion PR needs to land as well for that to be true.
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.
Also document how this should be used. Should users instantiate these things themselves? Who needs to hook these up? Or is everything already done implicitly. If so, reference those classes.
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.
Done.
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.
Each field of a dart class has its own dartdoc page. For example https://api.flutter.dev/flutter/dart-ui/Offset/infinite-constant.html
It needs to be sufficiently complete to stand on its own.
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 can't see what you are referring to in this comment.
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 meant each field's doc needs to fully describe what they do, when they're used etc without the context of any surrounding fields' docs or the class doc since they appear independently when we generate dartdoc pages
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.
Ok, I added documentation even on private fields.
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 didn't mean adding documentations on private fields. I just meant each public field of public classes (appearing in their own URL with their own page in the API doc website) should fully describe themselves.
Consider, as example, seeing https://screenshot.googleplex.com/K4iw8SwzUuE vs https://screenshot.googleplex.com/QxCLRcoZFqY
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.
Is this API futureproof enough? When each channel can specify its own cache size, how would it express it? I assume via the channel constructor. When it does so, how does it pass that size int into this signature? Would we need to leak the implementation detail of this class's implementation into the channel class implementation (by forcing it to come call ChannelBuffers.resize)?
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 believe so. For basic channels the constructor will call
ui.channelBuffers.resize(_name, size)
. For method channels they will callui.channelBuffers.resize(_name + '/' + method, size)
each time they encounter a new method, not in the constructor.This comment is on the "push" method, I'm guessing it was suppose to be somewhere else.
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.
Ah ok, SG
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.
When would this happen? Wouldn't consumers check isEmpty first?
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.
They should be calling isEmpty. You know how it is, troublemakers don't follow the protocol.
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.
dem delinquent kids these days
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.
All public Dart classes and fields need dartdocs.
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.
done.
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.
This should probably not be a static but owned by someone. When there are multiple engines, each should (via some chain of ownership) end up with a different instance of buffers.
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.
Won't each engine have its own isolate and therefore its own channelBuffers? If this is true this will also be a problem for
ui.window
which I followed.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.
Yup you're right, that was a nonsensical comment :D
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.
Still needs a doc (for public fields)
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.
Done.
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.
Related to the comment on the other PR. I think there are 2 lifecycles and we can't really conflate them. There's setting the onPlatformMessage which happens once per binding, and there's adding per channel handlers which happens at arbitrary time.
Once the onPlatformMessage is attached, all buffers will stop queueing. But there are no guarantee that the following sequence of actions won't happen.
1- run entrypoint, entrypoint runApps, which attaches window.onPlatformMessage.
2- engine sends a message on some channel. It hits _DefaultBinaryMessenger. _handlers for that channel is null. Nothing happens. It's also not queued since onPlatformMessage is not null.
3- framework side code creates that channel and attaches the setMessageHandler for that channel. Nothing happens.
It probably works now since the synchronous execution of runApp+binding constructor+our system channels setting handler all happens at the same time. But we can't guarantee that for users' own channels.
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've addressed this in the other PR. Both cases get queued up now.
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.
dartdoc
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'm not sure what you are referring to. I updated the docstring to match the ones found in
lib/ui/window.dart