You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2015/06/21 23:58:11 UTC
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
GitHub user ahgittin opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/704
clean up how locations are rendered in the js gui, and expand "delete"
there was (is) confusion between catalog name and location displayName and a named location.
it deserves a major sort-out, i think treating location defs as catalog items and location instances as entities;
and only using the catalog api, not a dedicated locations api (or if there is, it delegates to catalog),
also removing all the LocationManager blah.
but until then we needed something which avoided confusion where catalog item names for locations
aren't rendered, and magic display names from location-metadata were being showed so it looked
like nothing was being added, though the location worked.
for locations, a catalog item name now does nothing, but the location's name (i.e. human-readable ID aka catalog item ID, not the display name)
is shown in the catalog accordion list (the "IdentifierName"), better info (but not the full yaml yet) is shown in the detail,
and versions are at least handled without bugs, even if only one version for location is really supported in the gui.
also adds delete for entity and apps and policies, and fixes delete for locations.
@nakomis this should help prevent the user-confusion which you experienced
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/incubator-brooklyn jsgui-locations
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/704.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #704
----
commit 91532f39a43bdf6c1d46210c599503c1166c2215
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2015-06-21T19:21:21Z
clean up how locations are rendered in the js gui, and expand "delete"
there was (is) confusion between catalog name and location displayName and a named location.
it deserves a major sort-out, i think treating location defs as catalog items and location instances as entities;
and only using the catalog api, not a dedicated locations api (or if there is, it delegates to catalog),
also removing all the LocationManager blah.
but until then we needed something which avoided confusion where catalog item names for locations
aren't rendered, and magic display names from location-metadata were being showed so it looked
like nothing was being added, though the location worked.
for locations, a catalog item name now does nothing, but the location's name (i.e. human-readable ID aka catalog item ID, not the display name)
is shown in the catalog accordion list (the "IdentifierName"), better info (but not the full yaml yet) is shown in the detail,
and versions are at least handled without bugs, even if only one version for location is really supported in the gui.
also adds delete for entity and apps and policies, and fixes delete for locations.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33032018
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/LocationResource.java ---
@@ -82,7 +86,22 @@ public LocationSummary apply(LocationDefinition l) {
return FluentIterable.from(brooklyn().getLocationRegistry().getDefinedLocations().values())
.transform(transformer)
.filter(LocationSummary.class)
- .toSortedList(SummaryComparators.displayNameComparator());
+ .toSortedList(nameOrSpecComparator());
+ }
+
+ private static NaturalOrderComparator COMPARATOR = new NaturalOrderComparator();
+ private static Comparator<LocationSummary> nameOrSpecComparator() {
+ return new Comparator<LocationSummary>() {
+ @Override
+ public int compare(LocationSummary o1, LocationSummary o2) {
+ return COMPARATOR.compare(getNameOrSpec(o1).toLowerCase(), getNameOrSpec(o2).toLowerCase());
+ }
+ };
+ }
+ private static String getNameOrSpec(LocationSummary o) {
--- End diff --
would this be better in the comparator as a non static method? Or is it more widely useful?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33123662
--- Diff: usage/jsgui/src/main/webapp/assets/js/view/catalog.js ---
@@ -113,9 +113,16 @@ define([
// Could use wait flag to block removal of model from collection
// until server confirms deletion and success handler to perform
// removal. Useful if delete fails for e.g. lack of entitlement.
- this.activeModel.destroy();
- var displayName = $(event.currentTarget).data("name");
- this.renderEmpty(displayName ? "Deleted " + displayName : "");
+ var that = this;
+ var displayName = $(event.currentTarget).data("name") || "item";
+ this.activeModel.destroy({
+ success: function() {
+ that.renderEmpty("Deleted " + displayName);
+ },
+ error: function(info) {
+ that.renderEmpty("Unable to permanently delete " + displayName+". Deletion is temporary, client-side only.");
--- End diff --
We need to overhaul this anyway to treat locations like other catalog items so not going to address.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33034260
--- Diff: usage/jsgui/src/main/webapp/assets/js/view/catalog.js ---
@@ -113,9 +113,16 @@ define([
// Could use wait flag to block removal of model from collection
// until server confirms deletion and success handler to perform
// removal. Useful if delete fails for e.g. lack of entitlement.
- this.activeModel.destroy();
- var displayName = $(event.currentTarget).data("name");
- this.renderEmpty(displayName ? "Deleted " + displayName : "");
+ var that = this;
+ var displayName = $(event.currentTarget).data("name") || "item";
+ this.activeModel.destroy({
+ success: function() {
+ that.renderEmpty("Deleted " + displayName);
+ },
+ error: function(info) {
+ that.renderEmpty("Unable to permanently delete " + displayName+". Deletion is temporary, client-side only.");
--- End diff --
In fact, didn't realise the item would reappear as soon as refresh is clicked. Perhaps it would be better not to remove it from the GUI and just say it failed (and why)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33053149
--- Diff: usage/jsgui/src/main/webapp/assets/js/view/catalog.js ---
@@ -267,7 +279,12 @@ define([
//this.model is a constructor so it shouldn't be _.bind'ed to this
//It actually works when a browser provided .bind is used, but the
//fallback implementation doesn't support it.
- var model = this.model;
+ var that = this;
--- End diff --
Is `that` variable necessary?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by mikezaccardo <gi...@git.apache.org>.
Github user mikezaccardo commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33054382
--- Diff: usage/rest-server/src/main/java/brooklyn/rest/resources/LocationResource.java ---
@@ -152,6 +171,15 @@ public Response create(LocationSpec locationSpec) {
@Override
@Deprecated
public void delete(String locationId) {
- brooklyn().getCatalog().deleteCatalogItem(locationId);
+ // TODO make all locations be part of the catalog, then flip the JS GUI to use catalog api
+ if (deleteAllVersions(locationId)>0) return;
--- End diff --
Personal preference: make the condition == 0 then throw, no return necessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33123628
--- Diff: usage/jsgui/src/main/webapp/assets/js/view/catalog.js ---
@@ -267,7 +279,12 @@ define([
//this.model is a constructor so it shouldn't be _.bind'ed to this
//It actually works when a browser provided .bind is used, but the
//fallback implementation doesn't support it.
- var model = this.model;
+ var that = this;
--- End diff --
yes - else if we did `this.name` in line 285 it might not resolve. (whereas the `this.id` refers to the model's `id`.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33031624
--- Diff: core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java ---
@@ -355,6 +355,10 @@ public void load() {
default: throw new RuntimeException("Only entity, policy & location catalog items are supported. Unsupported catalog item type " + item.getCatalogItemType());
}
((AbstractBrooklynObjectSpec<?, ?>)spec).catalogItemId(item.getId());
+
+ if (Strings.isBlank( ((AbstractBrooklynObjectSpec<?, ?>)spec).getDisplayName() ))
+ ((AbstractBrooklynObjectSpec<?, ?>)spec).displayName(item.getDisplayName());
--- End diff --
Would be nicer to see this cast only once
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/704
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#issuecomment-114297746
Code looks sensible - haven't had a chance to test it though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#discussion_r33032941
--- Diff: usage/jsgui/src/main/webapp/assets/js/view/catalog.js ---
@@ -113,9 +113,16 @@ define([
// Could use wait flag to block removal of model from collection
// until server confirms deletion and success handler to perform
// removal. Useful if delete fails for e.g. lack of entitlement.
- this.activeModel.destroy();
- var displayName = $(event.currentTarget).data("name");
- this.renderEmpty(displayName ? "Deleted " + displayName : "");
+ var that = this;
+ var displayName = $(event.currentTarget).data("name") || "item";
+ this.activeModel.destroy({
+ success: function() {
+ that.renderEmpty("Deleted " + displayName);
+ },
+ error: function(info) {
+ that.renderEmpty("Unable to permanently delete " + displayName+". Deletion is temporary, client-side only.");
--- End diff --
Reverse the order of these two sentences - I had to do a double take because it looks like a failure message but the location is removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: clean up how locations are render...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/704#issuecomment-114764306
Good nit-picking, I don't disagree with most, but this code needs overhaul so not worth addressing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---