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

Split CB documentation to smaller pages #6893

Merged
merged 5 commits into from Jun 2, 2023

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented May 30, 2023

Pull Request Description

Now documentation for types, constructors and methods is displayed separately, with a links between pages.
It drastically improves the speed of documentation panel update (50-100x on my machine), and also provides more readable documentation.

2023-05-31.00-55-50.mp4

Before:

Screenshot 2023-05-31 at 01 02 47

After:

Screenshot 2023-05-31 at 00 54 53

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@vitvakatu vitvakatu force-pushed the wip/vitvakatu/documentation-opt branch from af17a3b to af06aa3 Compare May 30, 2023 21:09
@vitvakatu vitvakatu force-pushed the wip/vitvakatu/documentation-opt branch from af06aa3 to aa57ef3 Compare May 30, 2023 21:22
@vitvakatu vitvakatu self-assigned this May 30, 2023
@vitvakatu vitvakatu marked this pull request as ready for review May 30, 2023 21:26
app/gui/suggestion-database/src/documentation_ir.rs Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a while to realize the cleverness of this code.

I definitely needed some high-level explanation (and a warning) that creating documentation for an entry will also generate documentation for all entities in its module, allowing jumping over the module through links. The current documentation_ir module docs are outdated.

(I hope it was tested for huge modules, like Base.Data)

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 extended the documentation. We will generate module-level docs for every Method, Type or Constructor, not every type of entry (but pretty much for most of them). That's an unfortunate drawback, but it's hard to design the whole thing with links otherwise. One of the solutions is a documentation registry, but there are so much questions with cache invalidations there, that I don't even want to touch it.

As for testing, the new implementation actually does less work in some cases than the previous one, and the generation of IR was never an issue even on slow machines.

app/gui/view/documentation/src/html.rs Outdated Show resolved Hide resolved
app/gui/view/documentation/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Just a fast review, as Adam did deep one :)

Comment on lines +123 to +125
page: Documentation::ModuleMethod {
docs: method.clone_ref(),
module_docs: docs.clone_ref(),
Copy link
Member

Choose a reason for hiding this comment

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

We could make it a lot shorter in many places by doing Documentation::ModuleMethod(&method, &module_docs) and define its parameters in the new fn as something: Into<...>, This way it will work both for references and passing owned values, making the usage way nicer.

@farmaazon farmaazon linked an issue Jun 1, 2023 that may be closed by this pull request
@MichaelMauderer
Copy link
Contributor

QA: 🍏 Looks good to me.

@farmaazon farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Jun 2, 2023
@mergify mergify bot merged commit bd3ba26 into develop Jun 2, 2023
28 checks passed
@mergify mergify bot deleted the wip/vitvakatu/documentation-opt branch June 2, 2023 07:14
@radeusgd
Copy link
Member

radeusgd commented Jun 2, 2023

This is so awesome!

I'm just slightly surprised why we use the :: for delimiting the parent type and the method when displaying in CB: like Text::trim. We don't have such syntax anywhere in the language. Shouldn't we just use a . like Text.trim?

@vitvakatu
Copy link
Contributor Author

I'm just slightly surprised why we use the :: for delimiting the parent type and the method when displaying in CB: like Text::trim. We don't have such syntax anywhere in the language. Shouldn't we just use a . like Text.trim?

Yep, this is in the list of things to be fixed in the next PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split documentation into separate sub-pages
5 participants