Add a native messaging bridge for the Web Extension #10
Conversation
ghost
requested review from
rust/Cargo.toml
Outdated
| @@ -8,7 +8,6 @@ description = "Cross Platform Library for providing To Do List data" | |||
|
|
|||
| [lib] | |||
| name = "toodle" | |||
| crate-type = ["staticlib", "cdylib"] | |||
There was a problem hiding this comment.
I'm not sure how to make this iOS and Android-only...
There was a problem hiding this comment.
[target.'cfg(target_os = "ios")'] would be used for iOS, [target.'cfg(target_os="android")'] for Android. It looks like you can use multiple flags, so this should work:
[lib]
name = "toodle"
[target.'cfg(target_os="android", target_os="ios")'.lib]
crate-type = ["staticlib", "cdylib"]
There was a problem hiding this comment.
This doesn't work for me, unfortunately. 😞
[lib]
name = "toodle"
[target.'cfg(any(target_os="android", target_os="ios"))'.lib]
crate-type = ["staticlib", "cdylib"]I get warning: unused manifest key: target.cfg(any(target_os="android", target_os="ios")).lib. Same with just [target.'cfg(target_os="ios"))'.lib].
In both cases, cargo lipo fails with Failed to parse 'cargo read-manifest' output: No target with kind 'staticlib' found.
toodlext/src/main.rs
Outdated
| labels: Option<Vec<LabelInfo>>, | ||
| } | ||
|
|
||
| impl ItemInfo { |
There was a problem hiding this comment.
Take a look at the Into trait. By implementing Into for a type then you get From for free, which means you don't need to have both the to and from functions. https://doc.rust-lang.org/std/convert/trait.Into.html
toodlext/src/main.rs
Outdated
| _ => Err(Error::IOError), | ||
| } | ||
| } | ||
| _ => Err(Error::IOError), |
There was a problem hiding this comment.
You could probably use map_err to even better effect here:
let length = input.read_u32::<NativeEndian>().map_err(|_err| { Error::IOError })?;
let mut message = input.take(length as u64);
let mut buffer = Vec::with_capacity(length as usize);
eprintln!("Reading request from browser");
message.read_to_end(&mut buffer).map_err(|_err|{ Error::IOError})?;
serde_json::from_slice(&buffer).map_err(|err| {
eprintln!("Error parsing request payload {:?}: {:?}",
String::from_utf8_lossy(&buffer), err);
Error::BadJSON
})
toodlext/src/main.rs
Outdated
| if let Some(ref new_item) = | ||
| toodle | ||
| .update_item(item, name, due_date, completion_date, None) | ||
| .ok() |
There was a problem hiding this comment.
Do you need this ok here? and_then should accept the value inside the Result as an arg, not the result itself - unless of course the result of update_item is a Result inside a Result, which I don't think it is.
toodlext/src/main.rs
Outdated
| Err(Error), | ||
| } | ||
|
|
||
| fn update_item_by_uuid(toodle: &mut Toodle, |
There was a problem hiding this comment.
master branch now has toodle_update_item_by_uuid, which should work well here.
There was a problem hiding this comment.
toodle_update_item_by_uuid is the unsafe FFI function, right? I'll add update_item_by_uuid to Toodle, and have the bridge and toodle_update_item_by_uuid use it.
ghost
mentioned this pull request
|
@grigoryk, @fluffyemily Please take another look when it's convenient. |
|
When you land this, can you also include a brief "How to build & deploy the web extension" note that I can then put into the README? |
Writing to stdout confuses the bridge, because the native messaging module tries to interpret the logs as length-prefixed JSON.
The bridge communicates with the Web Extension via length-prefixed JSON
messages over std{in, out}. It manages a single `Toodle` store, in a
fixed location, and translates the messages into method calls. The API
still needs some work: in particular, we always read before and after
writing; we can't set labels yet, because we need to fetch their IDs to
set them on items, and removing items and labels isn't supported yet.
Error handling and reporting could also be more robust.
For the bridge to work, the native manifest must live in a specific
location, and include a `path` key that points to the bridge binary.
Running `./install-native-manifest.js` automatically sets the path and
copies the manifest to the correct location on macOS. Windows and Linux
use different locations, but the install script doesn't handle them
yet. This only needs to be done once.
|
I ran out of time to dig into this today, but I'm getting errors in |
|
@kitcambridge try running |
|
Shenanigans. Thanks, @fluffyemily! |
The bridge communicates with the Web Extension via length-prefixed JSON
messages over std{in, out}. It manages a single
Toodlestore, in afixed location, and translates the messages into method calls. The API
still needs some work: in particular, we always read before and after
writing; we can't set labels yet, because we need to fetch their IDs to
set them on items, and removing items and labels isn't supported yet.
Error handling and reporting could also be more robust.
For the bridge to work, the native manifest must live in a specific
location, and include a
pathkey that points to the bridge binary.Running
./install-native-manifest.jsautomatically sets the path andcopies the manifest to the correct location on macOS. Windows and Linux
use different locations, but the install script doesn't handle them
yet. This only needs to be done once.