Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

save chart as a png #3

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions examples/scatterplot.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
use procyon::*;
use showata::Showable;
use std::path::{Path, PathBuf};

fn main() -> Result<(), Box<dyn std::error::Error>> {
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let chart = Procyon::chart(
"https://raw.githubusercontent.com/procyon-rs/vega_lite_4.rs/master/examples/res/data/clustered_data.csv"
)
.mark_point()
.encode_axis("x", "y").encode_color("cluster")
.build();
.save(Path::new("/Users/aubrythomas/src/personal-git/procyon-rs/procyon/image.png")).await?;
Copy link
Member

Choose a reason for hiding this comment

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

you should add a new example instead of modifying an existing one...
and this path won't work...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes there will be scatterplot ans save_png


eprintln!("{:?}", chart);
chart.show().unwrap();
//chart.show().unwrap();

Ok(())
}
93 changes: 90 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! High level helpers to create beautiful graphs based on Vega-Lite

// warnings,
#![deny(
warnings,
Copy link
Member

Choose a reason for hiding this comment

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

removing warning is ok while under dev, please don't commit it

missing_debug_implementations,
trivial_casts,
trivial_numeric_casts,
Expand All @@ -13,7 +13,15 @@
)]

pub use showata::Showable;

//
use base64;
use fantoccini::{Client, Locator};
use retry::{delay::Fixed, retry, OperationResult};
Copy link
Member

Choose a reason for hiding this comment

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

could you also commit the cargo.toml changes for those dependencies

Copy link
Contributor Author

@ThomAub ThomAub May 5, 2020

Choose a reason for hiding this comment

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

Yes but also remember to update the Cargo.toml to point to the pending PR#15 in showata if you want to try locally

use std::fs::File;
use std::io::Write;
use std::path::Path;
use std::process::Command;
use std::{thread, time};
mockersf marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(feature = "vega_lite_4")]
mod vega_lite_4_bindings;

Expand Down Expand Up @@ -115,7 +123,86 @@ impl Procyon {

new
}

/// save the image
Copy link
Member

Choose a reason for hiding this comment

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

not enough as doc, it should mention that this methods needs gecko driver to be in path, firefox installed, the errors that can be thrown, and what the user can expect to happen when calling this method

Copy link
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 add a bit more doc ;) let's discuss on the format of the doc because i'm not sure how to list all possible errors and all.

pub async fn save(&self, image_path: &Path) -> Result<(), Box<dyn std::error::Error>> {
// Spawn in background a webdriver
// Currently geckodriver is hardcoded but should support
// all webdriver compliant driver.
// TODO:updated to all webdriver
let mut webdriver_process = Command::new("geckodriver")
.args(&["--port", "4444"])
Copy link
Member

Choose a reason for hiding this comment

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

if port 4444 is already taken this is going to fail...
you should take a random port, and retry until it doesn't fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will split the spawn of the webdriver connection to it in separate functions

.spawn()
.expect("webdriver failed to start");
// This should also be TODO:updated to all webdriver
let mut caps = serde_json::map::Map::new();
let opts = serde_json::json!({ "args": ["--headless"]});
Copy link
Member

Choose a reason for hiding this comment

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

the json string for this should be written directly, as it's not useful to compute it at runtime

Choose a reason for hiding this comment

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

This creates a serde_json::Value. If it is necessary to create such a Value I guess the macro use is okay.
If you create the Map of values and always serialize it as the same string, it may be not. But the call to Client::with_capabilities seems to expect a Map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is code taken from the example of fantoccini

caps.insert("moz:firefoxOptions".to_string(), opts);

let mut connection_ok = false;
while !connection_ok {
Copy link
Member

Choose a reason for hiding this comment

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

this may be an infinite loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using the crate retry and retry_futur.
What do you think of it ?

thread::sleep(time::Duration::from_secs(5));
Copy link
Member

Choose a reason for hiding this comment

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

probably not a good idea to sleep the thread in an async context, but I don't know how do to that without depending on the runtime

Choose a reason for hiding this comment

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

This, unfortunately, means that there is no way to wait less than 5 seconds to save a plot.
Maybe start with a much smaller delay and then exponentially backoff.

Maybe it is worth splitting the method of creating a headless Browser, for instance Chromium or Firefox, from creating and storing plots in the instance.

By doing this, users can decide to call a method that first starts the Browser and then saves the plot and tears down the Browser, or to keep the instance alive and reuse it.

Of course, this looks like a future task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will split it now because it will add readability and we can talk more specifically about this function signature and possible use.

// This should also be TODO:updated to all webdriver
let mut caps = serde_json::map::Map::new();
let opts = serde_json::json!({ "args": ["--headless"]});
caps.insert("moz:firefoxOptions".to_string(), opts);
Copy link
Member

Choose a reason for hiding this comment

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

should reuse the caps created above instead of recreating one

let mut connection_ok = Client::with_capabilities("http://localhost:4444", caps)
.await
.is_err();
Copy link
Member

Choose a reason for hiding this comment

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

in case of success, the client established is thrown away, does this have side effects?

if connection_ok == false {
Copy link
Member

Choose a reason for hiding this comment

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

you should avoid a bool variable used in a if == a bool, and using negative as it degrades readability

if Client::with_capabilities("http://localhost:4444", caps).await.is_ok() {
    break;
}

does the same thing

break;
};
}
let mut c = Client::with_capabilities("http://localhost:4444", caps).await?;
// let mut c = retry(Fixed::from_millis(100), || {
// match Some(c.as_ref().unwrap()) {
// Some(Client { tx, is_legacy }) => OperationResult::Ok(c),
// _ => OperationResult::Retry("not connected yet"),
// }
// })
// .unwrap()?;

//.expect("failed to connect to WebDriver");

// Current implem use the saved html by showata and imitate user clik to save the image
// Another approach is to updated the embeded js like in altair:
// https://github.com/altair-viz/altair_saver/blob/master/altair_saver/savers/_selenium.py
dbg!(self.build().to_html_file()?.to_str().unwrap());
c.goto(&format!(
"file:///private{}",
self.build().to_html_file()?.to_str().unwrap()
))
.await?;
// c.goto("file:///private/var/folders/32/h6lt_67s75g6jf3h4hx_myxc0000gn/T/showata/show-1587655243189631000.html").await?;
let mut summary_button = c
Copy link
Member

Choose a reason for hiding this comment

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

the click method returns the client and not the element, you should just reassign it to c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was to help with the understanding of the navigation when debugging. Final version can have Client only.

.wait_for_find(Locator::Css("summary"))
.await?
.click()
.await?;
let mut hidden_link = summary_button
.find(Locator::LinkText("Save as PNG"))
.await?
.click()
.await?;

let link = hidden_link
.find(Locator::LinkText("Save as PNG"))
.await?
.attr("href")
.await?;

let image_data_url = link.unwrap();
//let data64: Vec<&str> = image_data_url.split(',').collect::<Vec<&str>>();
//dbg!(data64[1]);
//let bytes: Vec<u8> = base64::decode(data64[1]).unwrap();
// "png;base64,iVB"
let _format = &image_data_url[11..14];
let bytes: Vec<u8> = base64::decode(&image_data_url[22..]).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

not sure if it's worth the cost, but there is a crate to parse data url : https://crates.io/crates/data-url
instead of magic numbers...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you don't like magic numbers 😄 I will look into it ans see if i can help for svgformat

Copy link
Member

Choose a reason for hiding this comment

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

no need to introduce a crate just for that, but you can replace the magic number by the "formula" used to compute eg: something like "data:".length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw you comment a bit late. Current version of the code use DataUrl but we can discuss about it.

let mut image_file = File::create(image_path).unwrap();
image_file.write(&bytes).unwrap();
hidden_link.close().await;
webdriver_process.kill();
Ok(())
}
/// Build the graph
#[cfg(feature = "vega_lite_4")]
pub fn build(&self) -> vega_lite_4::Vegalite {
Expand Down