Skip to content

Introduce ResolvableSource - #18

Open
AntsyLich wants to merge 2 commits into
mainfrom
resolvable
Open

Introduce ResolvableSource#18
AntsyLich wants to merge 2 commits into
mainfrom
resolvable

Conversation

@AntsyLich

Copy link
Copy Markdown
Member

No description provided.

Comment thread library/src/main/java/eu/kanade/tachiyomi/source/ResolvableSource.kt Outdated
@AntsyLich
AntsyLich marked this pull request as ready for review August 30, 2025 09:07
* @param url The [HttpUrl] to test.
* @return `true` if this source can resolve the URL, `false` otherwise.
*/
fun canResolve(url: HttpUrl): Boolean

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just make getResource return Resource? and remove this method.

Source might need to make network requests to know if they can resolve the thing. Most cases when they instantly return false, the suspend won't cause overhead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is mainly for early filtering on app side. should just be a regular regex check on source side

@stevenyomi stevenyomi Sep 9, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. getResource also needs the regex/URL path check to extract path segments. Your API forces extensions to repeat this logic.
  2. The app just calls the first then the second. What's your use case that only calls the first one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Purely for UX reason. When resolving the URL of Source A and B I would rather not show the user Source C with "No result found"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Returning null is equivalent to "can't resolve". No results found should be conveyed by throwing an exception.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What you're proposing is that we show user Source A, B and C. When Source C returns null we remove it from the list. Later Source A and B resolves to Manga A. I don't think that's a good UX. There should be a way for the app to filter out sources that 100% will never resolve the URL

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Running regex/path URL checks for a lot of extensions is also a time-consuming I/O task. You still need a loading screen before showing a candidate list. I think it's possible to achieve the same effect by checking if the coroutine returns early (no suspension done).

The two-method solution will require extension developers to write duplicate/boilerplate code and should be avoided if possible.

You also need to document what developers should do in the KDoc, like checking for the domain, etc, and when to throw what exception. How do they convey "this URL is for this site but doesn't resolve to a manga/chapter"?

* @property manga The resolved manga metadata.
* @property chapter An optional specific chapter of the manga.
*/
data class Manga(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't use data class. The methods added are useless, and SManga/SChapter are not hashable either.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why did we add it for MangasPage then...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's an app-side implementation. Extensions should not rely on this. That's why we need to unmark MangasPage in 1.6.

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.

2 participants