diff --git a/.claude/skills/android-idioms/SKILL.md b/.claude/skills/android-idioms/SKILL.md new file mode 100644 index 000000000..142bc8105 --- /dev/null +++ b/.claude/skills/android-idioms/SKILL.md @@ -0,0 +1,176 @@ +--- +name: android-idioms +description: Idiomatic Kotlin for Android in this codebase — decomposing oversized lifecycle functions, scope functions (run/apply/with), when and partition instead of switch, extension functions and AndroidX KTX over verbose Java utilities, null safety instead of platform types, and companion-object constants. Use when writing any new Kotlin or converting a Java class to Kotlin. +--- + + + +# Android + Kotlin Idioms + +The shape new Kotlin should take here, and the transformations to apply when converting +Java. Every one is behaviour-preserving. Examples are drawn from a real fragment conversion +in the wider nextcloud/android codebase — the principles transfer, but the class names +(`FileDetailSharingFragment`, `OCFile`, `fileActivity`) are from that project, not this one. + +## 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/deprecated-apis/SKILL.md b/.claude/skills/deprecated-apis/SKILL.md new file mode 100644 index 000000000..8f510863d --- /dev/null +++ b/.claude/skills/deprecated-apis/SKILL.md @@ -0,0 +1,89 @@ +--- +name: deprecated-apis +description: Replacements for deprecated Android APIs — the Activity Result API instead of startActivityForResult/onActivityResult, MenuProvider instead of onCreateOptionsMenu/onOptionsItemSelected, and why a behaviour-locked conversion keeps java.util.Observable rather than silently moving to StateFlow. Use when touching activity results, fragment menus, or observer callbacks. +--- + + + +# Retiring Deprecated Android APIs + +Never introduce these deprecated APIs in new code, and replace them when you touch code +that uses them — a Java→Kotlin conversion is the right moment, since the IDE converter +leaves them untouched. Each replacement below is behaviour-preserving. Examples are real +conversions from the nextcloud/android client (PRs #16878, #16792), not from this repository. + +## `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/fail-fast/SKILL.md b/.claude/skills/fail-fast/SKILL.md new file mode 100644 index 000000000..cadb07fbb --- /dev/null +++ b/.claude/skills/fail-fast/SKILL.md @@ -0,0 +1,120 @@ +--- +name: fail-fast +description: Guard clauses instead of nested if/else in Kotlin — require/requireNotNull/check matched to the original exception type, flattening nested pyramids into sequential early returns, "?: return" chains for nullables, resource cleanup on every early-return path, and when not to invert a branch. Use for any code with preconditions, nullable values, or nested conditionals. +--- + + + +# Fail Fast: Guard Clauses Over Nested `if`/`else` + +Write new conditionals this way, and invert Java's nested-`if` pyramids into it when you +touch them: 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. +Examples come from the wider nextcloud/android codebase; the class names are not from this +repository. + +## 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/project-conventions/SKILL.md b/.claude/skills/project-conventions/SKILL.md new file mode 100644 index 000000000..2d40c021a --- /dev/null +++ b/.claude/skills/project-conventions/SKILL.md @@ -0,0 +1,91 @@ +--- +name: project-conventions +description: Nextcloud Notes house conventions for any file you add, rename, or edit — SPDX header form, the 300-line and one-top-level-type-per-file limits, magic numbers, string/color/dimen resources, Java-interop annotations, and the verification commands that actually exist in this repository. Use when creating or renaming a file, and before reporting any change as done. +--- + + + +# Project Conventions (Nextcloud Notes) + +Apply these to every file you write so the change passes review. + +## 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, 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**, and a file already at or above 300 lines must not grow. If + decomposition pushes past it, split responsibilities into separate files/collaborators and + tell the developer. Do not reach for `@Suppress("LargeClass", "TooManyFunctions")` — those + are detekt rule names and this repository has no detekt, so the annotation suppresses + nothing and merely hides the problem from the reader. +- **≤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 + +These are the tasks this project actually has. `gplay` is not a flavor here — the flavors +are `fdroid`, `play`, `dev`, and `qa`. + +```bash +./gradlew lintFdroidDebug +./gradlew testFdroidDebugUnitTest +./gradlew createFdroidDebugUnitTestCoverageReport # JaCoCo, debug builds only +./gradlew check # lint + unit tests for all variants +``` + +Fix every finding in the files you changed before declaring done. Style rules that no task +enforces — line length, trailing newline, import order — are on you to check by reading the +diff. diff --git a/AGENTS.md b/AGENTS.md index 48d15afe0..b8e0b61d9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,13 +6,57 @@ This file provides guidance to all AI agents (Claude, Codex, Gemini, etc.) working with code in this repository. -You are an experienced engineer specialized on Java, Kotlin and familiar with the platform-specific details of Android. +You are an experienced engineer, familiar with the platform-specific details of Android. Much of this codebase +is still Java, and reading, editing, debugging and fixing that Java is a normal, expected part of your work — fix a bug +in a Java class by editing that Java class. What is not open to choice is the language of *new* files: those are always +Kotlin. Kotlin-first means new code is Kotlin, not that existing Java is off limits. ## Your Role - You implement features and fix bugs. -- Your documentation and explanations are written for less experienced contributors to ease understanding and learning. - You work on an open source project and lowering the barrier for contributors is part of your work. +- You explain your work to less experienced contributors in your chat replies and in the material the contributor uses + for the pull request description — never as comments inside the code. Readable code is the explanation the code + itself gets; see [Hard Rules](#hard-rules). + +## Hard Rules + +These are the rules that get broken most often. They are not preferences, and no local circumstance overrides them. +Verify each one against your own diff before you report a task as finished. + +1. **Every new file is Kotlin.** Never create a `.java` file. +2. **Write comments only if needed.** No explanatory line above a function, no note next to a variable, field, + branch or magic-free constant. Carry the meaning in names and small functions instead: if you feel the urge to + describe *what* the code does, rename it or extract it until the description is unnecessary. +3. **Never grow a large file.** 300 lines is the ceiling for any file. A file already at or above it must not gain a + single line: put the new code in a new Kotlin file — extension function, use case, mapper, state or model class — + and keep the edit to the existing file to the minimum that wires it up. "The class was already 900 lines" is a + reason not to add the 901st. If the task cannot be done without growing such a file, say so and + propose the extraction before writing the code. This rule bites on new functionality: never answer "where does this + new code go?" with "the bottom of the biggest class in the package." A fix that genuinely belongs in that file still + goes in that file. +4. **Review your own diff before reporting done.** Read it as a reviewer, not as its author, and delete what you would + ask a contributor to remove: dead code, unused parameters, redundant null checks, defensive branches that cannot be + reached, indirection used once, leftover scaffolding, and any file you touched only incidentally. Then confirm out + loud, in your final message, which language every new file is in and which files ended up over 300 lines. + +## Reference Guides + +`.claude/skills/` holds the detailed guidance behind the rules above: the concrete before/after transformations, so you +do not have to infer the house style from surrounding legacy code. Read the relevant guide **before** writing code, not +after a reviewer asks for changes. They apply to every agent, whichever tool you are: Claude Code loads them as skills +on demand, and every other agent can read them as plain Markdown with its file-reading tool. + +| Guide | Read it when | What it gives you | +|---|---|---| +| [`project-conventions`](.claude/skills/project-conventions/SKILL.md) | Any change that adds or renames a file | SPDX header form, the ≤300-line and one-top-level-type-per-file rules, magic-number and resource rules, Java-interop annotations (`@JvmStatic`, `@JvmOverloads`), commit expectations, and the verification commands that exist here | +| [`android-idioms`](.claude/skills/android-idioms/SKILL.md) | Writing any new Kotlin, or converting Java to Kotlin | How to decompose an oversized lifecycle function, scope functions (`run`/`apply`/`with`), `switch` → `when`/`partition`, extension functions and KTX over verbose Java utilities, null safety instead of platform types, `companion object` constants | +| [`fail-fast`](.claude/skills/fail-fast/SKILL.md) | Any code with preconditions, nullable values, or nested `if`/`else` | Guard-clause shapes, `require`/`requireNotNull`/`check` matched to the original exception type, flattening nested pyramids, `?: return` chains, and when *not* to invert a branch | +| [`deprecated-apis`](.claude/skills/deprecated-apis/SKILL.md) | Touching activity results, fragment menus, or observer callbacks | Activity Result API instead of `startActivityForResult`, `MenuProvider` instead of `onCreateOptionsMenu`, and why a behaviour-locked conversion keeps `java.util.Observable` rather than silently moving to Flow | + +Precedence: where a guide and the [Hard Rules](#hard-rules) appear to disagree, the Hard Rules win — flag the +contradiction to the contributor instead of quietly picking one. The guides use examples from the wider +nextcloud/android codebase; the principles transfer, the specific class names do not exist in this repository. ## Nextcloud Contribution Policy @@ -42,7 +86,9 @@ All contributions generated or assisted by this agent must fully comply with: ## Project Overview -Nextcloud Notes for Android — a notes management app that syncs with a Nextcloud server. Written primarily in Java (legacy) with new code in Kotlin. Targets API 24+ (minSdk 24, targetSdk 36). Uses Nextcloud Single Sign-On (SSO) for authentication. +Nextcloud Notes for Android — a notes management app that syncs with a Nextcloud server. Kotlin is the language of the +project; the large amount of Java still present is legacy that is being migrated away from, not a style to follow. +Targets API 28+ (minSdk 28, targetSdk 36). Uses Nextcloud Single Sign-On (SSO) for authentication. ## Build Commands @@ -62,13 +108,15 @@ Nextcloud Notes for Android — a notes management app that syncs with a Nextclo # Run instrumented tests (requires device/emulator) ./gradlew connectedAndroidTest -# Static analysis -./gradlew check -./gradlew ktlintCheck +# Android lint (per variant, or the default variant) +./gradlew lintFdroidDebug ./gradlew lint -# Auto-fix ktlint issues -./gradlew ktlintFormat +# Lint + unit tests across variants +./gradlew check + +# JaCoCo coverage report (debug builds only) +./gradlew createFdroidDebugUnitTestCoverageReport ``` Build output: `app/build/outputs/apk/` @@ -187,25 +235,27 @@ XML: ## Code Style +The bullets below are the summary; [`project-conventions`](.claude/skills/project-conventions/SKILL.md) and the other +[Reference Guides](#reference-guides) show what each one looks like in practice. + [//]: # (REUSE-IgnoreStart) -- Do not exceed 300 line of code per file. +- Every new file is Kotlin, no file exceeds 300 lines, and code carries no comments — see [Hard Rules](#hard-rules). - Line length: **120 characters** - Standard Android Studio formatter with EditorConfig. - Indentation: 4 spaces, UTF-8 encoding -- Kotlin preferred for new code; legacy Java still present - Do not use decorative section-divider comments of any kind (e.g. `// ── Title ───`, `// ------`, `// ======`). - Every new file must end with exactly one empty trailing line (no more, no less). -- Do not add comments, documentation for every function you created instead make it self explanatory as much as possible. -- `ktlint_code_style = android_studio`; disabled ktlint rules: `import-ordering`, `no-consecutive-comments`; trailing commas disallowed - All new files must include an SPDX license header: ` SPDX-License-Identifier: GPL-3.0-or-later ` - Translations: only modify `values/strings.xml`; never the translated `values-*/strings.xml` files - Create models, states in different files instead of doing it one single file. - Do not use magic number. - Apply fail fast principle instead of using nested if-else statements. - Do not use multiple boolean flags to determine states instead use enums or sealed classes. -- Use modern Java for Java classes. Optionals, virtual threads, records, streams if necessary. +- When you must edit an existing Java class, use modern Java — Optionals, records, streams where they genuinely help. + This applies to edits inside files that are already Java; it is never a reason to create a new Java file. - Avoid hardcoded strings, colors, dimensions. Use resources. -- Run lint, spotbugsGplayDebug, detekt, spotlessKotlinCheck and fix findings inside the files that have been changed. +- Run `./gradlew lintFdroidDebug` and `./gradlew testFdroidDebugUnitTest`, and fix every finding inside the files you + changed. [//]: # (REUSE-IgnoreEnd)