Skip to content

Conversation

patrickunterwegs
Copy link
Member

No description provided.

@rfc2822
Copy link
Member

rfc2822 commented Sep 15, 2025

It only affects the ktor package, so it should be fine :)

One thing I wonder: would it be possible to not duplicate the properties? Because they should be data classes and depend only on XML parsing.

@rfc2822
Copy link
Member

rfc2822 commented Sep 17, 2025

One thing I wonder: would it be possible to not duplicate the properties?

As far as I have seen, the files in the property package don't have okhttp dependencies. So I suggest to

  • move the properties to at.bitfire.dav4jvm.common.property (without duplicating them),
  • add the Ktor part (= classes with okhttp dependency) to at.bitfire.dav4jvm.ktor.* as planned and as this PR does.

What do you think?

@patrickunterwegs
Copy link
Member Author

Hi, basically it could be done, shouldn't be much effort anyway.
Why I would still hesitate doing that is the next step for Kotlin Multiplatform.

From my point of view it would make sense to adapt the Ktor version in a way that also the parser is Multiplatform compatible. If the properties are shared, another solution must be found about how to proceed when the XMLPullParser is replaced by another solution.
So I would rather prefer to build upon the ktor package and replace the parsing there with a Multiplatform compatible one. I wouldn't mind the shred properties for now, but then another approach must be found on how to deal with changes for Multiplatform compatibility.

@rfc2822
Copy link
Member

rfc2822 commented Sep 18, 2025

Why I would still hesitate doing that is the next step for Kotlin Multiplatform.

I understand, but I wouldn't like to have two different XML parsers (one for okhttp and one for ktor) and different properties in dav4jvm, if avoidable.

So my preferred way would be:

  1. Add the Ktor version of dav4jvm (but keep common properties).
  2. Make the Ktor version stable and integrate into DAVx5 (ideally not everything at once).
  3. Drop the okhttp version and make Ktor the only one (we can still use okhttp as backend for DAVx5).
  4. Continue with further refactoring for Multiplatform. This includes a solution for XML. In the worst case we have to define our own interfaces for XML parsing and then make an arbitrary parser provider for whatever is needed in the respective project.

Would that be a feasible way for you? Or would it be too slow? But you could change the XML parser and properties in a separate branch anyway.

@patrickunterwegs
Copy link
Member Author

Why I would still hesitate doing that is the next step for Kotlin Multiplatform.

I understand, but I wouldn't like to have two different XML parsers (one for okhttp and one for ktor) and different properties in dav4jvm, if avoidable.

So my preferred way would be:

  1. Add the Ktor version of dav4jvm (but keep common properties).
  2. Make the Ktor version stable and integrate into DAVx5 (ideally not everything at once).
  3. Drop the okhttp version and make Ktor the only one (we can still use okhttp as backend for DAVx5).
  4. Continue with further refactoring for Multiplatform. This includes a solution for XML. In the worst case we have to define our own interfaces for XML parsing and then make an arbitrary parser provider for whatever is needed in the respective project.

Would that be a feasible way for you? Or would it be too slow? But you could change the XML parser and properties in a separate branch anyway.

ad 1)
I have adapted the Pull Request in that way and lifted the properties the level up so that they are shared. However, I'm not sure how much if it can just be reused when the parser is changed for a Multiplatform compatible one. If you prefer it for now like this, it's okay. Adapting the parser will take some time. I prefer to go forward faster to work on an Android-version of the library and then adapt for a Multiplatform one.

However, there are small changes that are not super clean, but in my eyes acceptable:

  • There is shared code where the ContentType (ktor) / MediaType (okhttp) is used. I have tried to generalize it. Where it wasn't possible, I used to ContentType from ktor also in the shared package. This is still convenient and wouldn't break anything.
  • I have tried to generalize the exception package. But there are a couple of constructors relying on okhttp or ktor for the response. I thought of just adding the respective other constructor to a generalized version of the respective Exception class, but then more classes would be mixed with okhttp and ktor dependencies which would make it just harder once the Multiplatform version is created. So I refrained from that idea.
  • There are two classes in the property where I couldn't find a better way than to split the function fromResponse(...) to fromKtorResponse(...) and fromOkHttpResponse(...) where the Property classes now have a dependency on both libraries. I'd rather recommend to deal with that at a later stage for Multiplatform.

ad 2-4)
sounds good

The current PR would be ready for review.

rfc2822
rfc2822 previously approved these changes Sep 24, 2025
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

As long as we don't need the ktor package in davx5, I think we can explicitly exclude the dependency when we import dav4jvm.

@rfc2822 rfc2822 requested a review from sunkup September 24, 2025 12:20
@rfc2822
Copy link
Member

rfc2822 commented Sep 25, 2025

  • There is shared code where the ContentType (ktor) / MediaType (okhttp) is used. I have tried to generalize it. Where it wasn't possible, I used to ContentType from ktor also in the shared package. This is still convenient and wouldn't break anything.

    • I have tried to generalize the exception package. But there are a couple of constructors relying on okhttp or ktor for the response. I thought of just adding the respective other constructor to a generalized version of the respective Exception class, but then more classes would be mixed with okhttp and ktor dependencies which would make it just harder once the Multiplatform version is created. So I refrained from that idea.

    • There are two classes in the property where I couldn't find a better way than to split the function fromResponse(...) to fromKtorResponse(...) and fromOkHttpResponse(...) where the Property classes now have a dependency on both libraries. I'd rather recommend to deal with that at a later stage for Multiplatform.

I was too fast. Will have a look at these

@rfc2822 rfc2822 dismissed their stale review September 25, 2025 14:19

Was too fast sorry

Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Do we really have to introduce breaking changes in this PR? Would it not be better to leave that for another PR too?

imported Error file properly, adapted companion objects for eTag and ScheduleTag
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Looks good with the imports now. What do you think about retaining the method signature @rfc2822 ?

@rfc2822 rfc2822 requested a review from Copilot October 2, 2025 13:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reorganizes the dav4jvm library by moving shared code out of the OkHttp-specific package into a common base package, creating a dedicated Ktor implementation package with equivalent functionality. The changes establish a clear separation between HTTP client implementations while maintaining backward compatibility for the OkHttp version.

  • Extracts common classes (Property, XmlUtils, HttpUtils, etc.) from okhttp package to base dav4jvm package
  • Creates new ktor package with Ktor-based implementations of DAV client functionality
  • Updates all import statements across test and source files to reference the new package structure

Reviewed Changes

Copilot reviewed 133 out of 134 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
Property classes Moved from okhttp-specific to common base package for reuse across implementations
Test files Updated imports and added Ktor-specific test implementations
OkHttp classes Updated imports to reference moved common classes, retained OkHttp-specific functionality
Ktor classes New implementation using Ktor HTTP client with equivalent WebDAV functionality
Exception classes Created Ktor-specific exception hierarchy mirroring OkHttp structure

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Looks good, only two minor things are left.

@patrickunterwegs
Copy link
Member Author

Looks good, only two minor things are left.

@rfc2822 sounds good, changes are done :-)

Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

@rfc2822
Copy link
Member

rfc2822 commented Oct 2, 2025

As @sunkup already wrote "looks good" before, I'll now merge

@rfc2822 rfc2822 merged commit e79946c into bitfireAT:main Oct 2, 2025
1 check passed
@github-project-automation github-project-automation bot moved this from Todo to Done in DAVx⁵ Releases Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants