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

Add external docs url config option #1386

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

AaronErhardt
Copy link
Contributor

Many gtk-rs crates have license issues with their docs, so they exclude most docs from their source and build their documentation separately. However, doc.rs will always build the documentation, even if Cargo.toml specifies a different doc url.Sites like lib.rs even list the doc.rs docs as "API reference" and list it before the actual documentation. So overall, it's not unlikely for new users to end up on docs.rs and wonder why the docs are so bad.

Further, SEO is a real problem with the docs. Searching for libadwaita docs gives you the link to the Flutter rip-off, then an Arch package and then the docs built for Relm4 (which are just as bad the docs.rs docs). You can't even find the actual docs through DDG it seems. One way to improve this is to generate a lot of links because that's what search engines like to see.

With this merged, gtk-rs crates could configure their gir codegen to link every type to their actual docs.
Of course, this shouldn't interfere with the creation of the actual docs, so I hope that rustdoc-stripper or some other mechanism will be able to remove this.

Example

For gtk4, the config could look like this:

[options]
# ...
external_docs_url = "https://gtk-rs.org/gtk4-rs/stable/latest/docs/gtk4"

This would then generate a doc comment for all auto-generated types. This is how it would look for Box:

glib::wrapper! {
    #[doc(alias = "GtkBox")]
    /// This doc is limited due to license limitations. Please look at [our official docs](https://gtk-rs.org/gtk4-rs/stable/latest/docs/gtk4/index.html?search=Box) for more information.
    pub struct Box(Object<ffi::GtkBox, ffi::GtkBoxClass>) @extends Widget, @implements Accessible, Buildable, ConstraintTarget, Orientable;
    // ...
}

This is the generated link in this case: https://gtk-rs.org/gtk4-rs/stable/latest/docs/gtk4/index.html?search=Box

For now, the link is just a search query, but we could make it a direct link to the types as well. The only concern would be modules (when a type isn't in the crate's root, the generated doc link would need to include the module path as well) and API breakages leading to broken links.

} else if let Some(external_url) = external_url {
writeln!(
w,
"{}/// This doc is limited due to license limitations. Please look at [our official docs]({}/index.html?search={}) for more information.",
Copy link
Member

@GuillaumeGomez GuillaumeGomez Oct 15, 2022

Choose a reason for hiding this comment

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

It's not because of license limitations, it's because docs.rs prevents us from generating documentation because the files are read-only.

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 guess it depends on how you see it. The reason the docs aren't in the generated code in the first place are license issues AFAIK, but of course that could be resolved if docs.rs would allow us to modify files.

So I suggest to write this instead: "This documentation is incomplete due to restrictions on docs.rs. Please look ..."

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's a license issue. That docs.rs doesn't support our strange use case is a secondary problem caused by the former.

But in any case, linking to the proper docs would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we write: "This documentation is incomplete due to license restrictions and limitations on docs.rs. Please have a look at our official docs."

@sdroege
Copy link
Member

sdroege commented Oct 15, 2022

Further, SEO is a real problem with the docs

They're linked from the Cargo.toml and as such from crates.io but as that's all javascript generated it probably won't be picked up by search engines. It's also linked from libs.rs but that's much less widely used.

Signed-off-by: Aaron Erhardt <aaron.erhardt@t-online.de>
@AaronErhardt AaronErhardt requested review from sdroege and GuillaumeGomez and removed request for sdroege October 21, 2022 09:31
@AaronErhardt
Copy link
Contributor Author

It's also linked from libs.rs but that's much less widely used.

Actually, lib.rs first links to "API reference" first which is docs.rs and only the fourth link leads to the actual docs.

image

Btw. I have experimented a bit with generating direct links and it works pretty well for gtk4-rs, but I don't know whether that's a good idea because this approach assumes that all auto-generated types are in the same module (I don't know whether this is the case in general when using gir).

@AaronErhardt
Copy link
Contributor Author

Is there anything blocking this PR?

@sdroege
Copy link
Member

sdroege commented Nov 30, 2022

@GuillaumeGomez Is this all good for you?

@bilelmoussaoui
Copy link
Member

I am very much not in favour of this change btw, the problem should be solved on docs.rs itself and automatically use our docs link defined in Cargo.toml

@sdroege
Copy link
Member

sdroege commented Nov 30, 2022

For the record, I'm neutral about this. It's a workaround for docs.rs missing something, but if it helps avoiding some confusion and solving anything at docs.rs would be much harder...

@AaronErhardt
Copy link
Contributor Author

I also see it rather as a workaround. It's quite simple to implement and also quite simple to remove once docs.rs works properly. And in the meantime, it helps everyone to find the proper docs.

@sdroege
Copy link
Member

sdroege commented Nov 30, 2022

Did someone report something to docs.rs so it can be solved there properly? What's the plan / status there?

@bilelmoussaoui
Copy link
Member

Did someone report something to docs.rs so it can be solved there properly? What's the plan / status there?

Yes I did, rust-lang/docs.rs#1406

@sdroege
Copy link
Member

sdroege commented Nov 30, 2022

Looks mostly inactive. Maybe can ping there to ask what the way forward would be

@AaronErhardt
Copy link
Contributor Author

To me it looks like there's no real interest to implement this from the side of the docs.rs maintainers. After all, their security model seems to largely based on immutable files.

Maybe the macro approach suggested in the issue has some potential? Sounds pretty painful to implement though.

Also, the official docs.rs documentation suggests to write with a build script to the output directory, but I assume that doesn't allow us to create files in src/.

And even if that works we still have to somehow make things work without network access due to another limitation on docs.rs...

@AaronErhardt
Copy link
Contributor Author

I don't want to be annoying but can we decide whether to merge this or not? I'd be happy to help working on a proper solution for docs.rs, but that looks like it would take quite some time to complete.

In the meantime, we can use this solution to help people find the proper docs or we keep the status quo. I wouldn't mind if this PR was closed, but letting the code rot here doesn't make sense to me.

@AaronErhardt
Copy link
Contributor Author

There's seemingly no interest in merging this, so I'm going to close this PR if nobody comments in the next few days.

I'm aware that this is not a proper fix for the docs.rs issue, but it's still much better than nothing until a proper fix arrives.

@GuillaumeGomez
Copy link
Member

Sorry, took me a while to come back to this. Looking at it again, it seems like the right approach for me.

What do you think @sdroege @bilelmoussaoui ?

@sdroege
Copy link
Member

sdroege commented Mar 29, 2023

Ok for me

@GuillaumeGomez
Copy link
Member

Then let's merge. Thanks again @AaronErhardt and sorry for the delay.

@GuillaumeGomez GuillaumeGomez merged commit 2358cc2 into gtk-rs:master Mar 31, 2023
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.

4 participants