dcmjs icon indicating copy to clipboard operation
dcmjs copied to clipboard

454 refactored Encoding/Decoding logic to expose DICOM encoding mappings for consumption

Open luissantosHCIT opened this issue 4 months ago • 4 comments

Fixes #454.

Although, I did not find any bugs with dcmjs, some of the tooling in the library could be better exposed for consumption in other projects. I focus on exposing the DICOM buffer decoding here given an initial SpecificCharacterSet. If translation is not necessary, a few convenience methods are provided to set the correct encoder or decoder on demand.

This is a code cleanup / reorganization kind of PR.

Jest tests passed. I did not add any new tests since there is coverage from the current tests and the one class I added is trivial.

luissantosHCIT avatar Aug 19 '25 14:08 luissantosHCIT

Deploy Preview for dcmjs2 ready!

Name Link
Latest commit 50327d2ded87238d900c9593b88bdd5824483344
Latest deploy log https://app.netlify.com/projects/dcmjs2/deploys/68a4873833dd5100087c47e8
Deploy Preview https://deploy-preview-455--dcmjs2.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 project configuration.

netlify[bot] avatar Aug 19 '25 14:08 netlify[bot]

@pieper I went ahead and did some cleanup based on your comments in #454. In terms of the other issue (#373), I do not do anything here other than change the location of the logic in question which I partially move lower in the abstraction hierarchy. However, if I get some time in the near future and no one has addressed it, I might take a stab. I think that the reasonable approach is to find the specific character set at the level of the tree in the header and use that for that level only. That can get a bit complex than I have time at the moment and I will need to consult the standard for other potential cases.

luissantosHCIT avatar Aug 19 '25 14:08 luissantosHCIT

From a quick look I think the PR looks good 👍

I'd like another set of eyes on it though, so hopefully @wayfarer3130 can take a look too.

pieper avatar Aug 19 '25 19:08 pieper

@wayfarer3130 checking in if there is anything you think I should improve on in this PR. No rush, I know you have been very helpful in other PRs and very busy. I am just helping keep this PR in people's minds in case there is a chance to merge. Thank you!

luissantosHCIT avatar Sep 04 '25 17:09 luissantosHCIT