Fix @export_category interfering with property tooltips

[emr4378]

Fixes #64432
Don't assume the category name can be used as the doc_name. Just leave it blank so it can be properly determined later on just before looking up cached doc info.

Fixes (not) #63997
update_tree() populates description-related caches, but nothing was ever clearing them out so they'd forever hold onto the first description seen.

[YuriSizov]

cc @anvilfolk

[anvilfolk]

Oh, very interesting! I can't take a deeper look right now, but the idea behind the first fix sounds really cool!!!

I'm not super sure on the second one. When is EditorInspector::_clear() called? If it's very often, I believe it might basically invalidate the use of the cache, right? There's another PR that might address this, I believe: #71843, and I think that #70217 is related to #63997 as well!

[emr4378]

AFAICT EditorInspector::_clear() is only called in EditorInspector::edit() (so whenever the node selection changes) and at the start of EditorInspector::update_tree(). I put the doc cache clearing there since that's where other caches were being reset.

But I think you're right and it's invalidating the benefit of these particular caches. I'll dig around to find better spot(s) to clear them out.

EDIT: Landed on clearing them when the node to inspect changes (edit()) and/or when the script itself changes (_edit_request_change()).

[anvilfolk]

Gave this a tiny little bit more thought - sorry it's taking a while, life has been hectic lately :)

So I suspect that #71843 is the better solution to #63997, of which I think #70217 is partially a duplicate. Instead of clearing the cache completely, it simply stops caching doc information that comes from GDScripts. This means that the cache works fully with the static documentation like @Globals and native classes. It's never cleared at any point, so it's always available and doesn't need to be re-cached.

GDScript stuff is then always obtained from EditorHelp/DocTools directly when it's needed, so it's never outdated.

I haven't gotten around to testing the other fix in this PR, since that part of the code can be pretty subtle! But I love the simplicity of it 🥳 How much testing have you managed to do on it? Would you be able to check that e.g. resource docstrings still work? And can we be 100% certain that doc_name is always being set before being used? :)

I'll try to get some more testing done myself, time is just hard to come by atm! :)

[anvilfolk]

So I really want to test this but it's too much of a bother right now with having to restart the entire project every time you change docstrings so they update properly. #71843 fixes this but hasn't been accepted yet - I will come back to this once that's merged :)

[akien-mga]

#71843 fixes this but hasn't been accepted yet - I will come back to this once that's merged :)

It was merged ;)

[anvilfolk]

I see what you did there ;)

I will try to find some time to re-review this one sometime this week hopefully if I manage to catch up on class reading materials :)

[Calinou]

The caching issue should be resolved in master, see #63997 (comment).

[anvilfolk]

Correct! At this point the only outstanding issue should be the category issue, which I will attempt to test :) It's a single-line fix :)

[KoBeWi]

The cache changes should be removed

I looked into what doc_name line tries to do. Seems like it's supposed to use category name as type to fetch the property description. It obviously breaks with custom categories.
It was added by #64246, but removing this line does not reintroduce the original issue, so this change is ok.

[anvilfolk]

As long as the cache lines are removed, as @KoBeWi said, this fixes the issue.

The PR description is correct: there's no reason that a custom category should attempt to set doc_name, which is used to index where documentation can be found. That should be set once, when the script's own category is evaluated.

I couldn't find any regressions when using zero, one or multiple categories, or with the remaining categories, e.g. Node.

@emr4378, can you make these changes? :)

[YuriSizov]

Looks like there is unaddressed feedback in the discussion above. @emr4378 Are you still interested in contributing? This needs a rebase too.

[anvilfolk]

I'd say if someone with privileges for it wanted to amend this commit leaving only the removal of the doc_name = p.name; line, this would be good to merge. Sounds like the OP might be inactive at the moment, but could still be credited for fixing that bug :)

[akien-mga]

I updated this PR myself as advised by @anvilfolk, as OP seems to be inactive.

Should be ready to merge now.

[YuriSizov]

Should be ready to merge now.

I wanted to wait for #82051 to be merged to make sure this still behaves as expected afterwards. It's probably fine, but the code related to all these class and doc names is not straightforward so I wanted to be sure.

[akien-mga]

So I wanted to confirm the bug again to test the fix, and the bug seems already fixed since 4.2-dev5. Weird.

[akien-mga]

@dalexeev Is this one line change still worth doing for good measure, or would risk introducing other issues?

[dalexeev]

I haven't tested this yet, but I suspect the change may break tooltips for native classes. I'll reply later.

[dalexeev]

I tested it. The code doesn't look correct to me, doc_name changes only for scripts, not for native classes. This breaks class descriptions and context menu "Open Documentation".

Before:

After:

[dalexeev]

• The linked issue has already been fixed by Editor: Inspector and Signal docks improvements #81221.

Thanks for the contribution nonetheless!

[anvilfolk]

Oh, damn, great catch @dalexeev, I had focused on testing GDScript classes! Glad you came along ❤️