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

Fix incorrect links for inherited java methods from a collection #3529

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vmishenev
Copy link
Member

@vmishenev vmishenev commented Mar 12, 2024

The bug #2879 stems from inherited external members. The unit test below demonstrates it.
The proposed solution is to get rid of the resolving DRI to anchors only in DokkaLocationProvider. There are the following reasons:

  1. After removing typealiases, this logic is not needed anymore
  2. External inherited members lead to itself #3054 would be fixed as well

Other possible solutions can require a non-trivial logic.

@vmishenev vmishenev force-pushed the vmishenev/2879-Incorrect_link_for_inherited_java_method_from_collection branch from bf05a42 to e848994 Compare March 12, 2024 21:48
val dri = org.jetbrains.dokka.links.DRI("kotlin.collections", "Collection", Callable(name="isEmpty", receiver=null, params=emptyList()))
assertEquals("../-b/index.html#-719293276%2FFunctions%2F-1617659094", location.resolve(dri, sourceSet, classA))
assertEquals("index.html#-719293276%2FFunctions%2F-1617659094", location.resolve(dri, sourceSet, classB))
assertEquals("../-b/index.html#-719293276%2FFunctions%2F-1617659094", location.resolve(dri, sourceSet, classC))
Copy link
Member Author

@vmishenev vmishenev Mar 12, 2024

Choose a reason for hiding this comment

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

This is a minimal reproducer.
If we remove an anchor logic in DokkaLocationProvider, #3054 will be fixed as well. We have not discussed it at grooming, but it can be a big change for a user.

@IgnatBeresnev @whyoleg WDYT?

            open interface C : List<C> {
            /**
            * [Collection.isEmpty]
            **/
            val p = 0
            }
            interface A : C()
            interface B : C()
            

Copy link
Member

Choose a reason for hiding this comment

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

p in the example is a bit confusing, it looks like it's somehow related to the bug, but it seems like it isn't?

Given the example

interface C : List<C>

interface A : C
interface B : C

I can open the page for B and see inherited functions there, like isEmpty. If I click on the anchor link icon, it gives me a link to B#isEmpty, the same place where I generated it:

http://localhost:63342/dokka-debug-kts/build/dokka/html/dokka-debug-kts/me.beresnev.dokka.debug/-b/index.html#-1000881820%2FFunctions%2F769193423
image

What's going to happen if we implement the change? Will the anchor link lead to List#isEmpty on kotlinlang.org? If yes, what's the expected behaviour for overridden functions?

interface C : List<C>

interface A : C
interface B : C {

    /**
     * Overriding documentation
     */
    override fun isEmpty(): Boolean = true
}

Copy link
Member Author

Choose a reason for hiding this comment

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

p in the example is a bit confusing, it looks like it's somehow related to the bug, but it seems like it isn't?

The reproducer is in the unit test.

interface C : List<C>
interface A : C
interface B : C

The p is not related to the bug at all, but the link [Collection.isEmpty] is.

I can open the page for B and see inherited functions there, like isEmpty. If I click on the anchor link icon, it gives me a link to B#isEmpty, the same place where I generated it:

Sorry, I got it where you are confused. I mean only the resolving anchors by DRI logic (see DokkaLocationProvider .resolveDrI), but not generating anchor links for an icon (see DokkaBaseLocationProvider.anchorForDCI).
If you open the page for A or C and click on the isEmpty keyword, it opens -b/index.html#-1000881820%2FFunctions%2F769193423.

Will the anchor link lead to List#isEmpty on kotlinlang.org?

No, the generating anchor link icon will left the same.

If yes, what's the expected behaviour for overridden functions?

In this case, Dokka generates a dedicated page for the overridden functions.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think I understand now! After the fix, all anchor icon links will work the same way as before, but if C inherits isEmpty, clicking on C#isEmpty will lead to kotlinlang instead of leading to itself or to A/B, right? If so, it looks logical to me 👍

@vmishenev
Copy link
Member Author

After a discussion at our meeting, opening external documentation for such members can be a UI issue, e.g. there is no way to come back to the original documentation.
I will open an issue later.

Also, I have discovered #3542 It should be discussed as well before merging this PR.

@vmishenev vmishenev force-pushed the vmishenev/2879-Incorrect_link_for_inherited_java_method_from_collection branch from d7d0b98 to d937a1e Compare March 20, 2024 17:52
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.

External inherited members lead to itself Incorrect link for inherited java method from collection
2 participants