diff --git a/.github/skills/pr-review/SKILL.md b/.github/skills/pr-review/SKILL.md index a80516ce..c51ab5e4 100644 --- a/.github/skills/pr-review/SKILL.md +++ b/.github/skills/pr-review/SKILL.md @@ -56,13 +56,17 @@ Before acting on any review comment or suggestion, classify it by importance: |-------|----------|--------| | **P1 — Must Fix** | Bug, security issue, broken behavior, API contract violation, CI failure | Fix immediately | | **P2 — Should Fix** | Correctness risk, meaningful maintainability improvement, clear code smell with real impact | Fix with brief justification | -| **P3 — Optional** | Style preference, minor naming nitpick, debatable design choice, "could be cleaner" | **Do NOT auto-fix** — surface to user for decision | +| **P3 — Conditional** | Style preference, minor naming, "could be cleaner" | Fix if it aligns with best practices **and** the change is safe + trivial; defer if complex or has any potential functional impact | | **P4 — Reject** | Contradicts project conventions, introduces unnecessary complexity, or is factually wrong | Reject with explanation | **Rules:** -- **Do not implement P3 suggestions automatically.** Note them in the final summary and let the user decide. +- For **P3**, apply this two-question test before touching the code: + 1. **Best practice?** — Does the change follow language/framework conventions (e.g. prefer imports over FQNs, use existing imports, standard patterns)? + 2. **Safe & trivial?** — Is the diff mechanical with zero risk of behavioral change and low effort? + - Both **yes** → fix it silently, mark thread resolved. + - Either **no** → do NOT modify code; record in the summary table and let the user decide. - **Do not add comments to code** unless the comment explains non-obvious logic that is truly necessary. Never add comments just to acknowledge a review suggestion was applied. -- When in doubt about importance, prefer the lower severity (P3/P4) and defer to the user rather than making the change. +- When the two-question test is ambiguous, prefer deferring rather than guessing. ## Procedure @@ -80,7 +84,7 @@ Before acting on any review comment or suggestion, classify it by importance: 3. **Address review comments** — apply the Review Restraint Policy to each comment: - P1/P2: implement the fix, then **immediately resolve the thread** using the GraphQL mutation above - Outdated threads: resolve them regardless of priority (no action needed, just mark resolved) - - P3: record in summary, do **not** modify code, ask user + - P3: apply the two-question test — fix + resolve if both answers are yes; otherwise record in summary and defer to user - P4: record rejection reason in summary 4. **Commit & push** — single commit covering all non-workflow fixes (workflow fixes are pushed incrementally during step 2) 5. **Final verification** — once all fixes are applied, confirm every check is green: `GH_PAGER= gh pr checks ` @@ -94,8 +98,9 @@ Before acting on any review comment or suggestion, classify it by importance: ```markdown | # | Source (comment / CI) | Issue description | Priority | Action taken | Reason if not fixed | |---|-----------------------|-------------------|----------|--------------|---------------------| -| 1 | Reviewer @xxx | e.g. rename foo() | P3 | Not fixed | Style preference, no functional impact — deferred to user | -| 2 | CI: lint | e.g. null-check | P1 | Fixed | — | +| 1 | Reviewer @xxx | use import instead of FQN | P3 | Fixed | Best practice + trivial mechanical change | +| 2 | Reviewer @xxx | rename internal var foo→bar | P3 | Not fixed | Non-standard opinion, no best-practice backing — deferred to user | +| 3 | CI: lint | null-check missing | P1 | Fixed | — | ``` All P3/P4 items that were **not** fixed must appear in this table with a clear reason, so the user can make an informed decision. diff --git a/library/src/main/java/org/wysaid/camera/Camera1Provider.java b/library/src/main/java/org/wysaid/camera/Camera1Provider.java index 32dfb15e..386f859c 100644 --- a/library/src/main/java/org/wysaid/camera/Camera1Provider.java +++ b/library/src/main/java/org/wysaid/camera/Camera1Provider.java @@ -4,6 +4,8 @@ import android.hardware.Camera; import android.util.Log; +import java.util.List; + import org.wysaid.common.Common; /** @@ -103,7 +105,7 @@ public void focusAtPoint(float x, float y, float radius, AutoFocusCallback callb @Override public boolean setFlashMode(FlashMode mode) { - Camera.Parameters params = CameraInstance.getInstance().getParams(); + Camera.Parameters params = getParams(); if (params == null) return false; String camera1Mode = ICameraProvider.flashModeToCamera1(mode); @@ -124,7 +126,7 @@ public boolean setFlashMode(FlashMode mode) { @Override public FlashMode getFlashMode() { - Camera.Parameters params = CameraInstance.getInstance().getParams(); + Camera.Parameters params = getParams(); if (params == null) return null; return ICameraProvider.camera1ToFlashMode(params.getFlashMode()); } @@ -139,6 +141,80 @@ public void setFocusMode(String focusMode) { CameraInstance.getInstance().setFocusMode(focusMode); } + // ========== Private helpers ========== + + /** + * Shorthand for {@link CameraInstance#getParams()}. + * Returns {@code null} when the camera is closed. + */ + private Camera.Parameters getParams() { + return CameraInstance.getInstance().getParams(); + } + + /** + * Returns camera parameters only when the camera is open and zoom is supported; + * returns {@code null} otherwise. Use this as the single guard in all zoom methods. + */ + private Camera.Parameters getZoomParams() { + Camera.Parameters params = getParams(); + return (params != null && params.isZoomSupported()) ? params : null; + } + + // ========== Zoom ========== + + @Override + public boolean isZoomSupported() { + return getZoomParams() != null; + } + + @Override + public float getMinZoomRatio() { + return 1.0f; // Camera1 always starts at 1.0x (zoom index 0) + } + + @Override + public float getMaxZoomRatio() { + Camera.Parameters params = getZoomParams(); + if (params == null) return 1.0f; + List ratios = params.getZoomRatios(); + if (ratios == null || ratios.isEmpty()) return 1.0f; + int maxIndex = Math.min(params.getMaxZoom(), ratios.size() - 1); + return ratios.get(maxIndex) / 100.0f; + } + + @Override + public void setZoomRatio(float ratio) { + Camera.Parameters params = getZoomParams(); + if (params == null) return; + + if (!Float.isFinite(ratio) || ratio <= 0f) { + Log.e(LOG_TAG, "Camera1: invalid zoom ratio: " + ratio); + return; + } + + List zoomRatios = params.getZoomRatios(); + if (zoomRatios == null || zoomRatios.isEmpty()) return; + int targetHundredths = Math.round(ratio * 100.0f); + + // Find the closest discrete zoom index for the requested ratio. + int bestIndex = 0; + long bestDiff = Long.MAX_VALUE; + for (int i = 0; i < zoomRatios.size(); i++) { + long diff = Math.abs((long) zoomRatios.get(i) - targetHundredths); + if (diff < bestDiff) { + bestDiff = diff; + bestIndex = i; + } + } + + try { + params.setZoom(bestIndex); + CameraInstance.getInstance().setParams(params); + } catch (Exception e) { + Log.e(LOG_TAG, "Camera1: setZoomRatio failed: " + e.toString()); + } + } + // ========== Capture ========== /** @@ -190,7 +266,7 @@ public void takePicture(ShutterCallback shutterCallback, PictureDataCallback pic int jpegRotation = computeJpegRotation(camera1Facing); // Set rotation - Camera.Parameters params = CameraInstance.getInstance().getParams(); + Camera.Parameters params = getParams(); if (params != null) { try { params.setRotation(jpegRotation); diff --git a/library/src/main/java/org/wysaid/camera/CameraXProvider.java b/library/src/main/java/org/wysaid/camera/CameraXProvider.java index 9a833227..e197659a 100644 --- a/library/src/main/java/org/wysaid/camera/CameraXProvider.java +++ b/library/src/main/java/org/wysaid/camera/CameraXProvider.java @@ -10,6 +10,8 @@ import android.util.Size; import android.view.Surface; +import androidx.camera.core.ZoomState; + import androidx.camera.camera2.interop.Camera2Interop; import androidx.camera.camera2.interop.ExperimentalCamera2Interop; @@ -340,6 +342,67 @@ public void setFocusMode(String focusMode) { Log.i(LOG_TAG, "CameraX: setFocusMode ignored (CameraX manages focus automatically): " + focusMode); } + // ========== Zoom ========== + + @Override + public boolean isZoomSupported() { + if (mCamera == null) return false; + ZoomState state = mCamera.getCameraInfo().getZoomState().getValue(); + return state != null && state.getMaxZoomRatio() > state.getMinZoomRatio(); + } + + @Override + public float getMinZoomRatio() { + if (mCamera == null) return 1.0f; + ZoomState state = mCamera.getCameraInfo().getZoomState().getValue(); + return state != null ? state.getMinZoomRatio() : 1.0f; + } + + @Override + public float getMaxZoomRatio() { + if (mCamera == null) return 1.0f; + ZoomState state = mCamera.getCameraInfo().getZoomState().getValue(); + return state != null ? state.getMaxZoomRatio() : 1.0f; + } + + @Override + public void setZoomRatio(float ratio) { + if (mCamera == null) { + Log.w(LOG_TAG, "CameraX: setZoomRatio called but camera not bound."); + return; + } + + ZoomState zoomState = mCamera.getCameraInfo().getZoomState().getValue(); + if (zoomState == null) { + Log.w(LOG_TAG, "CameraX: setZoomRatio called but ZoomState not yet available."); + return; + } + + float minZoom = zoomState.getMinZoomRatio(); + float maxZoom = zoomState.getMaxZoomRatio(); + float clampedRatio = Math.max(minZoom, Math.min(ratio, maxZoom)); + + if (clampedRatio != ratio) { + Log.w(LOG_TAG, "CameraX: Requested zoom ratio " + ratio + + " is out of range [" + minZoom + ", " + maxZoom + "], clamped to " + + clampedRatio + "."); + } + + final ListenableFuture future = + mCamera.getCameraControl().setZoomRatio(clampedRatio); + future.addListener(() -> { + try { + future.get(); + } catch (ExecutionException e) { + Throwable cause = e.getCause(); + Log.e(LOG_TAG, "CameraX: setZoomRatio failed: " + (cause != null ? cause : e)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + Log.e(LOG_TAG, "CameraX: setZoomRatio interrupted.", e); + } + }, ContextCompat.getMainExecutor(mContext)); + } + // ========== Capture ========== @Override diff --git a/library/src/main/java/org/wysaid/camera/ICameraProvider.java b/library/src/main/java/org/wysaid/camera/ICameraProvider.java index 7b772bad..0cd26337 100644 --- a/library/src/main/java/org/wysaid/camera/ICameraProvider.java +++ b/library/src/main/java/org/wysaid/camera/ICameraProvider.java @@ -205,6 +205,56 @@ default void startPreview(SurfaceTexture texture, PreviewSizeReadyCallback callb */ void setFocusMode(String focusMode); + // ========== Zoom ========== + + /** + * Whether the camera hardware supports zoom. + * + *

CameraX always returns {@code true} (handled by the framework). + * Camera1 returns the result of {@link android.hardware.Camera.Parameters#isZoomSupported()}. + * + * @return {@code true} if zoom is supported, {@code false} otherwise. + */ + default boolean isZoomSupported() { + return false; + } + + /** + * Get the minimum zoom ratio supported by the camera hardware. + * Always 1.0 (no zoom, native field of view). + * + * @return minimum zoom ratio, typically 1.0f. + */ + default float getMinZoomRatio() { + return 1.0f; + } + + /** + * Get the maximum zoom ratio supported by the camera hardware. + * + * @return maximum zoom ratio (e.g. 10.0f); 1.0f when zoom is unsupported. + */ + default float getMaxZoomRatio() { + return 1.0f; + } + + /** + * Set the zoom ratio. + * + *

CameraX applies the ratio asynchronously via {@code CameraControl.setZoomRatio()}. + * Camera1 maps the float ratio to the closest supported integer zoom index + * ({@link android.hardware.Camera.Parameters#setZoom(int)}) — slight rounding may occur + * because Camera1 only exposes discrete steps. + * + *

Values are clamped to [{@link #getMinZoomRatio()}, {@link #getMaxZoomRatio()}]. + * Calling this method when {@link #isZoomSupported()} returns {@code false} is a no-op. + * + * @param ratio The desired zoom ratio. 1.0f means no zoom. + */ + default void setZoomRatio(float ratio) { + // Default no-op; implementations override. + } + // ========== Capture ========== /**