SLING-8655 - Updated dependency on API, provided Externalized Path In… #14
Conversation
…jector (with default Provider) and an unit test
paul-bjorkstrand
left a comment
paul-bjorkstrand
left a comment
There was a problem hiding this comment.
Not to push this too far out of scope, but have you given any thought on modifying link URLs inside rich text?
| } | ||
|
|
||
| /** Fallback Implementation of the Externalized Path Provider that uses the Resource Resolver's map function **/ | ||
| private class DefaultExternalizedPathProvider |
There was a problem hiding this comment.
This should be at least a static class, as it does not use its enclosing class at all. I would probably move this into its own class and make it a proper @Component.
There was a problem hiding this comment.
Moved this to be an regular OSGi Component
| // The providers are sorted so that the one with the highest priority is the first entry | ||
| Collections.sort( | ||
| providerList, | ||
| Comparator.comparingInt(ExternalizedPathProvider::getPriority).reversed() |
There was a problem hiding this comment.
Instead of sorting based on a new field called priority, you should probably use service.ranking of the implementations. Perhaps you want to look at how ModelAdapterFactory uses ranked services.
There was a problem hiding this comment.
Used RankedServices as suggested
| } | ||
| if (element.isAnnotationPresent(ExternalizePath.class)) { | ||
| ValueMap properties = getValueMap(adaptable); | ||
| if(properties != ObjectUtils.NULL) { |
There was a problem hiding this comment.
nitpicky formatting on this if and the if on line 85 as well.
| assertEquals("Wrong Provider was selected", mappedImagePath3, value); | ||
| } | ||
|
|
||
| private class TestExternalizedPathProvider |
There was a problem hiding this comment.
Similar to above, when you are not using the enclosing class data, the nested class should be static
2 participants
…jector (with default Provider) and an unit test
This PR depends on PR of the Sling Models API: apache/sling-org-apache-sling-models-api#2