Pushed new branch T23757-IPropertyOwner.

I'd like to propose a few changes to the interface resp. ask a few questions. I implemented it like this currently:

class MITKCORE_EXPORT IPropertyOwner
{
public:
  using ContextType = std::string;
  using KeyType = std::string;

  virtual ~IPropertyOwner();

  /**
   * \brief Get property by its key.
   *
   * \param[in] key Property key/name.
   * \param[in] context Optional, default is empty string. Search in specific context first.
   * \param[in] strict Optional, default is false. Restrict search to specified context and ignore default context.
   *
   * \return Found property, nullptr otherwise.
   */
  virtual BaseProperty * GetProperty(const KeyType &key, const ContextType &context = "", bool strict = false) const = 0;

  /**
   * \brief Add new or change existent property.
   *
   * \param[in] key Property key/name.
   * \param[in] property Property/value.
   * \param[in] context Optional, default is empty string. Context in which the property is set.
   *
   */
  virtual void SetProperty(const KeyType &key, BaseProperty *property, const ContextType &context = "") = 0;
};

• Let the caller of GetProperty() decide if she or he really needs a smart pointer
• I removed the strict/defaultFallback parameter of SetProperty()
• Why the const restriction for the returned property in the original proposal?
• I named the context parameters just context instead of contextID. Maybe I missed the whole point of this concept, but is it really supposed to be an ID?
• I removed GetPropertyKeys(). It seems a little random to me to have this and only this method in the interface besides Get/SetProperty(). Is it really necessary?

In T23757#117790, @kislinsk wrote:

• Let the caller of GetProperty() decide if she or he really needs a smart pointer

The reason I did it, was to indicate that it is also a transfer of responsibility => The interface does not guarantee/imply that it well take care of the live time handling of the returned properties. Thus they also could be generated on demand on the fly!
So it is a design decision. If we allow just returning pointer. We have document it in the class description that PropertyOwner must keep ownership of returned properties. I tended to my version because it is hardcoded error avoidance and don't rely on developers reading the docs. ;)

• I removed the strict/defaultFallback parameter of SetProperty()

Meaning you would always handle it as true or as false? I could imagen scenarios where both is the case. DefaultFallback == false what mean, that you basically only want to replace a property in an context if it already exists. I admit that this is likely to be a rare case, but I thought, that it is not a big deal (increase of complexity) to also offer it.
What is your reasoning to remove it?

• Why the const restriction for the returned property in the original proposal?

More control *muhahahah* and constness. I think it is a better pattern to enforce the explicit (re)setting of properties, when you want to change its values. And you don't force classes to break an imutablity pattern, if they implement this interface (which we have also discussed in context of data nodes in our hackfest)!!! If classes want to break immutablity, one could introduce a non const interface version, or a class specific interface and one could cast to this and get the properties, if they should be non const.

• I named the context parameters just context instead of contextID. Maybe I missed the whole point of this concept, but is it really supposed to be an ID?

I named it ID. Because *tata* its just a name/id. I want to keep it open, may be we introduce a context concept that is more then just a name and then we would call that class context. So we could name the variable just context, but I would prefer to keep the typename with ID. I think sparing two letters is not enough benfit, compared to bluring the meaning.

• I removed GetPropertyKeys(). It seems a little random to me to have this and only this method in the interface besides Get/SetProperty(). Is it really necessary?

Yes, as long you don't habe a better strategy to deduce in a abstract fashion the existing properties of an owner instance (brute forcing all possible letter compination is not a good strategy ;). So it was not random, but intended. It is neede to have a generic way to query what Get could offer. E.g. for something like a property view.

In T23757#117792, @floca wrote:

In T23757#117790, @kislinsk wrote:

• Let the caller of GetProperty() decide if she or he really needs a smart pointer

The reason I did it, was to indicate that it is also a transfer of responsibility => The interface does not guarantee/imply that it well take care of the live time handling of the returned properties. Thus they also could be generated on demand on the fly!
So it is a design decision. If we allow just returning pointer. We have document it in the class description that PropertyOwner must keep ownership of returned properties. I tended to my version because it is hardcoded error avoidance and don't rely on developers reading the docs. ;)

I agree partly, as the ownership thing is way better expressed by a smart pointer, but this interface is called IPropertyOwner which strongly indicates that the properties are quaranteed to be owned by the concrete interface implementation. If we really want to give the freedom of creating properties on the fly that are not owned by IPropertyOwner, we should definitely rename the interface to something like IPropertyProvider and make clear in the documentation that it is not guaranteed that the properties are owned by anyone else than the caller of GetProperty().

• I removed the strict/defaultFallback parameter of SetProperty()

Meaning you would always handle it as true or as false? I could imagen scenarios where both is the case. DefaultFallback == false what mean, that you basically only want to replace a property in an context if it already exists. I admit that this is likely to be a rare case, but I thought, that it is not a big deal (increase of complexity) to also offer it.
What is your reasoning to remove it?

A strict parameter for setting a property confused me. The only reasonable purpose of a strict parameter is - in my opinion - to express something like "set the property in this context but if this context doesn't exist, it is fine to set it in the default context instead". If we express something on this level, we should also add abstract methods to the interface to check for the existence of contexts or to list all available contexts.

• Why the const restriction for the returned property in the original proposal?

More control *muhahahah* and constness. I think it is a better pattern to enforce the explicit (re)setting of properties, when you want to change its values. And you don't force classes to break an imutablity pattern, if they implement this interface (which we have also discussed in context of data nodes in our hackfest)!!! If classes want to break immutablity, one could introduce a non const interface version, or a class specific interface and one could cast to this and get the properties, if they should be non const.

That's the one I disagree because of lessons learned in the past. :-) While correct academically, I can imagine realistic cases, which would lead to const casting users, like, what if I want to to something else with the returned property? For example, I want to add an observer and stuff like this. Hence I think that we should be less strict here.

• I named the context parameters just context instead of contextID. Maybe I missed the whole point of this concept, but is it really supposed to be an ID?

I named it ID. Because *tata* its just a name/id. I want to keep it open, may be we introduce a context concept that is more then just a name and then we would call that class context. So we could name the variable just context, but I would prefer to keep the typename with ID. I think sparing two letters is not enough benfit, compared to bluring the meaning.

In this case I strongly vote for contextName as contextID is rather confusing in my opinion especially when introducing IDs to MITK like we do. It implies to me that there is something like context classes deriving from a base context which I could query by an ID.

• I removed GetPropertyKeys(). It seems a little random to me to have this and only this method in the interface besides Get/SetProperty(). Is it really necessary?

Yes, as long you don't habe a better strategy to deduce in a abstract fashion the existing properties of an owner instance (brute forcing all possible letter compination is not a good strategy ;). So it was not random, but intended. It is neede to have a generic way to query what Get could offer. E.g. for something like a property view.

OK. :-)

In summary, the interface would look like this:

class MITKCORE_EXPORT IPropertyProvider
{
public:
  virtual ~IPropertyProvider();

  /**
  * \brief Get property by its key.
  *
  * \param[in] propertyName Name of property.
  * \param[in] contextName Optional, default is empty string (default
  *            context). Search in specified context.
  * \param[in] fallBackOnDefaultContext Optional, default is false. Also
  *            search in default context if property was not found in given
  *            context.
  *
  * \return Found property, nullptr otherwise.
  */
  virtual BaseProperty::Pointer GetProperty(const std::string &propertyName, const std::string &contextName = "", bool fallBackOnDefaultContext = false) const = 0;

  /**
  * \brief Add new or change existent property.
  *
  * \param[in] propertyName Name of property.
  * \param[in] property The actual property.
  * \param[in] contextName Optional, default is empty string(default
               context). Context in which the property is set.
  * \param[in] fallBackOnDefaultContext Optional, default is false. Set
  *            property in default context if given context does not
  *            exist.
  */
  virtual void SetProperty(const std::string &propertyName, BaseProperty *property, const std::string &contextName = "", bool fallBackOnDefaultContext = false) = 0;

  /**
  * \brief Query names of existing properties.
  *
  * \return List of property names.
  */
  virtual std::vector<std::string> GetPropertyNames() const = 0;

  /**
  * \brief Query names of existing contexts.
  *
  * \return List of context names.
  */
  virtual std::vector<std::string> GetPropertyContextNames() const = 0;
};

In T23757#117795, @kislinsk wrote:

In T23757#117792, @floca wrote:

In T23757#117790, @kislinsk wrote:

• Let the caller of GetProperty() decide if she or he really needs a smart pointer

The reason I did it, was to indicate that it is also a transfer of responsibility => The interface does not guarantee/imply that it well take care of the live time handling of the returned properties. Thus they also could be generated on demand on the fly!
So it is a design decision. If we allow just returning pointer. We have document it in the class description that PropertyOwner must keep ownership of returned properties. I tended to my version because it is hardcoded error avoidance and don't rely on developers reading the docs. ;)

I agree partly, as the ownership thing is way better expressed by a smart pointer, but this interface is called IPropertyOwner which strongly indicates that the properties are quaranteed to be owned by the concrete interface implementation. If we really want to give the freedom of creating properties on the fly that are not owned by IPropertyOwner, we should definitely rename the interface to something like IPropertyProvider and make clear in the documentation that it is not guaranteed that the properties are owned by anyone else than the caller of GetProperty().

Faire enough. Good point. You are right. If you go this road we should change the name.

• I removed the strict/defaultFallback parameter of SetProperty()

Meaning you would always handle it as true or as false? I could imagen scenarios where both is the case. DefaultFallback == false what mean, that you basically only want to replace a property in an context if it already exists. I admit that this is likely to be a rare case, but I thought, that it is not a big deal (increase of complexity) to also offer it.
What is your reasoning to remove it?

A strict parameter for setting a property confused me. The only reasonable purpose of a strict parameter is - in my opinion - to express something like "set the property in this context but if this context doesn't exist, it is fine to set it in the default context instead". If we express something on this level, we should also add abstract methods to the interface to check for the existence of contexts or to list all available contexts.

Thats correct.

• Why the const restriction for the returned property in the original proposal?

More control *muhahahah* and constness. I think it is a better pattern to enforce the explicit (re)setting of properties, when you want to change its values. And you don't force classes to break an imutablity pattern, if they implement this interface (which we have also discussed in context of data nodes in our hackfest)!!! If classes want to break immutablity, one could introduce a non const interface version, or a class specific interface and one could cast to this and get the properties, if they should be non const.

That's the one I disagree because of lessons learned in the past. :-) While correct academically, I can imagine realistic cases, which would lead to const casting users, like, what if I want to to something else with the returned property? For example, I want to add an observer and stuff like this. Hence I think that we should be less strict here.

Still disagree. We should less strict in another interface. But not be force introduce non-const dependency, which we would practicly do. The case you mention is only valid if we stay with PropertyOwner. If we have a PropertyProvider it gets less important.

• I named the context parameters just context instead of contextID. Maybe I missed the whole point of this concept, but is it really supposed to be an ID?

I named it ID. Because *tata* its just a name/id. I want to keep it open, may be we introduce a context concept that is more then just a name and then we would call that class context. So we could name the variable just context, but I would prefer to keep the typename with ID. I think sparing two letters is not enough benfit, compared to bluring the meaning.

In this case I strongly vote for contextName as contextID is rather confusing in my opinion especially when introducing IDs to MITK like we do. It implies to me that there is something like context classes deriving from a base context which I could query by an ID.

We can use ContextName. I think it is a flavor or depending on the fact if you see ID as UID or just as a "name".
But I won't fight for that. ;)

• I removed GetPropertyKeys(). It seems a little random to me to have this and only this method in the interface besides Get/SetProperty(). Is it really necessary?

Yes, as long you don't habe a better strategy to deduce in a abstract fashion the existing properties of an owner instance (brute forcing all possible letter compination is not a good strategy ;). So it was not random, but intended. It is neede to have a generic way to query what Get could offer. E.g. for something like a property view.

OK. :-)

• \return List of context names. */ virtual std::vector<std::string> GetPropertyContextNames() const = 0; };

Good idea!

I still disagree in the constness. I would bring the non const version into an other interface if needed. E.g. IMutablePropertyProvider (or IPropertyOwner because it relay keeps them; and then the whole function might be non const to ?!?) or directly in the specific class.

Okay, last try to convince you for non-const pointer: Besides the fact that we would lose the possibility to observe such properties returned by the interface (yeah, not nice but that's a design flaw by ITK), we still have a SetProperty() method in this interface, so the names owner vs. provider isn't the best point here. I discussed with Caspar way too long on this interface as well and we both think that it is a bad idea to allow to return properties on the fly also because of the SetProperty method which implies more. Being radical here would lead to an interface with just GetProperty() left. Thinking about realistic use cases for this interface - BaseData and DataNode - we would both vote for IPropertyOwner and raw pointer to express the guaranteed ownership, which we have anyway for free in these use cases. Less struggle for the client to check for everything all of the time.

If you still insist to use ConstPointer, we would go for pretty much the interface in my last post with just the return type changed to ConstPointer and the promise to visit you when generalizing the property view to IPropertyProvider and asking you why the heck we're not allowed to observe the properties without const-casting or dynamic casting the interface back to BaseData or DataNode and lose the actual generalization here. :-D

We discussed in person and here is the final summary: We'll provide two interfaces. IPropertyProvider, a "const" interface solely for getting properties and IPropertyOwner, an interface which allow non-const access but guarantees ownership. The latter derives from the former.