Misc changes #311
Misc changes #311
Conversation
|
PS: it also add support for flutter master. (can be dropped easily) |
|
dropped the fix for flutter master to make it build. let see :) |
Codecov Report
@@ Coverage Diff @@
## master #311 +/- ##
=======================================
Coverage 91.89% 91.89%
=======================================
Files 11 11
Lines 1074 1074
=======================================
Hits 987 987
Misses 87 87 Continue to review full report at Codecov.
|
I'm going to update travis config to use master by default per our discussion. |
| @@ -53,7 +53,8 @@ abstract class Node extends LinkedListEntry<Node> { | |||
|
|
|||
| /// Offset in characters of this node in the document. | |||
| int get documentOffset { | |||
| final parentOffset = (_parent is! RootNode) ? _parent.documentOffset : 0; | |||
| final parentOffset = | |||
| (_parent is! RootNode && _parent != null) ? _parent.documentOffset : 0; | |||
There was a problem hiding this comment.
I've seen this come up quite a few times, but it's not clear what's the root cause here. In general _parent should never be null when you're asking for a documentOffset on a node.
It looks like the node was detached from the document tree when this is called. Do you have a reproduction for this issue? I think we should really have a test case for this.
There was a problem hiding this comment.
no I dont.
I found it with selection on the web, and thought it was safer this way.
| @@ -11,7 +11,7 @@ class ZefyrScaffold extends StatefulWidget { | |||
| static ZefyrScaffoldState of(BuildContext context) { | |||
| final _ZefyrScaffoldAccess widget = | |||
| context.dependOnInheritedWidgetOfExactType<_ZefyrScaffoldAccess>(); | |||
| return widget.scaffold; | |||
| return widget?.scaffold; | |||
There was a problem hiding this comment.
Scaffold is required for zefyr to work, what's the reason for null-check here?
There was a problem hiding this comment.
avoid crash.
I dont think a 'of' function should crash when the scaffold is not found. It can be a way to know if we have one or not.
| @@ -97,6 +97,9 @@ class InputConnectionController implements TextInputClient { | |||
| @override | |||
| void performAction(TextInputAction action) { | |||
| // no-op | |||
| final val = _lastKnownRemoteTextEditingValue; | |||
| onValueChanged(val.selection.start, '', '\n', | |||
| TextSelection.collapsed(offset: val.selection.start + 1)); | |||
There was a problem hiding this comment.
What's the logic behind this change?
There was a problem hiding this comment.
Pretty sure it's written in the commit. it fixes pressing enter in some cases. (I think in the Android emulator at least).
There was a problem hiding this comment.
Thanks, these are great findings!
There is 3 separate issues here, as I see it:
- documentOffset issue in notus
- performAction impelementation in zefyr
- null-check for ZefyrScaffold
It'd be easier to land these if they were in separate PRs, so that no one blocks the other. Wondering if you can split it even further?
|
Core rewrite in branch 1.0-dev (see #409) makes this PR irrelevant. |
Small changes grouped together.