Fix condition for ISO-8859-1 encoding selection in GetTargetEncoding method

[Shane32]

Summary by CodeRabbit

• Bug Fixes
  • QR code encoding now correctly respects the selected ECI mode.
  • ISO-8859-1 fallback is applied only when ECI mode is set to Default, preventing unintended overrides with non-default modes.
  • Ensures consistent behavior across UTF-8, BOM, and other encodings when a specific ECI mode is chosen.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adjusts the encoding selection logic in ByteDataSegment: the fast path to ISO-8859-1 is now taken only when ECI mode is Default, not forced UTF-8, and the text is ISO-valid. Non-default ECI modes bypass this path and proceed through the switch-based handling.

Changes

Cohort / File(s)	Summary
ECI handling in ByteDataSegment QRCoder/QRCodeGenerator/ByteDataSegment.cs	Tightened condition for ISO-8859-1 fallback to require EciMode.Default; control flow for non-default ECI modes now continues into switch-case handling instead of early ISO path.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant ByteDataSegment

  Caller->>ByteDataSegment: Create byte data segment (plainText, eciMode, forceUtf8)
  alt eciMode == Default AND !forceUtf8 AND IsValidISO(plainText)
    Note right of ByteDataSegment: Use ISO-8859-1 path (unchanged)
    ByteDataSegment-->>Caller: Encoded bytes (ISO-8859-1)
  else Non-default ECI or forced UTF-8 or invalid ISO
    Note right of ByteDataSegment: Proceed to ECI-aware switch-case
    ByteDataSegment->>ByteDataSegment: switch (eciMode) { Utf8, Utf8BOM, ... }
    ByteDataSegment-->>Caller: Encoded bytes per selected ECI handling
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Eliminates exceptions when characters are outside ISO-8859-1 range #672 — Modifies IsValidISO implementation affecting the same ISO-8859-1 decision path, directly related to the updated conditional logic.

Suggested reviewers

• gfoidl

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title accurately summarizes the primary change by indicating a fix to the ISO-8859-1 encoding selection logic in the GetTargetEncoding method. It references the specific method and nature of the update, providing enough context for reviewers to understand the focus of the pull request. The phrasing is concise and clear without extraneous details or ambiguous terminology.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix_encoding

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

QRCoder/QRCodeGenerator/ByteDataSegment.cs (3)

124-125: Update comment to reflect ECI mode check.

The comment should mention that the ECI mode must also be Default for the ISO-8859-1 fast path.

Apply this diff:

-        // Check if the text is valid ISO-8859-1 and UTF-8 is not forced, then encode using ISO-8859-1.
+        // Check if ECI mode is Default, the text is valid ISO-8859-1, and UTF-8 is not forced, then encode using ISO-8859-1.
         if (eciMode == EciMode.Default && !forceUtf8 && IsValidISO(plainText))

---

186-190: Update remarks to reflect ECI mode handling.

The remarks are outdated. They should clarify that encoding selection also depends on the ECI mode. When an explicit non-default ECI mode is specified (e.g., EciMode.Utf8), that encoding is used regardless of ISO-8859-1 validity.

Apply this diff:

     /// <remarks>
-    /// The returned text is always encoded as ISO-8859-1 unless either the text contains a non-ISO-8859-1 character or
-    /// UTF-8 encoding is forced. This does not meet the QR Code standard, which requires the use of ECI to specify the encoding
-    /// when not ISO-8859-1.
+    /// The returned text is encoded as ISO-8859-1 when the ECI mode is Default, UTF-8 is not forced, and the text
+    /// contains only valid ISO-8859-1 characters. Otherwise, the encoding is determined by the ECI mode or defaults to UTF-8.
+    /// This does not meet the QR Code standard, which requires the use of ECI to specify the encoding when not ISO-8859-1.
     /// </remarks>

---

209-213: Update remarks to reflect ECI mode handling.

Same issue as lines 186-190. The remarks should clarify that encoding selection depends on the ECI mode.

Apply this diff:

     /// <remarks>
-    /// The returned text is always encoded as ISO-8859-1 unless either the text contains a non-ISO-8859-1 character or
-    /// UTF-8 encoding is forced. This does not meet the QR Code standard, which requires the use of ECI to specify the encoding
-    /// when not ISO-8859-1.
+    /// The returned text is encoded as ISO-8859-1 when the ECI mode is Default, UTF-8 is not forced, and the text
+    /// contains only valid ISO-8859-1 characters. Otherwise, the encoding is determined by the ECI mode or defaults to UTF-8.
+    /// This does not meet the QR Code standard, which requires the use of ECI to specify the encoding when not ISO-8859-1.
     /// </remarks>

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6cd987c and d315888.

📒 Files selected for processing (1)

• QRCoder/QRCodeGenerator/ByteDataSegment.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

QRCoder/QRCodeGenerator/ByteDataSegment.cs (1)

QRCoder/QRCodeGenerator.cs (1)

• IsValidISO (973-985)

🔇 Additional comments (1)

QRCoder/QRCodeGenerator/ByteDataSegment.cs (1)

125-125: LGTM! Correctly restricts ISO-8859-1 fast path to Default ECI mode.

This fix ensures that explicit ECI mode requests (e.g., EciMode.Utf8) are honored even when the text is valid ISO-8859-1. Previously, the method would incorrectly select ISO-8859-1 encoding when a user explicitly requested UTF-8 via EciMode.Utf8 but didn't set forceUtf8.

The condition ordering is also optimal: cheap enum and boolean checks before the more expensive IsValidISO string iteration.

[gfoidl]

Good bots 😄.