ImagingDataCommons / libdicom

C library for reading DICOM files
https://libdicom.readthedocs.io
MIT License
15 stars 7 forks source link

Various small cleanups and bugfixes #44

Closed jcupitt closed 1 year ago

jcupitt commented 1 year ago

A few smallish things I found while getting the openslide connnector working.

jcupitt commented 1 year ago

Sorry, the history on this is a bit of a mess.

Summary:

  1. I needed to be able to get tags from keywords, so I added another hash. I named the keyword <-> tag functions as :c:func:dcm_dict_tag_from_keyword() and :c:func:dcm_dict_keyword_from_tag().
  2. I thought the _lookup names were a little unclear (it's not obvious which direction they are going) so I renamed the other similar functions for consistency.
  3. We had the function dcm_filehandle_read_filehandle_meta(), left over from when we renamed file as filehandle. I thought this was probably an error and it should be dcm_filehandle_read_file_meta().
  4. We used _meta() in some places and _metadata() in others, which seemed inconsistent, so I switched to _metadata() everywhere.
  5. I found an annoying bug in dataset_insert.
hackermd commented 1 year ago

@jcupitt One other thing that I just noted, which is not directly related to this PR: The _DcmVR enum not only contains individual VRs, but also options such as "OB or OW" (see here). Strictly speaking, those are not VRs and, in my opinion, therefore don't belong into the enum for the type definition. Can we handle those options differently?

jcupitt commented 1 year ago

I split the DcmVR enum into two, a public and a private one, and put the combination VRs into the private version.

There are two new things:

bool dcm_is_valid_vr_for_tag(DcmVR vr, uint32_t tag);
DcmVR dcm_vr_from_tag(uint32_t tag);

That can quickly check if a VR can be used for a known tag, and for implicit mode, a thing that can get the VR for a tag.

The new arrangement should also make it easy to add new VRs and VR combinations without breaking ABI.

jcupitt commented 1 year ago

Could someone merge this? I don't have permission. Once this merges, I have a few more small things that take it up to v0.2, the one that openslide builds and tests against.

bgilbert commented 1 year ago

Looks like tests haven't run. A re-push might fix it, but I think it can also happen if GitHub fails to create a merge commit to run tests against.

jcupitt commented 1 year ago

Oh, that's odd, it was showing all all complete for me yesterday. I'll push again.