From 5fbbc8391e642626b8562ac5e263f9f713010268 Mon Sep 17 00:00:00 2001
From: Robert Lord <lard@google.com>
Date: Mon, 22 Apr 2019 19:56:11 +0000
Subject: [PATCH] [text] Switch TextFieldState from struct to table
Title says it all. We'd like this to be a table so that further upgrades
to this state object are easier.
This change also adds an alternative version of TextFieldState to three
Rust libraries, where the required fields on TextFieldState (document,
selection, revision) are T instead of Option<T>. Into/TryInto is also
implemented back and forth from the two types.
You'll notice that this implementation is duplicated in three
text_field_state.rs files, one for each project. I thought perhaps this
was better, since if we make changes to the table, each of these three
projects may want to upgrade to the new implementation separately?
Whereas if they all pulled text_field_state.rs from some common
location, we'd have to upgrade them all at once.
Change-Id: Icbb67498615b35e39825278a5c25c3d62fe7b0a0
---
garnet/bin/ui/ime/BUILD.gn | 1 +
garnet/bin/ui/ime/src/legacy_ime/handler.rs | 2 +-
garnet/bin/ui/ime/src/legacy_ime/state.rs | 13 ++-
.../bin/ui/text/default-hardware-ime/BUILD.gn | 1 +
.../ui/text/default-hardware-ime/src/main.rs | 6 +-
garnet/bin/ui/text/test_suite/BUILD.gn | 1 +
garnet/bin/ui/text/test_suite/src/main.rs | 3 +-
.../ui/text/test_suite/src/test_helpers.rs | 55 +++------
garnet/lib/ui/text/common/BUILD.gn | 28 +++++
garnet/lib/ui/text/common/src/lib.rs | 5 +
.../ui/text/common/src/text_field_state.rs | 105 ++++++++++++++++++
sdk/fidl/fuchsia.ui.text/text_field.fidl | 22 ++--
12 files changed, 182 insertions(+), 60 deletions(-)
create mode 100644 garnet/lib/ui/text/common/BUILD.gn
create mode 100644 garnet/lib/ui/text/common/src/lib.rs
create mode 100644 garnet/lib/ui/text/common/src/text_field_state.rs
diff --git a/garnet/bin/ui/ime/BUILD.gn b/garnet/bin/ui/ime/BUILD.gn
index 91ab54fbbe1..8f7cce9dd1b 100644
--- a/garnet/bin/ui/ime/BUILD.gn
+++ b/garnet/bin/ui/ime/BUILD.gn
@@ -10,6 +10,7 @@ rustc_binary("ime") {
edition = "2018"
deps = [
+ "//garnet/lib/ui/text/common:text_common",
"//garnet/public/lib/fidl/rust/fidl",
"//garnet/public/rust/fuchsia-async",
"//garnet/public/rust/fuchsia-component",
diff --git a/garnet/bin/ui/ime/src/legacy_ime/handler.rs b/garnet/bin/ui/ime/src/legacy_ime/handler.rs
index 9f907cf19a0..41555ca06d8 100644
--- a/garnet/bin/ui/ime/src/legacy_ime/handler.rs
+++ b/garnet/bin/ui/ime/src/legacy_ime/handler.rs
@@ -64,7 +64,7 @@ impl Ime {
let control_handle = stream.control_handle();
{
let mut state = await!(self_clone.0.lock());
- let res = control_handle.send_on_update(&mut state.as_text_field_state());
+ let res = control_handle.send_on_update(state.as_text_field_state().into());
if let Err(e) = res {
fx_log_err!("{}", e);
} else {
diff --git a/garnet/bin/ui/ime/src/legacy_ime/state.rs b/garnet/bin/ui/ime/src/legacy_ime/state.rs
index 2a0fec135e4..918cc90c0de 100644
--- a/garnet/bin/ui/ime/src/legacy_ime/state.rs
+++ b/garnet/bin/ui/ime/src/legacy_ime/state.rs
@@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+use text_common::text_field_state::TextFieldState;
use crate::fidl_helpers::clone_state;
use crate::ime_service::ImeService;
use crate::index_convert as idx;
@@ -129,9 +130,9 @@ impl ImeState {
) {
self.revision += 1;
self.text_points = HashMap::new();
- let mut state = self.as_text_field_state();
+ let state = self.as_text_field_state();
if let Some(input_method) = &self.input_method {
- if let Err(e) = input_method.send_on_update(&mut state) {
+ if let Err(e) = input_method.send_on_update(state.into()) {
fx_log_err!("error when sending update to TextField listener: {}", e);
}
}
@@ -156,8 +157,8 @@ impl ImeState {
}
/// Converts the current self.text_state (the IME API v1 representation of the text field's state)
- /// into the v2 representation txt::TextFieldState.
- pub fn as_text_field_state(&mut self) -> txt::TextFieldState {
+ /// into the v2 representation TextFieldState.
+ pub fn as_text_field_state(&mut self) -> TextFieldState {
let anchor_first = self.text_state.selection.base < self.text_state.selection.extent;
let composition = if self.text_state.composing.start < 0
|| self.text_state.composing.end < 0
@@ -171,7 +172,7 @@ impl ImeState {
} else {
txt::Range { start: end, end: start }
};
- Some(Box::new(text_range))
+ Some(text_range)
};
let selection = txt::Selection {
range: txt::Range {
@@ -193,7 +194,7 @@ impl ImeState {
},
affinity: txt::Affinity::Upstream,
};
- txt::TextFieldState {
+ TextFieldState {
document: txt::Range {
start: self.new_point(0),
end: self.new_point(self.text_state.text.len()),
diff --git a/garnet/bin/ui/text/default-hardware-ime/BUILD.gn b/garnet/bin/ui/text/default-hardware-ime/BUILD.gn
index cb97858def8..1560157b886 100644
--- a/garnet/bin/ui/text/default-hardware-ime/BUILD.gn
+++ b/garnet/bin/ui/text/default-hardware-ime/BUILD.gn
@@ -9,6 +9,7 @@ rustc_binary("bin") {
name = "default-hardware-ime"
edition = "2018"
deps = [
+ "//garnet/lib/ui/text/common:text_common",
"//garnet/public/lib/fidl/rust/fidl",
"//garnet/public/rust/fuchsia-async",
"//garnet/public/rust/fuchsia-component",
diff --git a/garnet/bin/ui/text/default-hardware-ime/src/main.rs b/garnet/bin/ui/text/default-hardware-ime/src/main.rs
index bf5e69964d3..d443d791b32 100644
--- a/garnet/bin/ui/text/default-hardware-ime/src/main.rs
+++ b/garnet/bin/ui/text/default-hardware-ime/src/main.rs
@@ -14,8 +14,10 @@ use futures::lock::Mutex;
use futures::prelude::*;
use serde_json::{self as json, Map, Value};
use std::collections::HashMap;
+use std::convert::TryInto;
use std::fs;
use std::sync::Arc;
+use text_common::text_field_state::TextFieldState;
const DEFAULT_LAYOUT_PATH: &'static str = "/pkg/data/us.json";
@@ -68,7 +70,7 @@ impl DefaultHardwareIme {
while let Some(evt) = await!(evt_stream.next()) {
match evt {
Ok(txt::TextFieldEvent::OnUpdate { state }) => {
- await!(this.0.lock()).on_update(state);
+ await!(this.0.lock()).on_update(state.try_into().unwrap());
}
Err(e) => {
fx_log_err!(
@@ -84,7 +86,7 @@ impl DefaultHardwareIme {
}
impl DefaultHardwareImeState {
- fn on_update(&mut self, state: txt::TextFieldState) {
+ fn on_update(&mut self, state: TextFieldState) {
self.last_selection = Some(state.selection);
self.last_revision = Some(state.revision);
}
diff --git a/garnet/bin/ui/text/test_suite/BUILD.gn b/garnet/bin/ui/text/test_suite/BUILD.gn
index f2908b9c829..43526746be8 100644
--- a/garnet/bin/ui/text/test_suite/BUILD.gn
+++ b/garnet/bin/ui/text/test_suite/BUILD.gn
@@ -12,6 +12,7 @@ rustc_binary("test_suite") {
edition = "2018"
deps = [
+ "//garnet/lib/ui/text/common:text_common",
"//garnet/public/lib/fidl/rust/fidl",
"//garnet/public/rust/fuchsia-async",
"//garnet/public/rust/fuchsia-component",
diff --git a/garnet/bin/ui/text/test_suite/src/main.rs b/garnet/bin/ui/text/test_suite/src/main.rs
index 3961739babb..a9587f1265a 100644
--- a/garnet/bin/ui/text/test_suite/src/main.rs
+++ b/garnet/bin/ui/text/test_suite/src/main.rs
@@ -46,8 +46,7 @@ fn main() -> Result<(), Error> {
let mut executor = fuchsia_async::Executor::new()
.context("Creating fuchsia_async executor for text tests failed")?;
let mut fs = ServiceFs::new();
- fs.dir("public")
- .add_fidl_service(bind_text_tester);
+ fs.dir("public").add_fidl_service(bind_text_tester);
fs.take_and_serve_directory_handle()?;
executor.run_singlethreaded(fs.collect::<()>());
Ok(())
diff --git a/garnet/bin/ui/text/test_suite/src/test_helpers.rs b/garnet/bin/ui/text/test_suite/src/test_helpers.rs
index 9bc2de63150..87045de699b 100644
--- a/garnet/bin/ui/text/test_suite/src/test_helpers.rs
+++ b/garnet/bin/ui/text/test_suite/src/test_helpers.rs
@@ -2,15 +2,17 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+use text_common::text_field_state::TextFieldState;
use failure::{bail, err_msg, Error, ResultExt};
use fidl_fuchsia_ui_text as txt;
use fuchsia_async::TimeoutExt;
use futures::prelude::*;
use std::collections::HashSet;
+use std::convert::TryInto;
pub struct TextFieldWrapper {
proxy: txt::TextFieldProxy,
- last_state: txt::TextFieldState,
+ last_state: TextFieldState,
defunct_point_ids: HashSet<u64>,
current_point_ids: HashSet<u64>,
}
@@ -37,38 +39,15 @@ impl TextFieldWrapper {
/// of the editing methods on the TextFieldWrapper, or if making calls on the proxy directly,
/// call `await!(text_field_wrapper.wait_for_update())` after you expect a new state update from
/// the TextField.
- pub fn state(&self) -> txt::TextFieldState {
- fn clone_range(range: &txt::Range) -> txt::Range {
- txt::Range {
- start: txt::Position { id: range.start.id },
- end: txt::Position { id: range.end.id },
- }
- }
- txt::TextFieldState {
- document: clone_range(&self.last_state.document),
- selection: txt::Selection {
- range: clone_range(&self.last_state.selection.range),
- anchor: self.last_state.selection.anchor,
- affinity: self.last_state.selection.affinity,
- },
- composition: self.last_state.composition.as_ref().map(|v| Box::new(clone_range(v))),
- composition_highlight: self
- .last_state
- .composition_highlight
- .as_ref()
- .map(|v| Box::new(clone_range(v))),
- dead_key_highlight: self
- .last_state
- .dead_key_highlight
- .as_ref()
- .map(|v| Box::new(clone_range(v))),
- revision: self.last_state.revision,
- }
+ pub fn state(&self) -> TextFieldState {
+ self.last_state.clone()
}
/// Waits for an on_update event from the TextFieldProxy, and updates the last state tracked
/// by TextFieldWrapper. Edit functions on TextFieldWrapper itself already call this; only
- /// use it if you're doing something with the TextFieldProxy directly.
+ /// use it if you're doing something with the TextFieldProxy directly. This also validates
+ /// that document, selection, and revision are all set on last_state, so these fields can be
+ /// unwrapped in other parts of the code.
pub async fn wait_for_update(&mut self) -> Result<(), Error> {
self.defunct_point_ids =
&self.defunct_point_ids | &all_point_ids_for_state(&self.last_state);
@@ -229,7 +208,7 @@ impl TextFieldWrapper {
}
}
-async fn get_update(text_field: &txt::TextFieldProxy) -> Result<txt::TextFieldState, Error> {
+async fn get_update(text_field: &txt::TextFieldProxy) -> Result<TextFieldState, Error> {
let mut stream = text_field.take_event_stream();
let msg_future = stream
.try_next()
@@ -237,11 +216,11 @@ async fn get_update(text_field: &txt::TextFieldProxy) -> Result<txt::TextFieldSt
.on_timeout(*crate::TEST_TIMEOUT, || Err(err_msg("Waiting for on_update event timed out")));
let msg = await!(msg_future)?.ok_or(err_msg("TextMgr event stream unexpectedly closed"))?;
match msg {
- txt::TextFieldEvent::OnUpdate { state, .. } => Ok(state),
+ txt::TextFieldEvent::OnUpdate { state, .. } => Ok(state.try_into()?),
}
}
-fn all_point_ids_for_state(state: &txt::TextFieldState) -> HashSet<u64> {
+fn all_point_ids_for_state(state: &TextFieldState) -> HashSet<u64> {
let mut point_ids = HashSet::new();
let mut point_ids_for_range = |range: &txt::Range| {
point_ids.insert(range.start.id);
@@ -261,8 +240,8 @@ mod test {
fn default_range(n: u64) -> txt::Range {
txt::Range { start: txt::Position { id: n }, end: txt::Position { id: n + 1 } }
}
- fn default_state(n: u64) -> txt::TextFieldState {
- txt::TextFieldState {
+ fn default_state(n: u64) -> TextFieldState {
+ TextFieldState {
document: default_range(n),
selection: txt::Selection {
range: default_range(n + 2),
@@ -284,7 +263,7 @@ mod test {
let (mut stream, control_handle) = server_end
.into_stream_and_control_handle()
.expect("Should have created stream and control handle");
- control_handle.send_on_update(&mut default_state(0)).expect("Should have sent update");
+ control_handle.send_on_update(default_state(0).into()).expect("Should have sent update");
fuchsia_async::spawn(
async {
let mut wrapper = await!(TextFieldWrapper::new(proxy))
@@ -327,20 +306,20 @@ mod test {
let (_stream, control_handle) = server_end
.into_stream_and_control_handle()
.expect("Should have created stream and control handle");
- control_handle.send_on_update(&mut default_state(0)).expect("Should have sent update");
+ control_handle.send_on_update(default_state(0).into()).expect("Should have sent update");
let mut wrapper =
await!(TextFieldWrapper::new(proxy)).expect("Should have created text field wrapper");
// send a valid update and make sure it works as expected
let mut state = default_state(10);
- control_handle.send_on_update(&mut state).expect("Should have sent update");
+ control_handle.send_on_update(state.clone().into()).expect("Should have sent update");
let res = await!(wrapper.wait_for_update());
assert!(res.is_ok());
assert_eq!(wrapper.state().document.start.id, 10);
// send an update with the same points but an incremented revision
state.revision += 1;
- control_handle.send_on_update(&mut state).expect("Should have sent update");
+ control_handle.send_on_update(state.into()).expect("Should have sent update");
let res = await!(wrapper.wait_for_update());
assert!(res.is_err()); // should fail since some points were reused
}
diff --git a/garnet/lib/ui/text/common/BUILD.gn b/garnet/lib/ui/text/common/BUILD.gn
new file mode 100644
index 00000000000..1bfb2df1dd7
--- /dev/null
+++ b/garnet/lib/ui/text/common/BUILD.gn
@@ -0,0 +1,28 @@
+# Copyright 2019 The Fuchsia 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("//build/rust/rustc_library.gni")
+import("//build/test/test_package.gni")
+
+rustc_library("text_common") {
+ with_unit_tests = true
+ edition = "2018"
+ deps = [
+ "//garnet/public/lib/fidl/rust/fidl",
+ "//garnet/public/rust/fuchsia-async",
+ "//garnet/public/rust/fuchsia-component",
+ "//garnet/public/rust/fuchsia-syslog",
+ "//garnet/public/rust/fuchsia-zircon",
+ "//sdk/fidl/fuchsia.ui.input:fuchsia.ui.input-rustc",
+ "//sdk/fidl/fuchsia.ui.text:fuchsia.ui.text-rustc",
+ "//sdk/fidl/fuchsia.ui.text.testing:fuchsia.ui.text.testing-rustc",
+ "//third_party/rust_crates:failure",
+ "//third_party/rust_crates:futures-preview",
+ "//third_party/rust_crates:lazy_static",
+ "//third_party/rust_crates:parking_lot",
+ "//third_party/rust_crates:pin-utils",
+ "//third_party/rust_crates:regex",
+ "//third_party/rust_crates:unicode-segmentation",
+ ]
+}
\ No newline at end of file
diff --git a/garnet/lib/ui/text/common/src/lib.rs b/garnet/lib/ui/text/common/src/lib.rs
new file mode 100644
index 00000000000..f1fa0f05213
--- /dev/null
+++ b/garnet/lib/ui/text/common/src/lib.rs
@@ -0,0 +1,5 @@
+// Copyright 2019 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+pub mod text_field_state;
diff --git a/garnet/lib/ui/text/common/src/text_field_state.rs b/garnet/lib/ui/text/common/src/text_field_state.rs
new file mode 100644
index 00000000000..dfc7ce0529b
--- /dev/null
+++ b/garnet/lib/ui/text/common/src/text_field_state.rs
@@ -0,0 +1,105 @@
+// Copyright 2019 The Fuchsia Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+use core::convert::TryFrom;
+use failure::{err_msg, Error};
+use fidl_fuchsia_ui_text as txt;
+
+/// A version of txt::TextFieldState that does not have mandatory fields wrapped in Options.
+/// It also implements Clone.
+pub struct TextFieldState {
+ pub document: txt::Range,
+ pub selection: txt::Selection,
+ pub revision: u64,
+ pub composition: Option<txt::Range>,
+ pub composition_highlight: Option<txt::Range>,
+ pub dead_key_highlight: Option<txt::Range>,
+}
+
+impl Clone for TextFieldState {
+ fn clone(&self) -> Self {
+ TextFieldState {
+ document: clone_range(&self.document),
+ selection: txt::Selection {
+ range: clone_range(&self.selection.range),
+ anchor: self.selection.anchor,
+ affinity: self.selection.affinity,
+ },
+ revision: self.revision,
+ composition: self.composition.as_ref().map(clone_range),
+ composition_highlight: self.composition_highlight.as_ref().map(clone_range),
+ dead_key_highlight: self.dead_key_highlight.as_ref().map(clone_range),
+ }
+ }
+}
+
+impl TryFrom<txt::TextFieldState> for TextFieldState {
+ type Error = Error;
+ fn try_from(state: txt::TextFieldState) -> Result<Self, Self::Error> {
+ let txt::TextFieldState {
+ revision,
+ selection,
+ document,
+ composition,
+ composition_highlight,
+ dead_key_highlight,
+ } = state;
+ let document = match document {
+ Some(v) => v,
+ None => {
+ return Err(err_msg(format!("Expected document field to be set on TextFieldState")))
+ }
+ };
+ let selection = match selection {
+ Some(v) => v,
+ None => {
+ return Err(err_msg(format!(
+ "Expected selection field to be set on TextFieldState"
+ )))
+ }
+ };
+ let revision = match revision {
+ Some(v) => v,
+ None => {
+ return Err(err_msg(format!("Expected revision field to be set on TextFieldState")))
+ }
+ };
+ Ok(TextFieldState {
+ document,
+ selection,
+ revision,
+ composition,
+ composition_highlight,
+ dead_key_highlight,
+ })
+ }
+}
+
+impl Into<txt::TextFieldState> for TextFieldState {
+ fn into(self) -> txt::TextFieldState {
+ let TextFieldState {
+ revision,
+ selection,
+ document,
+ composition,
+ composition_highlight,
+ dead_key_highlight,
+ } = self;
+ txt::TextFieldState {
+ document: Some(document),
+ selection: Some(selection),
+ revision: Some(revision),
+ composition,
+ composition_highlight,
+ dead_key_highlight,
+ }
+ }
+}
+
+fn clone_range(range: &txt::Range) -> txt::Range {
+ txt::Range {
+ start: txt::Position { id: range.start.id },
+ end: txt::Position { id: range.end.id },
+ }
+}
diff --git a/sdk/fidl/fuchsia.ui.text/text_field.fidl b/sdk/fidl/fuchsia.ui.text/text_field.fidl
index c2eae84c6da..1a334c28d2e 100644
--- a/sdk/fidl/fuchsia.ui.text/text_field.fidl
+++ b/sdk/fidl/fuchsia.ui.text/text_field.fidl
@@ -7,23 +7,23 @@ library fuchsia.ui.text;
/// Lists the Positions for selection and other related ranges, at a particular
/// revision number. Any time the revision number is incremented, all these Positions
/// become invalid, and a new TextFieldState is sent through OnUpdate.
-struct TextFieldState {
- /// The start and end of the entire text field.
- Range document;
+table TextFieldState {
+ /// (required) The start and end of the entire text field.
+ 1: Range document;
- /// The currently selected range of text.
- Selection selection;
+ /// (required) The currently selected range of text.
+ 2: Selection selection;
/// The range that indicates the text that is being composed, or currently
/// receiving suggestions from the keyboard. It should be displayed in some
/// distinct way, such as underlined.
- Range? composition;
+ 3: Range composition;
/// Some keyboards, notably Japanese, give the user buttons to highlight just a
/// subset of the composition string for suggestions. It must be equal to or a subset
/// of the composition range. If the composition range changes, the TextField may
/// discard this and require the keyboard to create a new one.
- Range? composition_highlight;
+ 4: Range composition_highlight;
/// A dead key is a key combination you press before another key to add diacritical
/// marks, accents, or other changes to the second key. After the first key, a
@@ -31,11 +31,11 @@ struct TextFieldState {
/// range is that highlighted character. If the selection moves away from this
/// highlight range, or if the contents of the highlight range change, the TextField
/// may discard this and require the keyboard to create a new one.
- Range? dead_key_highlight;
+ 5: Range dead_key_highlight;
- /// This number is increased any time content in the text field is changed, if the
- /// selection is changed, or if anything else about the state is changed.
- uint64 revision;
+ /// (required) This number is increased any time content in the text field is changed,
+ /// if the selection is changed, or if anything else about the state is changed.
+ 6: uint64 revision;
};
/// Indicates errors that can occur with various TextField methods. Until FIDL supports
--
GitLab