Skip to content

Conversation

@sunkup
Copy link
Member

@sunkup sunkup commented Jun 11, 2025

Merge BatchOperation from ical4android and vcard4android into one generic BatchOperation in the synctools package.

  • Create new generic BatchOperation with equal functionality (the only difference is in how the number of content provider operations per yield point are handled and now we always take maxOperationsPerYieldPoint as constructor param)
  • Replace usages in vcard4android and remember to now pass CONTACTS_OPERATIONS_PER_YIELD_POINT in the constructor (2x in AndroidContact)
  • Replace usages in ical4android

Note: I am unsure whether we might want start passing along BatchOperation.TASKS_OPERATIONS_PER_YIELD_POINT in JtxICalObject.insertOrUpdateListProperties() when constructing BatchOperation():

val deleteBatch = BatchOperation(collection.client)

Note2: We are using ical4android.BatchOperation and vcard4android.BatchOperation in many places in DAVx5, but the replacement should go quite smooth.

@sunkup sunkup linked an issue Jun 11, 2025 that may be closed by this pull request
@sunkup sunkup self-assigned this Jun 11, 2025
@sunkup sunkup added the refactoring Quality improvement of existing functions label Jun 11, 2025
@sunkup sunkup force-pushed the 11-unify-batchoperation branch 2 times, most recently from 4d95926 to 16e3d85 Compare June 11, 2025 10:51
@sunkup sunkup force-pushed the 11-unify-batchoperation branch from 85b41ef to 9547d19 Compare June 11, 2025 11:22
@sunkup sunkup force-pushed the 11-unify-batchoperation branch from 9547d19 to a6a043c Compare June 11, 2025 11:42
@sunkup sunkup requested a review from Copilot June 11, 2025 11:49

This comment was marked as outdated.

@sunkup sunkup marked this pull request as ready for review June 11, 2025 11:51
@sunkup sunkup requested a review from rfc2822 June 11, 2025 11:52
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.

Nice!

There's a BatchOperationTest in ical4android. I think it makes sense to move it to the new package, too.

I don't know whether it makes sense to already generalize it for multiple content providers or how to do so. Currently there seems to be only a BatchOperationTest for ical4android.

However it would be good if there would be tests for contacts (and ideally tasks), too, so that we can test for instance the max operations per yield point with all providers.

@sunkup sunkup requested review from rfc2822 and removed request for rfc2822 June 16, 2025 11:32
@sunkup sunkup requested a review from rfc2822 June 16, 2025 11:43
@rfc2822 rfc2822 requested a review from Copilot June 17, 2025 17:44
Copy link
Contributor

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 refactors and unifies the BatchOperation functionality by consolidating similar implementations from ical4android and vcard4android into a generic BatchOperation in the synctools package. It also updates all corresponding usages and tests to rely on the new provider-specific subclasses such as ContactsBatchOperation, CalendarBatchOperation, TasksBatchOperation, and JtxBatchOperation.

Reviewed Changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lib/src/main/kotlin/at/bitfire/synctools/storage/ContactsBatchOperation.kt Added a provider-specific BatchOperation for contacts with a fixed operations limit.
lib/src/main/kotlin/at/bitfire/synctools/storage/CalendarBatchOperation.kt Added a BatchOperation for the calendar provider with no yield point limitation.
lib/src/main/kotlin/at/bitfire/synctools/storage/BatchOperation.kt Refactored the base BatchOperation with new constructor parameters, logging updates, and internal visibility for testing.
lib/src/main/kotlin/at/bitfire/ical4android/JtxICalObject.kt Replaced the usage of the old BatchOperation with JtxBatchOperation.
Remaining files (DmfsTaskList.kt, DmfsTask.kt, AndroidEvent.kt, etc.) Updated imports and instantiation to use the new unified BatchOperation classes.
Test files Adjusted tests to reflect the new BatchOperation usage and validated transaction splitting behavior.
Comments suppressed due to low confidence (1)

lib/src/main/kotlin/at/bitfire/synctools/storage/BatchOperation.kt:29

  • [nitpick] It might be helpful to clarify in the documentation that setting maxOperationsPerYieldPoint to null disables yield points entirely. This can prevent any future confusion regarding its intended behavior.
 * @param maxOperationsPerYieldPoint    maximum number of operations per yield point (`null` for none)

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.

I have

  • moved BatchOperation to the "storage" package (because synctools will also get a mappings package),
  • changed enqueue to +=,
  • introduced BatchOperation subclasses that define the max operations per yield point for every provider,
  • changed the converter classes so that they require the specific BatchOperation so wrong max yield points can't be used accidentally,
  • created separate BatchOperation test classes for every provider (and a generic test class that tests splitting)

@rfc2822 rfc2822 merged commit 2a45226 into main Jun 18, 2025
9 checks passed
@rfc2822 rfc2822 deleted the 11-unify-batchoperation branch June 18, 2025 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Quality improvement of existing functions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify BatchOperation

2 participants