diff --git a/.claude/skills/android-java-to-kotlin/SKILL.md b/.claude/skills/android-java-to-kotlin/SKILL.md new file mode 100644 index 000000000000..1cec9a70feba --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/SKILL.md @@ -0,0 +1,130 @@ +--- +name: android-java-to-kotlin +description: > + Use when finishing a Java-to-Kotlin conversion in an Android project, when the user + mentions "java to kotlin", "j2k", "convert java", "migrate java to kotlin", "finish the + conversion", "make it idiomatic", or when a freshly IDE-converted .kt file needs to be + turned into clean, modern, idiomatic Kotlin. The developer first runs the IDE converter + (Android Studio: Code > Convert Java File to Kotlin File), then this skill drives the + second pass: idiomatic cleanup, fail-fast control flow, coroutines/lifecycleScope, + modern Android APIs, function decomposition, and a behaviour-locking test. +license: AGPL-3.0-or-later +SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +SPDX-License-Identifier: AGPL-3.0-or-later +metadata: + author: Nextcloud Android + version: "1.0.0" +--- + +# Android Java to Kotlin Conversion (Second Pass) + +The IDE converter produces Kotlin that *compiles* but is not *idiomatic*: platform types +everywhere, one giant function per lifecycle callback, nested `if`/`else`, `new Thread`, +`switch`, `TextUtils.isEmpty`, magic numbers. This skill drives the disciplined second +pass that turns that output into clean, modern, testable Kotlin — **without changing +observable behaviour** — and then writes a test that proves behaviour is unchanged. + +## The Two-Person Workflow + +This skill assumes a hand-off: + +1. **Developer** runs the mechanical IDE conversion (`Code > Convert Java File to Kotlin + File`, or ⌥⇧⌘K), producing a `.kt` file that compiles but is not idiomatic. +2. **You (Claude)** drive everything after that: + + Step 0 Baseline → Step 1 Idiomatic pass → Step 2 Behaviour-locking test → Step 3 + Verify. If Step 3 fails or behaviour drifts, loop back to Step 1. + +If you are handed a `.java` file instead, first apply a faithful 1:1 translation to reach +the same starting point, then continue. + +## The Prime Directive: Behaviour Must Not Change + +Every transformation in this skill is **behaviour-preserving**. You are refactoring for +readability, safety, and modern API usage — not adding features or fixing bugs. If you +spot a real bug, surface it to the developer; do not silently "fix" it inside a +conversion. The behaviour-locking test in Step 3 exists to keep you honest. + +## Step 0: Establish Baseline + +Before editing anything: + +1. Read the whole `.kt` file (and, if you can, the original `.java` via + `git show :.java` or the IDE's local history) to understand *what it does*. +2. Write down the **public/observable surface** you must keep intact: + - Lifecycle callbacks (`onCreate`, `onViewCreated`, `onSaveInstanceState`, …) and their + ordering of side-effects. + - Any Parcelable/Bundle keys, intent extras, and `newInstance(...)` factory shapes. +3. Note threading: which work runs off the main thread today (`Thread`, `AsyncTask`, + `runOnUiThread`, executors) — this maps to coroutines in Step 2. + +## Step 1: Idiomatic Pass + +Apply, in this order, then re-check the invariants: + +1. **Kill platform types.** Give every `!` platform type an explicit nullable/non-null + type based on the Java source and call sites. +2. **Fail fast.** Replace nested `if`/`else` pyramids and null-checks with guard clauses + and `require`/`requireNotNull`/`?: return`. See [FAIL-FAST.md](references/FAIL-FAST.md). +3. **Decompose.** Break each oversized lifecycle callback / `setupView`-style method into + small, single-purpose private functions named for their intent. See + [ANDROID-IDIOMS.md](references/ANDROID-IDIOMS.md). +4. **Modern concurrency.** Replace `new Thread`, `AsyncTask`, `runOnUiThread`, and + callback pairs with `lifecycleScope` + `suspend` + `withContext`. See + [CONCURRENCY.md](references/CONCURRENCY.md). +5. **Modern Android + Kotlin idioms.** Scope functions (`run`/`apply`/`let`), extension + functions, `when`/`partition`/`filter` over `switch`, string templates, `isNullOrEmpty`, + view/KTX extensions. See [ANDROID-IDIOMS.md](references/ANDROID-IDIOMS.md). Retire + deprecated Android APIs (`onActivityResult`, options-menu overrides, + `java.util.Observable`) — see [DEPRECATED-APIS.md](references/DEPRECATED-APIS.md). +6. **Project conventions.** SPDX header, no magic numbers (`companion object` + + `const val`), resources not hardcoded strings, ≤300 lines/file, ≤120 cols. See + [PROJECT-CONVENTIONS.md](references/PROJECT-CONVENTIONS.md). +7. **Testability seams.** Extract pure logic (URL building, permission math, partitioning) + into functions you can unit-test; mark with `@VisibleForTesting` where they must stay + `internal`/`private`-ish but be reachable from tests. + +Do NOT expand scope. Unrelated files stay untouched (AGENTS.md / AI policy). + +## Step 2: Write a Behaviour-Locking Test (Mandatory) + +A conversion is not complete until a test proves behaviour is unchanged. See +[TESTING.md](references/TESTING.md) for the decision tree. In short: + +- **Pure functions you extracted** (e.g. link builders, `isReshareForbidden`, + partitioning) → fast JVM unit tests in `app/src/test/` (JUnit4 + mockito-kotlin). +- **Fragment/Activity/DB behaviour** → instrumented test in `app/src/androidTest/` + extending the project's base test class, or a Robolectric test where the project uses it. +- Prefer testing the **seams you just created**. If the Java original had no test, your + new test is the characterization test that locks current behaviour. + +Every test file gets the SPDX header and follows the project's test conventions. + +## Step 3: Verify + +Run and report real output — never claim green without evidence: + +```bash +# Formatting + static analysis on the changed files +./gradlew spotlessKotlinCheck detektGplayDebug lintGplayDebug spotbugsGplayDebug + +# Unit tests +./gradlew jacocoTestGplayDebugUnitTest + +# Instrumented tests (if you wrote one), scoped to the class +./gradlew createGplayDebugCoverageReport -Pcoverage=true \ + -Pandroid.testInstrumentationRunnerArguments.class= +``` + +## Worked Example + +[assets/worked-example.md](assets/worked-example.md) is a real before/after from +`FileDetailSharingFragment` (871-line Java fragment → idiomatic Kotlin) showing each +transformation class in context. Read it when you need a concrete pattern. + +## Batch Conversion + +Convert one file at a time, leaf dependencies first. Report progress per file. Warn the +developer before a change set grows into several thousand lines — split into focused PRs +(AGENTS.md). Each commit needs `Assisted-by: :`; only the human adds +`Signed-off-by`. diff --git a/.claude/skills/android-java-to-kotlin/assets/worked-example.md b/.claude/skills/android-java-to-kotlin/assets/worked-example.md new file mode 100644 index 000000000000..58d1ce0379d9 --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/assets/worked-example.md @@ -0,0 +1,163 @@ +# Worked Example: FileDetailSharingFragment + +A real conversion of an 871-line Java fragment +(`com.owncloud.android.ui.fragment.FileDetailSharingFragment`) into idiomatic Kotlin. Each +section shows one transformation class from the skill, in context. Behaviour is unchanged +throughout; the only new capability (`@VisibleForTesting createInternalLink`) is a +testability seam that returns the same URL the Java produced. + +## A. SPDX Header Rewrite + +```diff +-/* +- * Nextcloud Android client application +- * +- * @author Andy Scherzinger +- * ... +- * Copyright (C) 2018 Andy Scherzinger +- * SPDX-License-Identifier: AGPL-3.0-or-later OR GPL-2.0-only +- */ ++/* ++ * Nextcloud - Android Client ++ * ++ * SPDX-FileCopyrightText: 2026 Alper Ozturk ++ * SPDX-License-Identifier: AGPL-3.0-or-later ++ */ +``` + +## B. Fail-Fast Preconditions + +```diff +-if (file == null) throw new IllegalArgumentException("File may not be null"); +-if (user == null) throw new IllegalArgumentException("Account may not be null"); +-fileActivity = (FileActivity) getActivity(); +-if (fileActivity == null) throw new IllegalArgumentException("FileActivity may not be null"); ++fileActivity = (activity as FileActivity?) ++requireNotNull(file) { "File may not be null" } ++requireNotNull(user) { "Account may not be null" } ++requireNotNull(fileActivity) { "FileActivity may not be null" } +``` + +## C. Decomposition of `onViewCreated` + +The Java `onViewCreated` inlined adapter creation (duplicated for internal/external), +layout managers, listeners, and the fetch kick-off. It became a readable sequence plus +extracted helpers `getUserId()`, `setupInternalShares()`, `setupExternalShares()`, +`createShareListAdapter(userId, type)`, `startAnimation()`. The two near-identical adapter +blocks collapsed into one parameterized factory: + +```kotlin +private fun createShareListAdapter(userId: String, type: SharesType): ShareeListAdapter = + ShareeListAdapter( + fileActivity!!, ArrayList(), this, userId, user, viewThemeUtils, + (file?.isEncrypted == true), type + ).apply { setHasStableIds(true) } +``` + +## D. `new Thread` + `runOnUiThread` → `lifecycleScope` + `suspend` + +```diff +-private void fetchE2EECounter(Runnable onComplete) { +- new Thread(() -> { +- try { ... fileDataStorageManager.saveFile(file); } +- catch (Exception e) { Log_OC.e(TAG, "..." + e.getMessage()); } +- Activity a = getActivity(); +- if (a != null) a.runOnUiThread(onComplete); +- }).start(); +-} ++private suspend fun fetchE2EECounter(): Boolean = withContext(Dispatchers.IO) { ++ return@withContext try { ++ val client = clientFactory.create(user) ++ val metadata = RefreshFolderOperation.getDecryptedFolderMetadata(true, file, client, user, requireContext()) ++ if (metadata is DecryptedFolderMetadataFile) { ++ file?.setE2eCounter(metadata.metadata.counter) ++ fileDataStorageManager?.saveFile(file) ++ } ++ true ++ } catch (e: Exception) { ++ Log_OC.e(TAG, "Error refreshing E2E counter: " + e.message) ++ false ++ } ++} +``` + +Caller now `lifecycleScope.launch { if (!fetchE2EECounter()) return@launch; withContext(Main){ ... } }`. +The callback-pair `fetchSharees(onSuccess, onError)` was likewise turned into a `suspend` +call returning `Boolean`. + +## E. `switch` Bucketing → `partition` + Set + +```diff +-for (OCShare share : shares) { +- if (share.getShareType() != null) { +- switch (share.getShareType()) { +- case PUBLIC_LINK: case FEDERATED_GROUP: case FEDERATED: case EMAIL: +- externalShares.add(share); break; +- default: internalShares.add(share); break; +- } +- } +-} ++private val externalShareTypes = setOf( ++ ShareType.PUBLIC_LINK, ShareType.FEDERATED_GROUP, ShareType.FEDERATED, ShareType.EMAIL ++) ++val (external, internal) = shares ++ .filter { it.shareType != null } ++ .partition { it.shareType in externalShareTypes } +``` + +## F. Nested `if` Cursor Handling → Guard Clauses + +The three-level-nested `handleContactResult` became flat sequential guards, each showing +the snackbar + log and returning, with `cursor.close()` preserved on every path. See +[FAIL-FAST.md](../references/FAIL-FAST.md) section "Deeply Nested". + +## G. Scope Functions & KTX + +```diff +-final LinearLayout shimmerLayout = binding.shimmerLayout.getRoot(); +-shimmerLayout.clearAnimation(); +-shimmerLayout.setVisibility(View.GONE); +-binding.shareContainer.setVisibility(View.VISIBLE); ++binding?.run { ++ shimmerLayout.root.run { clearAnimation(); visibility = View.GONE } ++ shareContainer.visibility = View.VISIBLE ++} +``` + +```diff +-for (int i = 0; i < viewGroup.getChildCount(); i++) { toggleSearchViewEnable(viewGroup.getChildAt(i), enable); } ++for (i in 0.. 3); ++companion object { ++ private const val TAG = "FileDetailSharingFragment" ++ private const val MIN_SHOW_ALL_VISIBLE_ITEM_COUNT = 3 ++ @JvmStatic fun newInstance(file: OCFile?, user: User?) = FileDetailSharingFragment().apply { ... } ++} ++binding.sharesListInternalShowAll.setVisibleIf(internalShares.size > MIN_SHOW_ALL_VISIBLE_ITEM_COUNT) +``` + +## I. Testability Seam + +`createInternalLink` was inlined string concatenation in Java. It was extracted to a pure +`@VisibleForTesting internal fun createInternalLink(user, file, capabilities): String` that +returns the identical URL — enabling the unit test in +[TESTING.md](../references/TESTING.md) without a device. + +## J. Detekt Suppression (Last Resort) + +The class is genuinely large and can't be fully split within one conversion, so a +file-level suppression documents the debt honestly: + +```kotlin +@Suppress("TooManyFunctions", "LargeClass", "TooGenericExceptionCaught", "ReturnCount") +class FileDetailSharingFragment : Fragment(), ... { +``` + +Prefer real decomposition; use this only when splitting is out of scope, and tell the +developer. diff --git a/.claude/skills/android-java-to-kotlin/references/ANDROID-IDIOMS.md b/.claude/skills/android-java-to-kotlin/references/ANDROID-IDIOMS.md new file mode 100644 index 000000000000..b669874f1873 --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/references/ANDROID-IDIOMS.md @@ -0,0 +1,170 @@ + + +# Android + Kotlin Idioms + +Transformations applied during the idiomatic pass. Every one is behaviour-preserving. +Examples are drawn from a real fragment conversion. + +## 1. Decompose Oversized Functions + +The IDE keeps the Java structure: one enormous `onViewCreated`/`setupView` that inflates, +themes, wires listeners, and kicks off loading in a single 80-line block. Split by +intent into small private functions. The lifecycle callback becomes a readable table of +contents. + +```kotlin +// BEFORE: onViewCreated does everything inline (adapters, layout managers, listeners, fetch) + +// AFTER +override fun onViewCreated(view: View, savedInstanceState: Bundle?) { + super.onViewCreated(view, savedInstanceState) + fileActivity ?: return + fileDataStorageManager = fileActivity?.storageManager + fileOperationsHelper = fileActivity?.fileOperationsHelper + + startAnimation() + val userId = getUserId() + setupInternalShares(userId) + setupExternalShares(userId) + binding?.pickContactEmailBtn?.setOnClickListener { checkContactPermission() } + fetchSharees() + setupView() +} +``` + +Rules: +- One function = one reason to change. Name it for *what it accomplishes* + (`setupInternalShares`, `themeView`, `disableE2EEShareForV1`), not *how*. +- Factor duplicated blocks into a parameterized helper + (`createShareListAdapter(userId, SharesType.INTERNAL)`). +- Keep files ≤300 lines (project rule). Heavy decomposition sometimes means splitting a + god-class into collaborators — raise that with the developer rather than exceeding 300. + +## 2. Scope Functions Over Repetition + +Replace repeated `binding.x` / `viewThemeUtils.material.y` chains with `run`/`apply`/`with`. + +```kotlin +// BEFORE +viewThemeUtils.material.themeSearchCardView(binding.searchCardWrapper); +viewThemeUtils.material.colorMaterialButtonPrimaryOutlined(binding.sendCopyBtn); +viewThemeUtils.material.colorMaterialButtonPrimaryBorderless(binding.sharesListInternalShowAll); + +// AFTER +binding.run { + viewThemeUtils.material.run { + themeSearchCardView(searchCardWrapper) + colorMaterialButtonPrimaryOutlined(sendCopyBtn) + colorMaterialButtonPrimaryBorderless(sharesListInternalShowAll) + } +} +``` + +Use `apply {}` when configuring and returning the receiver: + +```kotlin +ShareeListAdapter(fileActivity!!, ArrayList(), this, userId, user, viewThemeUtils, encrypted, type) + .apply { setHasStableIds(true) } +``` + +## 3. `switch` → `when` / `filter` + `partition` + +Collapse a `switch` that sorts items into buckets into a declarative pipeline with a +constant `Set`. + +```kotlin +// BEFORE: for-loop with switch(shareType) adding to internalShares / externalShares + +// AFTER +private val externalShareTypes = setOf( + ShareType.PUBLIC_LINK, ShareType.FEDERATED_GROUP, ShareType.FEDERATED, ShareType.EMAIL +) + +val (external, internal) = shares + .filter { it.shareType != null } + .partition { it.shareType in externalShareTypes } +``` + +## 4. Extension Functions & KTX + +Import members directly and lean on AndroidX KTX instead of verbose Java utilities. + +| Java / verbose | Idiomatic Kotlin | +|---|---| +| `TextUtils.isEmpty(s)` | `s.isNullOrEmpty()` | +| `BundleExtensionsKt.getParcelableArgument(b, k, T.class)` | `b.getParcelableArgument(k, T::class.java)` | +| `for (int i = 0; i < vg.getChildCount(); i++)` | `for (i in 0..` / `// endregion` (lifecycle, +private methods, overrides, companion) aids IDE folding. This is IDE structure, not a +decorative divider. Match the surrounding file's existing style; do not introduce ASCII +banner comments (`// ==== ====`), which the project forbids. diff --git a/.claude/skills/android-java-to-kotlin/references/CONCURRENCY.md b/.claude/skills/android-java-to-kotlin/references/CONCURRENCY.md new file mode 100644 index 000000000000..449608df0078 --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/references/CONCURRENCY.md @@ -0,0 +1,213 @@ + + +# Concurrency: Java Threads → Coroutines & `lifecycleScope` + +Replace `new Thread`, `AsyncTask`, executors, and `runOnUiThread`/`Handler.post` +callback-passing with structured coroutines scoped to the component lifecycle. This is +behaviour-preserving *and* fixes leaks: `lifecycleScope`/`viewModelScope` cancel +automatically when the owner is destroyed, so no work touches a dead view. + +## Choose the Right Scope + +| Context | Scope | +|---|---| +| `Fragment` (UI work tied to view) | `viewLifecycleOwner.lifecycleScope` (preferred) or `lifecycleScope` | +| `Activity` | `lifecycleScope` | +| `ViewModel` | `viewModelScope` | +| No lifecycle owner | inject a `CoroutineScope` / use a repository suspend fun | + +Use `Dispatchers.IO` for disk/network/DB, `Dispatchers.Main` (or `withContext(Main)`) to +touch views. + +## `AsyncTask` → Coroutines + +`AsyncTask` is deprecated. Map its callbacks onto a single `launch`: + +| `AsyncTask` | Coroutine | +|---|---| +| `onPreExecute()` | code before `withContext(IO)` (runs on Main) | +| `doInBackground()` | `withContext(Dispatchers.IO) { ... }` | +| `onPostExecute(result)` | code after, back on `Dispatchers.Main` | +| `isCancelled()` | `isActive` (or `ensureActive()`) | +| `cancel(true)` | `job.cancel()` | +| the `AsyncTask` instance field | the returned `Job` | + +A self-contained task class stops extending `AsyncTask` and holds the lifecycle owner so +its scope cancels with the screen: + +```kotlin +// BEFORE: class GallerySearchTask extends AsyncTask { ... } +class GallerySearchTask( + private val fragment: GalleryFragment, + private val user: User, + private val storageManager: FileDataStorageManager, + private val endDate: Long, + private val limit: Int +) { + fun execute(): Job = fragment.lifecycleScope.launch(Dispatchers.IO) { + if (!isActive) return@launch + val context = fragment.context ?: return@launch + val result = performSearch(context) + withContext(Dispatchers.Main) { + fragment.searchCompleted(result.emptySearch, result.lastTimestamp) + } + } +} +``` + +The caller keeps the `Job` and cancels it with the view: + +```kotlin +private var photoSearchTask: Job? = null +// ... +photoSearchTask = GallerySearchTask(this, user, storageManager, endDate, limit).execute() + +override fun onDestroyView() { + photoSearchTask?.cancel() + photoSearchTask = null +} +``` + +Because `lifecycleScope` cancels on destroy, the old `WeakReference` leak-guard +disappears — a direct `fragment` reference plus `fragment.context ?: return@launch` is +enough. (PR #16908) + +## `AsyncTask` Behind a Callback API → `runCatching` + `fold` + +When a service method hid an `AsyncTask` that set mutable fields (a boolean success flag, +`errorMessage`, result fields) and dispatched them in `onPostExecute`, keep the public +callback signature but drive it from a coroutine. Return an **immutable** result from the +background function and branch with `fold`: + +```kotlin +private data class ActivitiesResult( + val activities: List, + val client: NextcloudClient, + val lastGiven: Long +) + +override fun getActivities(lastGiven: Long, callback: ActivitiesServiceCallback) { + scope.launch { + runCatching { withContext(Dispatchers.IO) { fetchActivities(lastGiven) } } + .fold( + onSuccess = { (activities, client, updated) -> + callback.onLoaded(activities, client, updated) + }, + onFailure = { callback.onError(it.message ?: "") } + ) + } +} +``` + +`runCatching` / `fold` replaces the boolean-success + `errorMessage`-field idiom; the +background function `fetchActivities` throws on failure instead of returning `false`, and +the immutable `ActivitiesResult` destructures straight into `onSuccess`. (PR #16654) + +> **Scope caveat:** that PR launched on an ad-hoc `CoroutineScope(Dispatchers.Main)` with +> no lifecycle owner — it never cancels and can leak. Prefer an injected `CoroutineScope` +> or expose a `suspend` function the presenter runs on `lifecycleScope` / `viewModelScope`. +> Flag ad-hoc scopes in review. + +## `new Thread { ... runOnUiThread(...) }` → `lifecycleScope.launch` + `withContext` + +```kotlin +// BEFORE +private void fetchE2EECounter(Runnable onComplete) { + new Thread(() -> { + try { + OwnCloudClient client = clientFactory.create(user); + Object metadata = RefreshFolderOperation.getDecryptedFolderMetadata(true, file, client, user, ctx); + if (metadata instanceof DecryptedFolderMetadataFile m) { + file.setE2eCounter(m.getMetadata().getCounter()); + fileDataStorageManager.saveFile(file); + } + } catch (Exception e) { Log_OC.e(TAG, "..." + e.getMessage()); } + Activity a = getActivity(); + if (a != null) a.runOnUiThread(onComplete); + }).start(); +} + +// AFTER: background work is a suspend fun on IO; the UI reaction runs on Main +private suspend fun fetchE2EECounter(): Boolean = withContext(Dispatchers.IO) { + return@withContext try { + val context = requireContext() + val client = clientFactory.create(user) + val metadata = RefreshFolderOperation.getDecryptedFolderMetadata(true, file, client, user, context) + if (metadata is DecryptedFolderMetadataFile) { + file?.setE2eCounter(metadata.metadata.counter) + fileDataStorageManager?.saveFile(file) + } + true + } catch (e: Exception) { + Log_OC.e(TAG, "Error refreshing E2E counter: " + e.message) + false + } +} + +// caller +lifecycleScope.launch { + val ok = fetchE2EECounter() + if (!ok) return@launch + withContext(Dispatchers.Main) { /* update views */ } +} +``` + +Notes: +- The old code returned nothing and drove the UI via a `Runnable`. The idiomatic version + returns a `Boolean` result and lets the caller decide — same observable outcome, no + callback threading. +- `getActivity() != null` guard becomes `lifecycleScope` cancellation + `binding?`/`?: + return` guards. Preserve any "is the view still alive?" check as a `binding == null` + guard inside `launch`. + +## Callback-Pair API → `suspend` Result + +Repositories that took `onSuccess`/`onError` lambdas become `suspend` functions returning +a result, awaited inside a coroutine. + +```kotlin +// BEFORE +shareRepository.fetchSharees(remotePath, onSuccess = { ... }, onError = { ... }); + +// AFTER +lifecycleScope.launch { + val result = shareRepository.fetchSharees(remotePath) + if (binding == null) return@launch + if (result) { refreshSharesFromDB(); stopLoadingAnimationAndShowShareContainer() } + else { stopLoadingAnimationAndShowShareContainer(); showError() } +} +``` + +## Moving DB/IO off the Main Thread + +Wrap the blocking DB read in `withContext(Dispatchers.IO)` and hop back to `Main` to touch +adapters/views. This can be a *behaviour improvement* (fewer main-thread stalls) — flag it +to the developer if the Java version was doing DB work on the main thread synchronously, +since timing changes can be observable (e.g. tests that assumed synchronous population). + +```kotlin +private suspend fun loadAndPartitionShares(): Pair, List> = + withContext(Dispatchers.IO) { + val shares = fileDataStorageManager?.getSharesWithForAFile(file?.remotePath, user?.accountName) + ?: emptyList() + val (external, internal) = shares.filter { it.shareType != null } + .partition { it.shareType in externalShareTypes } + internal to external + } +``` + +## Pitfalls + +- Never launch on `GlobalScope` — it outlives the screen and leaks. +- An ad-hoc `CoroutineScope(Dispatchers.Main)` created inside a service/presenter has the + same problem: no lifecycle owner, no cancellation, a leak. Use an injected scope or a + `suspend` function the caller runs on `lifecycleScope` / `viewModelScope`. +- Keep `try/catch` semantics: coroutine cancellation throws `CancellationException`; + rethrow it (don't swallow in a broad `catch (e: Exception)` that logs and continues) or + catch `Exception` only around the real work, not around the whole `launch`. +- Preserve exception-to-UI mapping exactly (same snackbar, same log tag/message). diff --git a/.claude/skills/android-java-to-kotlin/references/DEPRECATED-APIS.md b/.claude/skills/android-java-to-kotlin/references/DEPRECATED-APIS.md new file mode 100644 index 000000000000..944b78541ad2 --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/references/DEPRECATED-APIS.md @@ -0,0 +1,85 @@ + + +# Retiring Deprecated Android APIs During Conversion + +A Java→Kotlin conversion is the right moment to replace deprecated Android APIs the IDE +converter leaves untouched. Each replacement below is behaviour-preserving. Examples are +real conversions (nextcloud/android PRs #16878, #16792). + +## `startActivityForResult` / `onActivityResult` → Activity Result API + +The request-code + `onActivityResult` protocol is deprecated. Register a launcher at +construction time and receive the result in its callback. + +```kotlin +// BEFORE +startActivityForResult(action, SELECT_LOCATION_REQUEST_CODE) +override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?) { + if (requestCode == SELECT_LOCATION_REQUEST_CODE && data != null) { handle(data) } +} + +// AFTER +private val folderPickerLauncher = registerForActivityResult( + ActivityResultContracts.StartActivityForResult() +) { result -> + if (result.resultCode == Activity.RESULT_OK) { + handle(result.data) + } +} + +// launch it: +folderPickerLauncher.launch(intent) +``` + +Type-safe, no manual request-code bookkeeping, and it survives process death because +registration is declarative. Register during initialization (a field initializer or +`onCreate`/`onViewCreated`) — never inside a click handler, or the registration is lost. + +## `onCreateOptionsMenu` / `onOptionsItemSelected` → `MenuProvider` + +`setHasOptionsMenu(true)` plus the two menu overrides are deprecated on `Fragment`. Add a +`MenuProvider` bound to the view lifecycle instead. + +```kotlin +// AFTER +val menuHost: MenuHost = requireActivity() +menuHost.addMenuProvider(object : MenuProvider { + override fun onCreateMenu(menu: Menu, inflater: MenuInflater) = + inflater.inflate(R.menu.gallery_menu, menu) + + override fun onMenuItemSelected(item: MenuItem): Boolean = when (item.itemId) { + R.id.action_select_all -> { selectAll(); true } + else -> false + } +}, viewLifecycleOwner, Lifecycle.State.RESUMED) +``` + +Passing `viewLifecycleOwner` + `Lifecycle.State.RESUMED` auto-adds and removes the menu as +the view's lifecycle changes — no leak, no manual `setHasOptionsMenu`. (PR #16878) + +## `java.util.Observable` — Keep or Migrate? + +`java.util.Observable` / `Observer` are deprecated (Java 9+). But a conversion is +behaviour-locked, so **do not** silently swap the notification mechanism — existing Java +observers rely on `setChanged()` / `notifyObservers()`. PR #16792 deliberately KEPT it: + +```kotlin +class UploadsStorageManager(...) : Observable() { + fun notifyObserversNow() { + Handler(Looper.getMainLooper()).post { + setChanged() + notifyObservers() + } + } +} +``` + +Migrating to `StateFlow` / `SharedFlow` changes the observation contract and every call +site — that is a separate, opt-in refactor, not part of a 1:1 conversion. Note the +deprecation, propose the flow migration as a follow-up, and keep the current mechanism +unless the developer scopes the larger change. diff --git a/.claude/skills/android-java-to-kotlin/references/FAIL-FAST.md b/.claude/skills/android-java-to-kotlin/references/FAIL-FAST.md new file mode 100644 index 000000000000..79a0ef910f22 --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/references/FAIL-FAST.md @@ -0,0 +1,115 @@ + + +# Fail Fast: Guard Clauses Over Nested `if`/`else` + +The IDE preserves Java's nested-`if` pyramids. Invert them into guard clauses that return +(or throw) early, leaving the happy path at the lowest indentation. Behaviour is +identical — the branches are the same, only the shape changes. + +## Precondition Checks → `require` / `requireNotNull` + +`requireNotNull` returns the smart-cast non-null value AND throws +`IllegalArgumentException` with the message — exactly matching the Java `if (x == null) +throw new IllegalArgumentException(...)`. + +```kotlin +// BEFORE +if (file == null) throw IllegalArgumentException("File may not be null"); +if (user == null) throw IllegalArgumentException("Account may not be null"); +fileActivity = (FileActivity) getActivity(); +if (fileActivity == null) throw IllegalArgumentException("FileActivity may not be null"); + +// AFTER +fileActivity = activity as? FileActivity +requireNotNull(file) { "File may not be null" } +requireNotNull(user) { "Account may not be null" } +requireNotNull(fileActivity) { "FileActivity may not be null" } +``` + +Use `require(condition) { msg }` for boolean preconditions: + +```kotlin +require(activity is FileActivity) { "Calling activity must be of type FileActivity" } +``` + +`check`/`checkNotNull` are the `IllegalStateException` equivalents — use them when the Java +threw `IllegalStateException`. Match the original exception type; that is observable +behaviour. + +## Early Return Over Nested Success Path + +```kotlin +// BEFORE +private void checkShareViaUser() { + if (!MDMConfig.INSTANCE.shareViaUser(requireContext())) { + binding.searchContainer.setVisibility(View.GONE); + } +} + +// AFTER +private fun checkShareViaUser() { + if (shareViaUser(requireContext())) return + binding?.searchContainer?.visibility = View.GONE +} +``` + +## Deeply Nested `if`/`else` → Sequential Guards + +The most valuable transformation. A cursor-handling method nested three levels deep +becomes a flat sequence of guard clauses, each handling one failure and returning. + +```kotlin +// BEFORE: if (cursor != null) { if (moveToFirst()) { if (columnIndex != -1) {...} else ... } else ... } else ... + +// AFTER +private fun handleContactResult(contactUri: Uri) { + val cursor = fileActivity?.contentResolver?.query(contactUri, projection, null, null, null) + if (cursor == null) { + DisplayUtils.showSnackMessage(this, R.string.email_pick_failed) + Log_OC.e(TAG, "Failed to pick email address as Cursor is null.") + return + } + if (!cursor.moveToFirst()) { + DisplayUtils.showSnackMessage(this, R.string.email_pick_failed) + Log_OC.e(TAG, "Failed to pick email address as no Email found.") + return + } + val columnIndex = cursor.getColumnIndex(ContactsContract.CommonDataKinds.Email.ADDRESS) + if (columnIndex == -1) { + DisplayUtils.showSnackMessage(this, R.string.email_pick_failed) + Log_OC.e(TAG, "Failed to pick email address.") + cursor.close() + return + } + val email = cursor.getString(columnIndex) + // ... happy path at base indentation + cursor.close() +} +``` + +Watch the cleanup: if the Java relied on falling through to a single `cursor.close()`, +each early return must still close it (or wrap in `use {}`). Missing that changes +behaviour (resource leak) — verify it. + +## Nullable-Guard Idioms + +```kotlin +val activity = fileActivity ?: return +val clientRepository = activity.clientRepository ?: return +val remotePath = file?.remotePath ?: return +``` + +Each `?: return` collapses one Java `if (x == null) return;`. Chain them at the top of the +function so the body works with non-null smart-cast locals. + +## When NOT to Invert + +- Do not turn a genuine two-branch decision (both branches do real work) into a guard if + it obscures the symmetry — a `when`/`if-else` expression is clearer there. +- Do not change the *order* of side-effects while inverting; the snackbar/log calls above + must fire in the same cases as before. diff --git a/.claude/skills/android-java-to-kotlin/references/PROJECT-CONVENTIONS.md b/.claude/skills/android-java-to-kotlin/references/PROJECT-CONVENTIONS.md new file mode 100644 index 000000000000..5d2e15f89d00 --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/references/PROJECT-CONVENTIONS.md @@ -0,0 +1,73 @@ +# Project Conventions (Nextcloud Android) + +Apply these during Step 2 so the converted file passes review and the quality gates. They +are enforced by `spotlessKotlinCheck`, `detekt`, `lint`, and `spotbugsGplayDebug`. + +## SPDX Header (every new/renamed file) + +The IDE keeps the old license block. Replace it with the current template. The year is the +year the Kotlin file is created. New contributions are `AGPL-3.0-or-later`; keep +`OR GPL-2.0-only` only if the original file carried it. + +```kotlin +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +``` + +If the developer wants personal attribution (as in the real conversion), the form +`SPDX-FileCopyrightText: ` is also used — match what the developer +asks for; default to the "Nextcloud GmbH and Nextcloud contributors" line. + +## Structural Rules + +- **≤300 lines per file.** If decomposition pushes past it, split responsibilities into + separate files/collaborators and tell the developer. A file/class-level + `@Suppress("LargeClass", "TooManyFunctions")` is a last resort for legacy god-classes. +- **≤120 columns per line.** +- **One top-level type per file.** Extract models, states, sealed classes, and listener + interfaces into their own files rather than nesting many types in one. +- **Exactly one trailing newline** at end of file. + +## No Magic Numbers / Hardcoded Resources + +- Extract literals into named `const val` in a `companion object` + (`MIN_SHOW_ALL_VISIBLE_ITEM_COUNT = 3`, `INTERNAL_LINK_PATH_PRETTY = "/f/"`). +- Strings, colors, dimens come from resources (`R.string.*`, `R.dimen.*`), never inline. +- Only `app/src/main/res/values/strings.xml` may be edited for strings; never touch + `values-*` translation folders. + +## Comments & Naming + +- No decorative divider comments (`// ==== ====`, `// ---- Title ----`). `// region` / + `// endregion` for IDE folding is allowed and should match the file's existing style. +- Prefer self-explanatory names over per-function KDoc. Preserve genuinely informative + Javadoc as KDoc (invariant 4); drop noise. +- Do not use multiple boolean flags to model state — use an `enum`/sealed class. + +## Modern Java Interop + +When the file still has Java callers, keep the Java-facing API clean: +`@JvmStatic` for factory/companion functions, `@JvmField` for exposed constants, +`@JvmOverloads` for defaulted params, `@Throws` for checked exceptions. + +## Git & Commits (developer-driven) + +- Preserve history: the rename `git mv Foo.java Foo.kt` should be a **separate commit** + from the content change so `git blame` follows through. +- Conventional Commits (`refactor(sharing): convert FileDetailSharingFragment to Kotlin`). +- Every AI-assisted commit needs an `Assisted-by: :` trailer. +- Only the human contributor adds `Signed-off-by` (DCO). You must never add it, and never + open PRs/issues autonomously (AI policy). + +## Quality Gate + +```bash +./gradlew spotlessKotlinCheck detektGplayDebug lintGplayDebug spotbugsGplayDebug \ + jacocoTestGplayDebugUnitTest +``` + +Fix every finding in the files you changed before declaring done. diff --git a/.claude/skills/android-java-to-kotlin/references/TESTING.md b/.claude/skills/android-java-to-kotlin/references/TESTING.md new file mode 100644 index 000000000000..20f313683e0e --- /dev/null +++ b/.claude/skills/android-java-to-kotlin/references/TESTING.md @@ -0,0 +1,107 @@ +# Behaviour-Locking Tests (Mandatory Step) + +A conversion is not done until a test proves behaviour did not change. If the Java class +already had tests, they are your primary safety net — run them and keep them green. If it +had none, you must add a **characterization test** that pins the current behaviour so the +refactor is provably safe. + +## Decision Tree + +```dot +digraph test_choice { + rankdir=LR; + "What are you locking?" -> "Pure logic (no Android SDK)" [label="fn returns a value"]; + "What are you locking?" -> "Component behaviour" [label="Fragment/Activity/DB/View"]; + "Pure logic (no Android SDK)" -> "JVM unit test app/src/test/ (JUnit4 + mockito-kotlin)"; + "Component behaviour" -> "Robolectric unit test (if project uses it)"; + "Component behaviour" -> "Instrumented test app/src/androidTest/ extends base IT class"; +} +``` + +**Prefer the seams you just created.** The idiomatic pass extracts pure functions +(link builders, permission math, partitioning, `isReshareForbidden`) specifically so they +can be unit-tested fast without a device. Test those first; they give the most behaviour +coverage per second. + +## 1. JVM Unit Tests — Pure Functions + +Location: `app/src/test/`. Fast, no emulator. Command: +`./gradlew jacocoTestGplayDebugUnitTest`. + +Mark the function `@VisibleForTesting internal` so the test module can reach it while it +stays out of the public API. + +```kotlin +/* + * Nextcloud - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +package com.owncloud.android.ui.fragment + +import com.owncloud.android.lib.resources.status.OCCapability +import org.junit.Assert.assertEquals +import org.junit.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever + +class FileDetailSharingFragmentTest { + + @Test + fun `internal link uses pretty path when modRewrite is on`() { + val user = mock { whenever(it.server.uri).thenReturn(URI("https://cloud.example")) } + val file = mock { whenever(it.localId).thenReturn(42L) } + val caps = mock { whenever(it.modRewriteWorking.isTrue).thenReturn(true) } + + val link = FileDetailSharingFragment().createInternalLink(user, file, caps) + + assertEquals("https://cloud.example/f/42", link) + } + + @Test + fun `internal link falls back to index php path when modRewrite is off`() { + // ... same setup with modRewriteWorking.isTrue == false + // assertEquals("https://cloud.example/index.php/f/42", link) + } +} +``` + +The point: run this test's logic against **both** the pre-conversion and post-conversion +code paths mentally (or, if the Java is still on disk, literally) and confirm the assertion +holds for both. That is what "behaviour must not change" means concretely. + +## 2. Instrumented Tests — Component Behaviour + +Location: `app/src/androidTest/`. For Fragment/Activity/DB behaviour that needs the Android +runtime. Extend the project's base test class (`AbstractOnServerIT` when server +communication is required) and follow its conventions (separate test user, etc.). + +```bash +./gradlew createGplayDebugCoverageReport -Pcoverage=true \ + -Pandroid.testInstrumentationRunnerArguments.class=com.owncloud.android.ui.fragment.FileDetailSharingFragmentIT +``` + +Use `@VisibleForTesting` hooks the original exposed (e.g. a `search(query)` or +`showSharingMenuActionSheet(share)` method) to drive and assert UI state, exactly as the +existing IT suite does. Do not weaken visibility further than the Java original did. + +## 3. What to Assert + +Lock the observable contract, not the implementation: +- Same return values / thrown exception types for the same inputs (including the + `require`/`requireNotNull` messages if callers depend on them). +- Same Bundle keys, intent extras, and `newInstance` argument wiring. +- Same branch outcomes (e.g. "reshare forbidden when FEDERATED", "show-all button visible + only when > 3 shares"). +- For coroutine conversions, assert the end state, and use the project's test dispatcher / + `runTest` so async work is deterministic — never assert on wall-clock timing. + +## 4. Guardrails + +- Every new test file gets the SPDX header and ends with exactly one trailing newline. +- Do not modify unrelated tests to make yours pass. +- Report actual test output. If a test reveals the conversion changed behaviour, fix the + conversion — do not adjust the test to match the drift. +- If a genuine pre-existing bug is uncovered, surface it to the developer separately; do + not fold a behaviour change into the conversion. diff --git a/AGENTS.md b/AGENTS.md index 5fdd984bd590..9614463afedf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,6 +30,13 @@ Java, Kotlin, XML, Jetpack Compose are the key technologies used for building th - `./app/src/test/` Unit tests (small, isolated tests without Android SDK) - `./app/src/androidTest/` Instrumented tests (require Android SDK) - `./app/src/main/res/values/` Translations. Only update `./app/src/main/res/values/strings.xml`. Do not modify any other translation files or folders. Ignore all `values-*` directories (e.g., `values-es`, `values-fr`). +- `./.claude/skills/` Reusable agent skills. Each subdirectory is one skill with a `SKILL.md` entry point plus `references/` and `assets/`. + +## Agent Skills + +Project-specific skills live in `./.claude/skills//`. Load a skill when the task matches its trigger. + +- **`android-java-to-kotlin`** (`./.claude/skills/android-java-to-kotlin/SKILL.md`) — Completes a Java-to-Kotlin conversion in this Android app. Use it when finishing a conversion, when the user mentions "java to kotlin", "j2k", "convert java", or "make it idiomatic", or when a freshly IDE-converted `.kt` file needs cleanup. The workflow is two-person: the developer first runs the Android Studio converter (`Code > Convert Java File to Kotlin File`), then the agent drives the idiomatic second pass — fail-fast control flow, function decomposition, `lifecycleScope`/coroutines instead of Java threads, modern Android APIs, and project conventions (SPDX headers, no magic numbers, `@JvmStatic`). The conversion must preserve behaviour, and the agent must write a behaviour-locking test before declaring it done. ## General Guidance