Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

135 property and header handling #138

Merged
merged 11 commits into from Oct 4, 2016

Conversation

saschazelzer
Copy link
Member

@saschazelzer saschazelzer commented Sep 7, 2016

This PR introduces (partial) OSGi compliance for retrieving bundle manifest headers and framework properties:

  • Changes as outlined in Fix up bundle property and manifest header handling #135
  • Localization of header attributes is not supported
  • A new AnyMap class to avoid testing an Any for different container types
  • The framework properties FRAMEWORK_VERSION and FRAMEWORK_VENDOR are now always added. Also, the FRAMEWORK_STORAGE property is always set.

Note that my editor fixed up all the white-space issues in BundleManifest.cpp so the diff unfortunately has a lot of noise.

US_TEST_CONDITION(buExec.GetBundleContext().GetProperty(Constants::FRAMEWORK_UUID).Empty() == false, "Test for non-empty framework uuid property")
auto props = buExec.GetBundleContext().GetProperties();
US_TEST_CONDITION_REQUIRED(props.empty() == false, "Test for non-empty bundle props")
US_TEST_CONDITION(props.count(Constants::FRAMEWORK_VERSION) > 0, "Test for existing framework version prop")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we ever have more than one FRAMEWORK_VERSION property? If not, I'd suggest making this check for the existence of only one FRAMEWORK_VERSION property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, will do. I was only concerned about existence in the test. We do not support multi maps.

*/
US_Framework_EXPORT extern const std::string BUNDLE_COPYRIGHT; // = "bundle.copyright";

/**
* Manifest header containing a brief description of the bundle's
* functionality.
*
* The header value may be retrieved via the \c Bundle::GetProperty method.
* The header value may be retrieved from the \c AnyMap object
* returned by the \c Bundle::GetHeaders() method.
*/
US_Framework_EXPORT extern const std::string BUNDLE_DESCRIPTION; // = "bundle.description";

/**
* Manifest header identifying the bundle's menifest version.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo; "menifest"

@saschazelzer
Copy link
Member Author

I decided to split the AnyMap class into a pure STL container class any_map following STL naming conventions and a derived class AnyMap adding convenience functions. These convenience functions use our PascalCase naming. They are still mixed with the inherited under_score style functions, but I feel reluctant to create a STL base container using PascalCase.

@saschazelzer
Copy link
Member Author

saschazelzer commented Sep 29, 2016

@karthikreddy09 Are you still thinking about the "any map" issue? What is your conclusion?

@karthikreddy09
Copy link
Member

karthikreddy09 commented Sep 30, 2016

@saschazelzer Sorry, I have been procrastinating on this issue.

I am having a hard time figuring out why a user would want to iterate the map returned by Bundle::GetHeaders and check for typeid of values.

The primary use case would be to retrieve a specific value out of the headers. GetUsingCompoundKey will solve this use case. Another one I can think of is to print the map, the stream operator overload serves this purpose.

Even if the user wants to iterate the map returned by Bundle::GetHeaders, Would it not be sufficient to document the fact that the only possible value types are : scalar type, std::string, std::vector<us::Any>, AnyMap(case insensitive due to manifest header requirement)

Could you provide an example usecase or a code snippet to help me understand why a user would want to iterate and check the typeid of values inside the returned AnyMap.

You mentioned about AnyMap replacing all direct usages of STL associative containers in a Any type. Did you mean all maps with key_type=std::string ? or any key_type ? A us::Any object with content_type=std::map<int,us::Any> cannot be stored in a AnyMap.

@saschazelzer
Copy link
Member Author

I agree that for our own specific use cases today, we would not need the more generic AnyMap class.

However, I am not comfortable with focusing on the origins of an Any instance (e.g. the GetHeaders function) and making assumptions based on that origin. The Any class is generic framework API and as such independent of any assumptions about its contained types.

  1. Something like AtCompoundKey returns a Any. This could hold some map (e.g. the key from a manifest files references a JSON object). Prior knowledge (e.g. from documentation) could be used to extract the actual value using the documented map type. But I am concerned about the generic case without prior knowledge.
  2. AtCompoundKey is part of the type-erasing AnyMap class and hence "just works". As a free function, it would need to branch out to different map types to be able to recurse into their contents in order to resolve the compound key.
  3. It is more elegant IMHO to test for only one map type instead of many, in case the underlying type needs to be accessed (which is the case in our ToString and ToJSON functions where the compiler branches out for us, selecting the correct overload, or which would also be the case for a free AtCompoundKey function implementation withoug AnyMap).
  4. An example of recursive iteration of an arbitrary Any is the webconsole bundle (the code is not part of the development branch yet). It takes an arbitrary Any object (typically from the GetHeaders function, but it should not need to assume that) and recursively iterates the Any contents to display it in structured way (e.g. a tree) on a web page. Granted, in that case using the ToJSON function will be easier because JSON is native to JavaScript.

The Any class can hold a std::map<int,Any> type (if a stringification function is provided), but I do not see how we could elegantly type-erase the key. The proposal here tries to homogenize the map<string,Any> case for different containers types and comparators (but still a fixed set).

@karthikreddy09
Copy link
Member

It sounds reasonable to simplify for the most common use case of std::map.

+1 for merge.

@saschazelzer saschazelzer merged commit 07e57d0 into development Oct 4, 2016
@saschazelzer saschazelzer deleted the 135-property-and-header-handling branch October 4, 2016 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants