Skip to content

Make EditableText cursor configurable - #18888

Merged
sandrasandeep merged 26 commits into
flutter:masterfrom
sandrasandeep:cursor
Jul 3, 2018
Merged

sandrasandeep merged 26 commits into
flutter:masterfrom
sandrasandeep:cursor

Conversation

@sandrasandeep

@sandrasandeep sandrasandeep commented Jun 27, 2018 •

Copy link
Copy Markdown
Contributor

The cursor in text fields is currently unlike native iOS and Material cursors. This PR adds fields for cursor width and radius (how rounded the corners of the cursor RRect are) so that the cursor can (in future PRs) be configured to look more like the native.

I removed a global variable in RenderEditable called _kCaretWidth and replaced it with an instance variable called cursorWidth. The default value was changed from 1.0 to 2.0, as per the Apple UI Design Guidelines. Changing the default value caused a TextField test to fail, so I changed the test.

@Hixie

Hixie commented Jul 2, 2018

Copy link
Copy Markdown
Contributor

Try adding this to get the image base64 encoded:

diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart
index 5f960788a..ce0529b6b 100644
--- a/packages/flutter_goldens/lib/flutter_goldens.dart
+++ b/packages/flutter_goldens/lib/flutter_goldens.dart
@@ -1,23 +1,24 @@
 // Copyright 2018 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

 import 'dart:async';
+import 'dart:convert';
 import 'dart:io' as io;
 import 'dart:typed_data';

 import 'package:collection/collection.dart';
 import 'package:file/file.dart';
 import 'package:file/local.dart';
+import 'package:flutter/foundation.dart';
 import 'package:flutter_test/flutter_test.dart';
-import 'package:meta/meta.dart';
 import 'package:platform/platform.dart';
 import 'package:process/process.dart';

 // If you are here trying to figure out how to use golden files in the Flutter
 // repo itself, consider reading this wiki page:
 // https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package%3Aflutter

 const String _kFlutterRootKey = 'FLUTTER_ROOT';

 /// Main method that can be used in a `flutter_test_config.dart` file to set
@@ -81,22 +82,25 @@ class FlutterGoldenFileComparator implements GoldenFileComparator {
     return new FlutterGoldenFileComparator(goldens.repositoryRoot.childDirectory(testDirectoryRelativePath).uri);
   }

   @override
   Future<bool> compare(Uint8List imageBytes, Uri golden) async {
     final File goldenFile = _getGoldenFile(golden);
     if (!goldenFile.existsSync()) {
       throw new TestFailure('Could not be compared against non-existent file: "$golden"');
     }
     final List<int> goldenBytes = await goldenFile.readAsBytes();
-    // TODO(tvolkert): Improve the intelligence of this comparison.
-    return const ListEquality<int>().equals(goldenBytes, imageBytes);
+    if (const ListEquality<int>().equals(goldenBytes, imageBytes))
+      return true; // they match
+    final String encoded = base64.encode(imageBytes);
+    debugPrint('base64 encoded image:\n$encoded\nEND');
+    return false;
   }

   @override
   Future<void> update(Uri golden, Uint8List imageBytes) async {
     final File goldenFile = _getGoldenFile(golden);
     await goldenFile.parent.create(recursive: true);
     await goldenFile.writeAsBytes(imageBytes, flush: true);
   }

   File _getGoldenFile(Uri uri) {

@xster xster left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

this.ignorePointer = false,
bool obscureText = false,
Locale locale,
double cursorWidth = 2.0,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this value iOS specific? I would not introduce any skin specific opinions into the rendering package.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is iOS specific. Originally, the value was 1.0. I will change it back to that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also change the PR description and remove the claim that the default value has been changed?

if (_cursorRadius == value)
return;
_cursorRadius = value;
markNeedsLayout();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to re-layout here or just re-paint?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only needs re-paint. Made the change.

});

testWidgets('cursor layout has correct radius', (WidgetTester tester) async {
final GlobalKey<EditableTextState> editableTextKey = new GlobalKey<EditableTextState>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is correct. What's happened is that a space has been printed and the cursor is to the right of the space. The font that the golden file tests use renders a space as a blank/transparent block.

@sandrasandeep
sandrasandeep requested a review from goderbauer July 3, 2018 02:00
await tester.idle();
expect(tester.testTextInput.editingState['text'], equals('test'));
expect(tester.testTextInput.setClientArgs['inputAction'], equals(serializedActionName));
expect(tester.testTextInput.setClientArgs['inputAction'],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This file is hard to review because it contains so many unrelated format changes.

this.ignorePointer = false,
bool obscureText = false,
Locale locale,
double cursorWidth = 2.0,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also change the PR description and remove the claim that the default value has been changed?

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

);

expect(topLeft.dx, equals(399.0));
expect(topLeft.dx, equals(398.5));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have to be reverted as well now that the default has been reverted to 1.0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will do that. Thank you!

@xster xster left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (_cursorWidth == value)
return;
_cursorWidth = value;
markNeedsPaint();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you still need layout in this specific setter since _caretPrototype seems to use it.

@sandrasandeep
sandrasandeep merged commit 581df52 into flutter:master Jul 3, 2018
sandrasandeep added a commit that referenced this pull request Jul 3, 2018
@sandrasandeep
sandrasandeep deleted the cursor branch July 3, 2018 21:53
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants