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.

Sign up for GitHub

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

Jump to bottom

Add inheriting classes to DocTools #77554

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Add inheriting classes to DocTools #77554

merged 1 commit into from
Jan 16, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented May 27, 2023 •
edited
Loading

Integrating a documentation hierarchy into DocTools

Part of the work for #77471 and #62524, see there for the discussions and background

Edit: I believe this is ready

@AThousandShips AThousandShips changed the title Add inheriting classes to DocData Add inheriting classes to DocTools May 27, 2023
@Calinou Calinou added this to the 4.x milestone May 27, 2023
@AThousandShips AThousandShips mentioned this pull request May 28, 2023
Merged
@YuriSizov
Copy link
Contributor

Still working on deciding how to deal with the ordering searches, as this changes the previous order which put Object at the top, if maintaining that order is important I can rework it

Order should be alphabetical, so global scopes go at the top and then everything else is in their case-independent alphanumerical order (natural, perhaps?). This is something that I've been working on, actually, when I stumbled upon your work :) So if you actually manage to address this issue here, that's an amazing improvement!

@YuriSizov YuriSizov self-requested a review May 29, 2023 11:22
@AThousandShips
Copy link
Member Author

AThousandShips commented May 29, 2023 •
edited
Loading

Will change the ordering to natural, I think there's a helper already for it! Sounds great

Bonus: unlike before new classes will end up in the right place in both search and the header of the help page, where before they were at the end

Note that at least currently this is not true for searching without hierarchy, as it uses the order in class_list, and can't easily resolve that here, #77213 would help with this

@AThousandShips
Copy link
Member Author

AThousandShips commented May 29, 2023 •
edited
Loading

We could use case sensitive natural sorting, but I think in this case this is sufficient, I'm not sure which order is preferred:

  • Light3D
  • LightmapProbe
  • LightOccluder3D

or:

  • Light3D
  • LightOccluder3D
  • LightmapProbe

@YuriSizov
Copy link
Contributor

I'd say the first one makes more sense when you search for something by the name. Either way, we can try and see if there are complaints.

@AThousandShips AThousandShips marked this pull request as ready for review May 29, 2023 12:20
@AThousandShips
Copy link
Member Author

I think this stage is ready for review properly now, and will continue to investigate the wider help indexing system as I am able

@AThousandShips
Copy link
Member Author

AThousandShips commented Jul 13, 2023 •
edited
Loading

Might solve #66407, will test

Seems to still be an intermittent problem, will try investigate

Edit: Does not solve it, seems to be caused by issues with the parser and circular references and don't think any implementation here can solve it, it appears to be that the inheriting data in the documentation is not correctly assigned if the parent class has cyclic errors like this, would require some more complicated updates to the data in the docgen for GDScript, so out of scope for this

@akien-mga
Copy link
Member

Would you have a screenshot of the result?

@AThousandShips
Copy link
Member Author

Will provide, for the correct order of sorted inherited classes?

@akien-mga
Copy link
Member

Well I'm not 100% sure what this PR changes but I'm assuming there's a before/after for the EditorHelp changes?

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 8, 2023 •
edited
Loading

This makes the hierarchy of inherited classes explicitly available in DocTools, which improves the sorting, and makes the accessing of the inheriting classes easier to access, for example when generating the doc view, it is also part of a larger documentation enhancement where the access to that is convenient

The direct benefit is avoiding scanning the entire class list in order to figure out inherited classes, and also a building block for handling the same

Will provide screenshot for the sorting

Edit: Forgot it also makes help search sorted alphabetically

Before:
image
image

After:
image
image

AThousandShips
AThousandShips commented Sep 8, 2023
View reviewed changes
editor/editor_help.cpp
Comment on lines +803 to 830
if ((cd.is_script_doc || ClassDB::class_exists(cd.name)) && doc->inheriting.has(cd.name)) {
_push_normal_font();
for (const KeyValue<String, DocData::ClassDoc> &E : doc->class_list) {
if (E.value.inherits == cd.name) {
if (!found) {
class_desc->push_color(theme_cache.title_color);
class_desc->add_text(TTR("Inherited by:") + " ");
found = true;
}
class_desc->push_color(theme_cache.title_color);
class_desc->add_text(TTR("Inherited by:") + " ");

if (prev) {
class_desc->add_text(" , ");
}
_add_type_icon(E.value.name, theme_cache.doc_font_size, "ArrowRight");
class_desc->add_text(non_breaking_space); // Otherwise icon borrows hyperlink from _add_type().
_add_type(E.value.name);
prev = true;
for (RBSet<String, NaturalNoCaseComparator>::Element *itr = doc->inheriting[cd.name].front(); itr; itr = itr->next()) {
if (itr->prev()) {
class_desc->add_text(" , ");
}

_add_type_icon(itr->get(), theme_cache.doc_font_size, "ArrowRight");
class_desc->add_text(non_breaking_space); // Otherwise icon borrows hyperlink from _add_type().
_add_type(itr->get());
}
_pop_normal_font();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the relevant change for that, which also avoids scanning the entire class list for all classes just to find inheriting ones

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Sep 27, 2023
@AThousandShips AThousandShips force-pushed the doc_hierarchy branch 2 times, most recently from 80a7b38 to ae34661 Compare December 7, 2023 12:19
@AThousandShips AThousandShips mentioned this pull request Dec 7, 2023
Merged
YuriSizov
YuriSizov reviewed Jan 16, 2024
View reviewed changes
editor/doc_tools.cpp Outdated Show resolved Hide resolved
YuriSizov
YuriSizov approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

This looks great to me, love the new ordered list very much!

Some minor notes remain, and I'm also not great with data structures, so maybe @KoBeWi could take a quick look. But overall it all seems sensible.

PS. We should improve at some point the relevance of what is selected as the closest match, I think. Typing "path" with "Display All", for example, selects the first complete match which is a property of a navigation server struct. Very unlikely to be relevant.

godot windows editor dev x86_64_2024-01-16_14-29-02

editor/editor_help.cpp Outdated Show resolved Hide resolved
editor/editor_help_search.cpp Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member Author

Will see about the relevance part in the final part, unsure how to improve it but good point!

@akien-mga akien-mga merged commit 107f296 into godotengine:master Jan 16, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the doc_hierarchy branch January 16, 2024 15:48
@AThousandShips
Copy link
Member Author

Thank you! Great to have this one done with

@YuriSizov YuriSizov mentioned this pull request Jan 22, 2024
Closed
This was referenced Feb 6, 2024
Closed
Closed
@AThousandShips AThousandShips mentioned this pull request Nov 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuriSizov YuriSizov YuriSizov approved these changes

Assignees
No one assigned
Labels
enhancement topic:editor
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Documentation: @GDScript and @GlobalScope are at the bottom of the list
4 participants
@AThousandShips @YuriSizov @akien-mga @Calinou