linebender / xilem

An experimental Rust native UI framework
Apache License 2.0
3.64k stars 115 forks source link

Automating screenshots for documentation #501

Open DJMcNab opened 2 months ago

DJMcNab commented 2 months ago

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 https://github.com/rust-lang/rust/issues/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.

Philipp-M commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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:

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 commented 3 weeks ago

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

PoignardAzur commented 2 weeks ago

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

We do something similar to https://github.com/linebender/xilem/pull/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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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.