Skip to content

Create docs button for FileSystem resources #33143

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiagoamendola
Copy link
Contributor

@thiagoamendola thiagoamendola commented Oct 28, 2019

Like the node's context menu from the Scene tab, the context menu for
resources in the FileSystem tab should contain an "Open Documentation" button.

The "Open Documentation" button for resources can open the documentation for
given resource's type. It works with multiple file selections and avoids
opening an empty documentation tab for folders.

@aaronfranke
Copy link
Member

@thiagoamendola Is this still desired? If so, it needs to be rebased on the latest master branch. While there are no conflicts, rebasing is important so that reviewers can easily test the PR.

@thiagoamendola
Copy link
Contributor Author

Thanks for the ping. Just rebased the branch.

@groud
Copy link
Member

groud commented Jul 27, 2020

I think the feature might be desired, but I see two problems with the implementation:

  • Whatever the file we have, the option is displayed anyway. I am pretty sure some files aren't going to have an associated documentation page. So it would be better if the option is only displayed when a documentations associated to the file type exists.
  • What happen to files that can be imported as different types (like, where you can choose the type in the import panel) ?

@thiagoamendola
Copy link
Contributor Author

@groud about those 2 issues:

  • Does it make sense to quickly load the documentation page and check if it's empty? I'm worried about adding an overhead time in the popup opening step. Another options could be to either cache available extension pages or show a different default page in case there's none available.
  • I think it'd be okay to process a file with its current chosen type. Do you see any cases where this can be a problem?

@groud
Copy link
Member

groud commented Oct 24, 2020

I had a look to the code. Apparently there is no checks done for the documentation page to exist anywhere else in the code. So I think keeping it like that is ok. Custom types without a documentation will likely trigger an error and do nothing, but I am not sure.

I think it'd be okay to process a file with its current chosen type

I think it should be enough, but I think we should mention the class name in the text line then. Something like "Open documentation for <class name>".

@thiagoamendola
Copy link
Contributor Author

By testing the button on a file type with no available documentation, the following empty page is displayed:

Screenshot from 2020-10-25 09-38-10

It doesn't break but it doesn't tell much either. Do you think it's ok showing it?

Also, about the "Open documentation for " suggestion, won't the option string become too big, making the popup cluttered?

Screenshot from 2020-10-25 09-57-58

@groud
Copy link
Member

groud commented Oct 25, 2020

I am not sure, I think it would be better to have something implemented to avoid this, as we're going to have issue report about this. I guess we should summon @akien-mga here to give us his opinion.

@akien-mga
Copy link
Member

By testing the button on a file type with no available documentation, the following empty page is displayed:

Yeah that would need to be prevented. The code should only query opening the docs if it's a file type that matches a builtin Node.

Also, about the "Open documentation for " suggestion, won't the option string become too big, making the popup cluttered?

Yeah that's a bit too verbose I think. Some translations might make it even worse.
Could be "Open Class Documentation" only without putting the specific class name. And the code you have to figure out the class name can likely be used to solve the first point above, so that the menu entry is not shown when there's no class name. Beware of user-defined classes which may not have documentation either though (but I think ClassDB or DocData might have relevant methods to let you figure out what's builtin/documented and what isn't).

Like the node's context menu from the Scene tab, the context menu from
resources in the FileSystem tab should contain an "Open Documentation" button.

The "Open Documentation" button for resources can open the documentation for
given resource's type. It works with multiple file selections and avoid
opening an empty documentation tab for folders or non built-in types.
@YuriSizov YuriSizov requested a review from a team August 24, 2021 20:55
@akien-mga akien-mga modified the milestones: 4.0, 4.x Jul 26, 2022
@akien-mga
Copy link
Member

Would be good to get this re-reviewed / tested, @godotengine/docks.

I think it's fine to add this kind of option but we should make sure it also works e.g. for going to the generated documentation for GDScript classes.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 11, 2022

I tested with scripts and it opens GDScript resource documentation instead of the custom class documentation. Seems to work fine in other cases.

So this needs a rebase and fixing the GDScript issue.

@MewPurPur
Copy link
Contributor

Are you still around to rebase?

@aaronfranke aaronfranke marked this pull request as draft August 29, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants