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

Automating screenshots for documentation #501

Open
DJMcNab opened this issue Aug 9, 2024 · 10 comments
Open

Automating screenshots for documentation #501

DJMcNab opened this issue Aug 9, 2024 · 10 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed

Comments

@DJMcNab
Copy link
Member

DJMcNab commented Aug 9, 2024

Our documentation currently does not include screenshots, which can be instrumental in understanding the purpose of different widgets.

Since #475 (which landed as part of #233), we are able to store screenshots in this repository. We should find a way to include some of these screenshots in our documentation. For example, see TextInput in iced.

Ideally, this would be natively supported by rustdoc. This is tracked in rust-lang/rust#32104
However, we would like to have something working before this is resolved, as that issue has been open for a very long time.

The implementation pathway I would recommend is to host a GitHub pages site from a folder in this repository (https://docs.github.com/en/pages/getting-started-with-github-pages/configuring-a-publishing-source-for-your-github-pages-site#publishing-with-a-custom-github-actions-workflow). We would then use URLs to this site in our documentation.

I do foresee some concerns with this approach:

  1. This does not gracefully handle cases where the screenshot changes between versions of Xilem. This is because the website would show the updated version, even for old versions of the documentation. This is primarily a concern for when the screenshot would change, or if a specific screenshot would get removed. I think the easiest way to mitigate this would be by versioning the screenshots.
  2. This does not work for new screenshots in local documentation. I can't see a good way to work around this with the abilities provided by rustdoc, and so. For reviewing, the screenshots would be shown in the PR's diff view, and would become available as soon as the PR was merged.

However, this approach would still be a significant improvement from our current state.

@DJMcNab DJMcNab added documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed labels Aug 9, 2024
@Philipp-M
Copy link
Contributor

I wonder if that should also include snapshot testing?
I don't think we get around a custom solution anyway.

We could use the git commit versioned link I think like this: https://media.githubusercontent.com/media/linebender/vello/5aa5937d78dd1ec3b5b067dc173781858c38550e/vello_tests/snapshots/splash.png (after a quick skim of the response headers, I think it should work embedded in docs.rs).

So when a screenshot changes, "breaking" the (doc-)tests, then a local link to the screenshot has to be made to resolve the issue in CI.
And needs to be updated explicitly with a new commit (via a bot in CI, that scans for such local-changes to avoid git-commit-self-references?). I'm not sure about the details, and how that would work out in practice (e.g. does that count as LFS bandwidth? (needs some testing I guess)).

But it would at least make sure that all the screenshots are up to date and valid with the current (or following) commit.

@DJMcNab
Copy link
Member Author

DJMcNab commented Aug 12, 2024

I wonder if that should also include snapshot testing?

I don't understand what you're asking here. Wouldn't these screenshots already be snapshot tested?

I'm hesitant to use those Git commit URLs, for three reasons:

  1. I don't want us to be locked-in to those URLs provided by GitHub.
  2. The commit used would never/rarely be a commit which is actually on the main branch, which means there's a potential chance of pruning.
  3. It's not clear what the TOS/support for those URLs is.

@Philipp-M
Copy link
Contributor

The commit used would never/rarely be a commit which is actually on the main branch, which means there's a potential chance of pruning.

I meant commits based on main, i.e. have a local link to that file (something like ./path/to/screenshot) merge it with a PR into main, another (automated) CI creates a PR with an update with the fixed link of that previous commit hash (on main).
I don't think it needs to be specifically a link to github, though it would be convenient and save us an extra step with uploading these (versioned-snapshots) somewhere else (but I see the argument here).

We should find a way to include some of these screenshots

Ah yeah, that would include the snapshots, I think I've thought about some kind of doc-comment-tests, producing a screenshot, which would directly serve as snapshot-test

@PoignardAzur
Copy link
Contributor

My current plan is to not even bother creating a "book"-type documentation page, and just have all our documentation in rustdoc format, including tutorials.

See #632 for what that looks like.

The upside of doing that is we might not need any additional CI. I'm not sure this lets us use local file paths, though, unless whatever docs.rs does to pull a repository is git-lfs compatible.

@DJMcNab
Copy link
Member Author

DJMcNab commented Oct 7, 2024

Overall, that sounds like a good plan, but I struggle to see how that follows. Is this thread not discussing workarounds to let us have images in rustdoc?

There are two related root causes for this issue existing, which I perhaps didn't lay out clearly enough, but which are discussed in the linked rustdoc issue:

  • that rustdoc can't display images from the crate
  • that including images would increase the crate file size massively for very little gain to most users who use online docs

Additionally, docs.rs doesn't work based on the repositories, instead it works based on the uploaded crate. That allows it to work for any crate, not just those which happen to have a public repository.

@PoignardAzur
Copy link
Contributor

Oh, I see. Yeah, in that case, I have nothing.

@PoignardAzur
Copy link
Contributor

I've given it some thought, and I have a potential solution:

We do something similar to #663

Basically, we write an include_screenshot!(...) macro, which resolves to a Github URL under cfg(docrs), and to a local file path otherwise. Ideally the Github URL would be computed from the current commit hash, but I'm not sure if macros have access to that.

That way, docs.rs gets a link that point to a stable-ish outside source, but the source of truth remains a local URL. Maintenance work should be minimal.

@PoignardAzur
Copy link
Contributor

Quick check suggests the only way to get the current commit hash is from a build.rs script, which is a bit more overhead than I'm comfortable with. It's probably still worth it, but I'm hoping there's an easier workaround.

@DJMcNab
Copy link
Member Author

DJMcNab commented Oct 13, 2024

That's a really good solution - with one small detail.

Instead of using the commit hash, we can just use the tag (e.g. https://media.githubusercontent.com/media/linebender/vello/refs/tags/v0.3.0/vello_tests/snapshots/blurred_rounded_rect.png) gives Blurred rounded rectangles

@PoignardAzur
Copy link
Contributor

That's one direction we can explore. I don't know how diligent we are with tags, but if we do already use them, it's fine.

As far as I know env!("CARGO_PKG_VERSION") gives us the string we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants