-
Notifications
You must be signed in to change notification settings - Fork 415
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
base: master
Are you sure you want to change the base?
Fix incorrect links for inherited java methods from a collection #3529
Conversation
bf05a42
to
e848994
Compare
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)) |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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
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
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
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. Also, I have discovered #3542 It should be discussed as well before merging this PR. |
d7d0b98
to
d937a1e
Compare
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:Other possible solutions can require a non-trivial logic.