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

Improve animint2pages by automatically taking screenshot of the animint #131

Merged
merged 47 commits into from
Oct 9, 2024

Conversation

siddhesh195
Copy link
Contributor

This pull request is a part of GSoC 2024 task of adding the feature of automatic screenshot in animint2pages function.

Added line Chromote session initialization as a starting step for using chromote screenshot API
@siddhesh195 siddhesh195 self-assigned this Jun 12, 2024
@tdhock
Copy link
Collaborator

tdhock commented Jun 12, 2024

great thanks for sharing. can yoiu please convert the PR to draft until it is ready for review?

@siddhesh195 siddhesh195 marked this pull request as draft June 12, 2024 16:31
added chromote in suggests for testing chromote's screenshot API
added chromote package for testing chromote screenshot API
changed the location of chromote init line in the code and added print to see local address of animint
used chromote's API of taking screenshot by navigating to url of local temporary directory
corrected variable name from chrome to chrome.session
changed chrome sessions captureScreenshot() API to chromote's screenshot() API to compare which option will be better.
added new test to see if screenshot.png file exists in github pages repo.
corrected hardcoded repo owner name to actual repo owner during the test by getting the repo owner from animint2pages
corrected hardcoded repo name to actual repo name during the test
test if gh-pages branch exist duting github actions testing
added code to write data from chromote screenshot api to local dir which has animint
code for server pointing to temp directory where dynamic html file is there
tests/testthat/test-compiler-ghpages.R Outdated Show resolved Hide resolved
@tdhock
Copy link
Collaborator

tdhock commented Aug 29, 2024

I manually tested the functionality by running the code below,

library(animint2)
mtcars$Cyl <- factor(mtcars$cyl)
viz <- animint(
  ggplot(mtcars, aes(x = mpg, y = disp, color=Cyl)) +
    geom_point(),
  ggplot(mtcars, aes(x = hp, y = wt, color=Cyl)) +
    geom_point(),
  title="Motor Trend Cars data viz",
  source="https://github.com/animint/animint2/blob/master/R/z_pages.R"
)
animint2pages(viz, "2024-08-29-screenshot-test")

which gave me this viz https://tdhock.github.io/2024-08-29-screenshot-test/ and the screenshot below,
Capture
overall the functionality looks great, but the widgets at the bottom are undesirable.
So I just pushed a commit which re-arranges the plots and widgets into a table, with a new <td class="plot_content"> so we can use screenshot(selector=".plot_content") to screenshot only the plots (not the widgets).
https://rstudio.github.io/chromote/reference/ChromoteSession.html#method-screenshot-

That results in the image below,
screenshot without widgets
(without widgets at the bottom)

@siddhesh195
Copy link
Contributor Author

I manually tested the functionality by running the code below,

library(animint2)
mtcars$Cyl <- factor(mtcars$cyl)
viz <- animint(
  ggplot(mtcars, aes(x = mpg, y = disp, color=Cyl)) +
    geom_point(),
  ggplot(mtcars, aes(x = hp, y = wt, color=Cyl)) +
    geom_point(),
  title="Motor Trend Cars data viz",
  source="https://github.com/animint/animint2/blob/master/R/z_pages.R"
)
animint2pages(viz, "2024-08-29-screenshot-test")

which gave me this viz https://tdhock.github.io/2024-08-29-screenshot-test/ and the screenshot below, Capture overall the functionality looks great, but the widgets at the bottom are undesirable. So I just pushed a commit which re-arranges the plots and widgets into a table, with a new <td class="plot_content"> so we can use screenshot(selector=".plot_content") to screenshot only the plots (not the widgets). https://rstudio.github.io/chromote/reference/ChromoteSession.html#method-screenshot-

That results in the image below, screenshot without widgets (without widgets at the bottom)

yes. now the screenshots are better. widgets are not necessary in the screenshot. thank you for pointing that out and making a commit. I will look into it to understand the changes you made

R/z_pages.R Outdated
res <- animint2dir(plot.list, open.browser = FALSE, ...)
portNum <- servr::random_port()
normDir <- normalizePath(res$out.dir, winslash = "/", mustWork = TRUE)
code = sprintf("servr::httd(dir='%s', port=%d)", normDir, portNum)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this functionality (starting a server using servr::httd) seems duplicated here, and when printing animint, and in tests_init. is there some way to combine the repeated logic into a helper function?

@siddhesh195
Copy link
Contributor Author

@tdhock, I resolved all the comments. Can you please have another look to see if the PR is now ready to be merged ?

R/z_pages.R Outdated Show resolved Hide resolved
tests/testthat/helper-functions.R Outdated Show resolved Hide resolved
R/utilities.r Show resolved Hide resolved
@siddhesh195
Copy link
Contributor Author

hello @tdhock , is there anything more that needs to refactored before merging this PR ?

R/z_animint.R Outdated Show resolved Hide resolved
NAMESPACE Outdated
@@ -477,6 +478,7 @@ export(stat_summary_bin)
export(stat_summary_hex)
export(stat_unique)
export(stat_ydensity)
export(stop_server)
Copy link
Collaborator

Choose a reason for hiding this comment

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

two different spellings of server, in start_servr and stop_server, is potentially confusing. please change to stop_servr for consistency.

@tdhock
Copy link
Collaborator

tdhock commented Oct 9, 2024

great, thanks for your patience! this is a really useful contribution.

@tdhock tdhock merged commit 5842300 into master Oct 9, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants