Drill down to alarm sub tree displays from alarm area panel #3061
Conversation
Add query parameter to alarm URI.
| // expand tree item if is matches item name | ||
| if (tree_view.getRoot() != null && itemName != null) { | ||
| for (TreeItem treeItem : tree_view.getRoot().getChildren()) { | ||
| if (String.valueOf(treeItem.getValue()).startsWith(itemName)) { |
There was a problem hiding this comment.
Shouldn't .equals() be used instead of startsWith()?
There was a problem hiding this comment.
Ok. The better meaning comparison, the better solution. Drawback in this case is uglier casting and comparison of tree items.
| * @throws Exception | ||
| */ | ||
| private Node create(final URI input) throws Exception | ||
| private Node create(final URI input, String itemName) throws Exception |
There was a problem hiding this comment.
Perhaps the parameter itemName could be given a more descriptive name, like, e.g., itemToExpand or similar? (This also applies to other places in the PR where variations of "item name" are used for naming.)
There was a problem hiding this comment.
I have handled this by javadoc comments. I will expand on javadoc comments for this.
| * @return ["localhost:9092", "Accelerator", null] or ["localhost:9092", "Accelerator", "param=value"] | ||
| * @throws Exception on error | ||
| */ | ||
| public static String[] parseAlarmURI(final URI resource) throws Exception |
There was a problem hiding this comment.
I would suggest to change the return type of parseAlarmURI() to a type with more structure by:
- Importing
org.apache.commons.lang3.tuple.ImmutableTriple - Change the return type from
String[]to the typeImmutableTriple<String, String, Map<String, String>>which provides more structure. - Basically build the functionality of
getRawQueryParameterValue()into this function by computing the mapMap<String, String>from parameter-keys to their values, and returning that map as part of the return value of this function.
The function getRawQueryParameterValue() should then most likely no longer be necessary, since the values of parameters can be determined by querying the map from keys to values.
There was a problem hiding this comment.
I don't understand why to consider ImmutableTriple.
It is possible and fairly easy to return e.g. ImmutableMap<String, String> (Guava) with parameters and values. However ImmutableMap does not allow null value. An URI with query string ...?param1=value1¶m2 would give null value for param2.
This means that e.g. url "alarm://host.my.site/Test?param1=value¶m2" corresponds to map with 2 entries, param and param2 of which first has value "value1" and second has null value. However this does not work when converting this to ImmutableMap. ( names and values are put into Map than then is converted into ImmutableMap )
Therefore, options for method are to return either a Map<String, String> or as currently is done. Going for Map<String, String>.
| } | ||
| } | ||
|
|
||
| OpenTreeViewAction otva = new OpenTreeViewAction(alarmConfigName, "itemName=" + URLEncoder.encode(item_name, StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Should the = be replaced by a reference to AlarmURI.DELIMITER_QUERY_PARAMETER_VALUE?
There was a problem hiding this comment.
Alternatively, since the delimiters are standard for URIs I believe, should the two constants AlarmURI.DELIMITER_QUERY_PARAMETERS and AlarmURI.DELIMITER_QUERY_PARAMETER_VALUE be removed?
There was a problem hiding this comment.
I recognize that it's somewhat verbose having such constants. I removed those constans and instead introduced constant for "itemName".
There was a problem hiding this comment.
Very nice that you have written tests!
| /** | ||
| * Constructor | ||
| * @param alarmConfigName The alarm configuration name | ||
| * @param alarmRawQuery raw query for alarm (null if no such information is available) |
There was a problem hiding this comment.
Using null for empty values is of course OK! However, using the type Optional<String> together with the constructor-methods Optional.empty() and Optional.of() would convey the intention of the code more clearly through the types, I think. (This also applies to other places where null is used to mean an empty value.)
There was a problem hiding this comment.
Refactored constructors to avoid need to explicitly have null value.
|
Merged after approval |
Add query parameter to alarm URI.