Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: properly handle multiple charsets following DICOM standard #4698

Closed

Conversation

devin-ai-integration[bot]
Copy link
Contributor

Fix multiple character sets handling in dicom-json-generator

Description

This PR fixes issue #4695 by implementing proper character set handling in dicom-json-generator.js according to the DICOM standard. When multiple character sets are present in a DICOM file, the script now properly detects and handles them following ISO/IEC 2022 Code Extension techniques.

Changes

  • Added proper character set detection following DICOM standard
  • Implemented ISO/IEC 2022 character set handling
  • Added comprehensive error handling for DICOM header parsing
  • Improved logging for character set detection
  • Handles UTF-8 (ISO_IR 192) exclusively when present
  • Falls back to ISO 2022 character set transitions when needed

Implementation Details

  • Properly reads SpecificCharacterSet (0008,0005) from DICOM header
  • Validates DICOM file structure (preamble, magic number)
  • Handles edge cases (invalid headers, missing character sets)
  • Provides detailed warning messages for troubleshooting
  • No silent error suppression - proper handling instead

Testing

Note: Build and unit tests pass successfully. Awaiting test data with multiple character sets (ISO_IR 192 and ISO 2022 IR 100) for full verification.

Link to Devin run: https://app.devin.ai/sessions/92befc3d9ab945eea8cf5f8d30359353

devin-ai-integration bot and others added 2 commits January 16, 2025 18:54
- Add ignoreErrors option to DicomMessage.readFile to handle multiple character sets
- Script will now continue processing with first character set when multiple are present
- Addresses issue #4695

Co-Authored-By: Alireza Sedghi <[email protected]>
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add "(aside)" to your comment to have me ignore it.
  • Look at CI failures and help fix them

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link

netlify bot commented Jan 16, 2025

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit f836f30
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/678958b7b3397b00080a33bf
😎 Deploy Preview https://deploy-preview-4698--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 16, 2025

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit f836f30
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/678958b735128e000824bfbf
😎 Deploy Preview https://deploy-preview-4698--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cypress bot commented Jan 16, 2025

Viewers    Run #4714

Run Properties:  status check passed Passed #4714  •  git commit f836f30d87: refactor: improve character set detection error handling
Project Viewers
Branch Review devin/1737054386-fix-multiple-charsets
Run status status check passed Passed #4714
Run duration 02m 10s
Commit git commit f836f30d87: refactor: improve character set detection error handling
Committer Devin AI
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 44
View all changes introduced in this branch ↗︎

@sedghi
Copy link
Member

sedghi commented Jan 16, 2025

Not a good solution

@sedghi sedghi closed this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant