save chart as a png #3

ThomAub · 2020-04-30T14:59:42Z

This is a first working solution but it still need a lot a work. It will address the issue #2

…ly support for firefox and png at the moment

mockersf · 2020-04-30T19:37:38Z

examples/scatterplot.rs

    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?;


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

yes there will be scatterplot ans save_png

mockersf · 2020-04-30T19:38:23Z

src/lib.rs

 #![deny(
-    warnings,


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

mockersf · 2020-04-30T19:39:07Z

src/lib.rs

+use base64;
+use fantoccini::{Client, Locator};
+use retry::{delay::Fixed, retry, OperationResult};


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

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

src/lib.rs

mockersf · 2020-04-30T19:45:55Z

src/lib.rs

@@ -115,7 +123,86 @@ impl Procyon {

        new
    }
-
+    /// save the image


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

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.

mockersf · 2020-04-30T19:53:15Z

src/lib.rs

+        caps.insert("moz:firefoxOptions".to_string(), opts);
+
+        let mut connection_ok = false;
+        while !connection_ok {


this may be an infinite loop

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

mockersf · 2020-04-30T19:54:45Z

src/lib.rs

+            caps.insert("moz:firefoxOptions".to_string(), opts);
+            let mut connection_ok = Client::with_capabilities("http://localhost:4444", caps)
+                .await
+                .is_err();


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

mockersf · 2020-04-30T19:56:57Z

src/lib.rs

+            let mut connection_ok = Client::with_capabilities("http://localhost:4444", caps)
+                .await
+                .is_err();
+            if connection_ok == false {


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

mockersf · 2020-04-30T19:59:27Z

src/lib.rs

+        ))
+        .await?;
+        // c.goto("file:///private/var/folders/32/h6lt_67s75g6jf3h4hx_myxc0000gn/T/showata/show-1587655243189631000.html").await?;
+        let mut summary_button = c


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

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

mockersf · 2020-04-30T20:10:21Z

src/lib.rs

+        let _format = &image_data_url[11..14];
+        let bytes: Vec<u8> = base64::decode(&image_data_url[22..]).unwrap();


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...

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

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

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

ChristianBeilschmidt · 2020-05-04T08:30:41Z

src/lib.rs

+
+        let mut connection_ok = false;
+        while !connection_ok {
+            thread::sleep(time::Duration::from_secs(5));


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.

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

…s with and used crate retry to handle the retry loop. Now use DataUrl to parse url content. Added save_png as a new example and added cargo.toml

mockersf · 2020-05-05T17:33:43Z

src/lib.rs

+/// The chrome case will be commented for now and will be tested later.str
+/// It also need the port from the webdriver.
+pub async fn create_headless_client(
+    client_type: &str,


could this be an enum that would force user to specify a supported browser instead of a free string?

:sparkless: use headless firefow with fantocinni to save the plot. on…

a0ffbe3

…ly support for firefox and png at the moment

ThomAub requested review from mockersf and davidB April 30, 2020 15:00

ThomAub marked this pull request as draft April 30, 2020 15:00

ThomAub self-assigned this Apr 30, 2020

ThomAub added the enhancement New feature or request label Apr 30, 2020

mockersf requested changes Apr 30, 2020

View reviewed changes

ChristianBeilschmidt reviewed May 4, 2020

View reviewed changes

:refactor: move creation of webdriver and client to seperate function…

2f2faa0

…s with and used crate retry to handle the retry loop. Now use DataUrl to parse url content. Added save_png as a new example and added cargo.toml

mockersf reviewed May 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

save chart as a png #3

save chart as a png #3

ThomAub commented Apr 30, 2020 •

edited

Loading

mockersf Apr 30, 2020

ThomAub May 5, 2020

mockersf Apr 30, 2020

mockersf Apr 30, 2020

ThomAub May 5, 2020 •

edited

Loading

mockersf Apr 30, 2020

ThomAub May 5, 2020

mockersf Apr 30, 2020

ThomAub May 5, 2020

mockersf Apr 30, 2020

mockersf Apr 30, 2020

mockersf Apr 30, 2020

ThomAub May 5, 2020

mockersf Apr 30, 2020

ThomAub May 5, 2020

davidB May 5, 2020

ThomAub May 5, 2020

ChristianBeilschmidt May 4, 2020

ThomAub May 5, 2020

mockersf May 5, 2020

		let _format = &image_data_url[11..14];
		let bytes: Vec<u8> = base64::decode(&image_data_url[22..]).unwrap();

save chart as a png #3

Are you sure you want to change the base?

save chart as a png #3

Conversation

ThomAub commented Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomAub May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomAub commented Apr 30, 2020 •

edited

Loading

ThomAub May 5, 2020 •

edited

Loading