Page MenuHomePhabricator

PropertyServices should also handle "property pathes" with wildcards
Closed, ResolvedPublic

Description

There are use cases where it is needed to be abel to specifiy Properties in properties services with wildcards to address a whole set of (potentially yet unkown) properties.

One concrete use case is the handling of DICOM information as properties. I case of sequence elements we will have several properties (prop[0], prop[1], prop[n]) that should all be adressed (e.g. prop[*]).

Todo:

  1. standardize the syntax of sequenze adressing and implement helper classes to generate, parse and verify valid property names that regard this syntax.
  1. extend the PropertyServices to also support this wildcarded property names.

Proposal for the syntax would be close to DCMTK tag pathes and to the visualization style of the workbench property view.

Names/pathsegments are seperated by ".". If a segment has multiple values (is a sequence of items), the items must be selected/differentiated by an index/array selector in square bracket "[n]". The segment name can be wild carded with "*" as well as the index selector.

Examples:
"propA" will select the normal property propA.

"propB[1]" will select the first item of propB. PropB is assumed to be a sequence of items.

"*.propC" will select every property propC, no matter what comes before.

"propA.propD[*]" will select every item of property propD that is "child" of propA.

Revisions and Commits

Event Timeline

User floca has pushed new remote branch:

bug-19791-PropertyServices_with_wildcards

Chose a more generic approach for the core property services. Wildcards can now be expressed by regulare expressions. This gives even more freedom in the configuration of the services.

The current fix extends just the PropertyPersistence and the PropertyDescription service to also support regular expressions. (Reason: These to are currently needed to support the feature). For other Services the extension and implementation of PropertyDescription service can be the role model.

The "path" stuff (Todos) of the innitial bug comment is currently only needed in the DICOM context. So it is moved to another bug and is handled there/in the DICOMReader module. Pros: Keeps the core slimmer, core implementation is even more generic now (reg exs). DICOM specific implementation Details do not spill over into the core, but can arbitrary reused by other moduls, if needed.

General remarks

Wow, hard to review/swallow at once so I put some trust in you and your tests to be honest. :-) Do you have some points that you're not sure/confident about?

Did you already let different nightlies build your branch? - Can imagine that there might be a compiler warning here and there in the Core.

Is the property view still working as expected as you changed property services which the view is using?

Typos

  • mitkIPropertyPersistence.h
    • is already a property info instances
    • overrite
    • overweite
  • mitkPropertyPersistenceInfo.h
    • not another info with a more specific mime type is available -> another info ... isn't available
  • mitkItkImageIO.cpp
    • Check of there is

Important stuff

  • You return const functional types here and there like "const DeserializationFunctionType". I can remember a lot of trouble with CppMicroservices and compiler warnings, that "const functional types" wouldn't make sense or something like that. So I guess they shouldn't be const. However, to confused at the moment with other stuff to really think about it. :-)
  • Copyright notice for CreateJSONEscapes() if it was copied from somewhere else?

Considerations

  • Infos -> Info?
  • std::unordered_multimap instead of std::multimap, or do we profit from the ordering?
  • I see the point in the template aliases for more complex types but personally I would prefer the direct type in the following example as it is often more clear for the client of what he can expect as return value: using MimeTypeNameType = std::string
  • Why the extra namespaces for the serialization stuff like "namespace PropertyPersistenceSerialization" and so on? - Perfectly fine to have these functions in the mitk namespace. On the other hand probably longer function names to keep all the functions separate from each other. So maybe just stick with namespaces I guess?
  • Split inline lambdas to multiple lines instead of single line monsters

First of all. Thanks for crawling through the patch!

Do you have some points that you're not sure/confident about?

Nope. I am fine. Everything that was sure we had already discussed at our hack fest beforehand.

Did you already let different nightlies build your branch?

Not yet. I waited till I have the human review, because after that, I have to go through the nightlies anyway.

Is the property view still working as expected as you changed property services which the view is using?

Yes. I tested it. And at last all features I know of, seem to work fine.

Typos.

Thanks. Fixed.

Important stuff

Copyright notice for CreateJSONEscapes()

It is based on code from boost property tree. Regarding the boost license, formaly there is no need for a copy right notice. But I think adding a comment on that is a good idea.

Considerations

  • Infos -> Info?

Fixed.

  • std::unordered_multimap instead of std::multimap

In context of PropertyPersistence::InfoMap I indeed want to keept the ordering. ;)

[...] I would prefer the direct type in the following example as it is often more clear for the client of what he can expect as return value: using MimeTypeNameType = std::string

Here I disagree (But it is a matter of taste and maybe I am so template adapted that it does not annoy me enough). I would keep it because I want to emphasize the dependency on the type defined by PeropertyPersistenceInfo
PropertyPersistenceInfo is already a strong dependency for the IPropertyPersistence class interface. So we would gain no benefit in terms of class indepence from decoupling this type only.

Why the extra namespaces ...

I am indifferent too; same reasoning. I would keep it like it currently is. If there is a clear design descision at some point, we can refactor it.

Split inline lambdas to multiple lines instead of single line monsters

Done.

kislinsk triaged this task as Wishlist priority.Aug 9 2016, 9:37 AM
floca added a revision: Restricted Differential Revision.Sep 7 2016, 6:17 PM
floca raised the priority of this task from Wishlist to Normal.Sep 18 2016, 11:02 PM
floca added a project: Restricted Project.

Thanks for the hint. Documentation ist fixed.

floca moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Oct 19 2016, 10:27 AM