Skip to content

Commit

Permalink
MAIN: Review comments
Browse files Browse the repository at this point in the history 
* Remove dangerous methods
* Add documentation for new methods that are staying
  • Loading branch information
@naterichman
naterichman committed Mar 12, 2024
1 parent 09514a2 commit 27a45ff
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 129 deletions.
4 changes: 0 additions & 4 deletions core/src/header.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,6 @@ impl<I, P> DataElement<I, P> {
&self.value
}

pub fn value_mut(&mut self) -> &mut Value<I, P> {
&mut self.value
}

/// Move the data value out of the element, discarding the rest. If the
/// value is a sequence, its lifetime may still be bound to its original
/// source.
Expand Down
189 changes: 64 additions & 125 deletions object/src/mem.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,11 @@ where
self.entries.get(&tag)
}

pub fn get_mut(&mut self, tag: Tag) -> Option<&mut InMemElement<D>> {
// Get a mutable reference to a particular DICOM attribute from this object by tag.
//
// Should be private as it would allow a user to change the tag of an
// element and diverge from the dictionary
fn get_mut(&mut self, tag: Tag) -> Option<&mut InMemElement<D>> {
self.entries.get_mut(&tag)
}

Expand Down Expand Up @@ -805,7 +809,7 @@ where
self.len = Length::UNDEFINED;
}

/// Obtain a temporary mutable reference to a DICOM value,
/// Obtain a temporary mutable reference to a DICOM value by tag,
/// so that mutations can be applied within.
///
/// If found, this method resets all related lengths recorded
Expand Down Expand Up @@ -846,6 +850,55 @@ where
}
}

/// Obtain a temporary mutable reference to a DICOM value by AttributeSelector,
/// so that mutations can be applied within.
///
/// If found, this method resets all related lengths recorded
/// and returns `true`.
/// Returns `false` otherwise.
///
/// See the documentation of [`AttributeSelector`] for more information
/// on how to write attribute selectors.
///
/// NOTE: Also consider using the [`apply`].
///
/// # Example
///
/// ```
/// # use dicom_core::{DataElement, VR, dicom_value, value::DataSetSequence};
/// # use dicom_dictionary_std::tags;
/// # use dicom_object::InMemDicomObject;
/// # use dicom_core::ops::{AttributeAction, AttributeOp, ApplyOp};
/// let mut dcm = InMemDicomObject::from_element_iter([
/// DataElement::new(
/// tags::OTHER_PATIENT_I_DS_SEQUENCE,
/// VR::SQ,
/// DataSetSequence::from(vec![InMemDicomObject::from_element_iter([
/// DataElement::new(
/// tags::PATIENT_ID,
/// VR::LO,
/// dicom_value!(Str, "1234")
/// )])
/// ])
/// ),
/// ]);
/// let selector = (
/// tags::OTHER_PATIENT_I_DS_SEQUENCE,
/// 0,
/// tags::PATIENT_ID
/// );
///
/// // update referenced SOP instance UID for deidentification potentially
/// dcm.update_value_at(*&selector, |e| {
/// let mut v = e.primitive_mut().unwrap();
/// *v = dicom_value!(Str, "abcd");
/// });
///
/// assert_eq!(
/// dcm.entry_at(*&selector).unwrap().value().to_str().unwrap(),
/// "abcd"
/// );
/// ```
pub fn update_value_at(
&mut self,
selector: impl Into<AttributeSelector>,
Expand All @@ -860,80 +913,9 @@ where
})
}

/// Obtain the DICOM value by finding the element
/// that matches the given selector.
///
/// Returns an error if the respective element or any of its parents
/// cannot be found.
///
/// See the documentation of [`AttributeSelector`] for more information
/// on how to write attribute selectors.
///
/// # Example
///
/// ```no_run
/// # use dicom_core::prelude::*;
/// # use dicom_core::ops::AttributeSelector;
/// # use dicom_dictionary_std::tags;
/// # use dicom_object::InMemDicomObject;
/// # let obj: InMemDicomObject = unimplemented!();
/// let referenced_sop_instance_iod = obj.value_at(
/// (
/// tags::SHARED_FUNCTIONAL_GROUPS_SEQUENCE,
/// tags::REFERENCED_IMAGE_SEQUENCE,
/// tags::REFERENCED_SOP_INSTANCE_UID,
/// ))?
/// .to_str()?;
/// # Ok::<_, Box<dyn std::error::Error>>(())
/// ```
pub fn value_at(
&self,
selector: impl Into<AttributeSelector>,
) -> Result<&Value<InMemDicomObject<D>, InMemFragment>, AtAccessError> {
let selector = selector.into();

let mut obj = self;
for (i, step) in selector.iter().enumerate() {
match step {
// reached the leaf
AttributeSelectorStep::Tag(tag) => {
return obj.get(*tag).map(|e| e.value()).with_context(|| {
MissingLeafElementSnafu {
selector: selector.clone(),
}
})
}
// navigate further down
AttributeSelectorStep::Nested { tag, item } => {
let e = obj
.entries
.get(&tag)
.with_context(|| crate::MissingSequenceSnafu {
selector: selector.clone(),
step_index: i as u32,
})?;

// get items
let items = e.items().with_context(|| NotASequenceSnafu {
selector: selector.clone(),
step_index: i as u32,
})?;

// if item.length == i and action is a constructive action, append new item
obj =
items
.get(*item as usize)
.with_context(|| crate::MissingSequenceSnafu {
selector: selector.clone(),
step_index: i as u32,
})?;
}
}
}

unreachable!()
}

/// Get a DataElement by AttributeSelector
///
/// If the element or other intermediate elements do not exist, the method will return an error.
pub fn entry_at(
&self,
selector: impl Into<AttributeSelector>,
Expand Down Expand Up @@ -982,7 +964,10 @@ where
unreachable!()
}

pub fn entry_at_mut(
// Get a mutable reference to a particular entry by AttributeSelector
//
// Should be private for the same reason as `self.get_mut`
fn entry_at_mut(
&mut self,
selector: impl Into<AttributeSelector>,
) -> Result<&mut InMemElement<D>, AtAccessError> {
Expand Down Expand Up @@ -1030,55 +1015,9 @@ where
unreachable!()
}

pub fn value_at_mut(
&mut self,
selector: impl Into<AttributeSelector>,
) -> Result<&mut Value<InMemDicomObject<D>, InMemFragment>, AtAccessError> {
let selector = selector.into();

let mut obj = self;
for (i, step) in selector.iter().enumerate() {
match step {
// reached the leaf
AttributeSelectorStep::Tag(tag) => {
return obj.get_mut(*tag).map(|e| e.value_mut()).with_context(|| {
MissingLeafElementSnafu {
selector: selector.clone(),
}
})
}
// navigate further down
AttributeSelectorStep::Nested { tag, item } => {
let e = obj
.entries
.get_mut(&tag)
.with_context(|| crate::MissingSequenceSnafu {
selector: selector.clone(),
step_index: i as u32,
})?;

// get items
let items = e.items_mut().with_context(|| NotASequenceSnafu {
selector: selector.clone(),
step_index: i as u32,
})?;

// if item.length == i and action is a constructive action, append new item
obj =
items
.get_mut(*item as usize)
.with_context(|| crate::MissingSequenceSnafu {
selector: selector.clone(),
step_index: i as u32,
})?;
}
}
}

unreachable!()
}

/// Apply the given attribute operation on this object.
///
/// For a more free-form update, see [`update_value_at`].
///
/// See the [`dicom_core::ops`] module
/// for more information.
Expand Down

0 comments on commit 27a45ff

Please sign in to comment.