Catalogtree #126

procrastinatio · 2013-07-12T16:47:31Z

I think that this PR is now in a state where it makes sense to merge it into master.

What's done:

Load Catalogtree based on topic and language
Loads Catalogtree definition from catalog service
React on topic changes (reloads tree)
React on language changes (reloads tree)
On hover, a preview of the layer will be shown directly in the map (inspired by oteral's wms browser)
Add/remove layers to/from map by clicking checkbox in tree
Collaps/Expand catalog tree, initial state reflects data as returned by gaLayers layers definition initially
Integrated in accordion
Style more or less resembles what is specified
Working example

What's not done (to be done in future PR's):

React on info button (showing metadata -> which service do we use?)
Keep state of tree on language changes (this will not be easy as we load from server on language changes)
Probably more styling (didn't test on different browsers)
Tests
Once the layer selection component is in place, we need to synchronize with catalogtree component
other things...

Note:
CatalogService returns layers that are not defined in corresponding layers service. This means that certain layers are not addable from the catalogtree

Below is the original message of Marc

This is only to discuss:

There is a basic example in /ltmom/app/src/catalogtree/example/catalogtree.html

- Expandable/collapsible branches
- Configured with a json file (the equivalent service has to be merged  mf-chsdi3/pull/13)
- Opened/closed branch as defined
- I am not very happy with my template: I need two templates (branches/leafs), should I use two directives, or is there a way to use only parts of a template ?

gjn · 2013-07-19T07:43:26Z

Discussions went on in our mailing list:
https://groups.google.com/forum/#!topic/re3-dev/L_zjmnwRRGk

I think we should continue this here.

gjn · 2013-07-19T07:50:49Z

I've just added new commits. We are now using the approach found by Eric on jsfiddle. I think it's very similar to what Marc did to start with, but it looks a little more clean.

Also, the directive is now using a standard partial (templateUrl parameter) for all purposes. We are using ng-if statements to make sure the right stuff is created.

Examples as well as integration in accordion is done.

There's a couple of things still to be done (I'll work on that next week. Today no more time):

First analysis shows that we create massive memory leaks. We need to get rid of those. ng-repeat obviously does not destroy all scopes. I verified this using chromes Profiles tab (Take Heap Snapshots), which increases constantly when changing topic on the tree.
I don't understand why our ng-click functions from the template need to manually call preventDefault() and stopPropagation(). I thought this was part of ng-click? Does anyone has experience in that regard?
Adding functionality (selection, etc) which have to wait on other components
Lots of css (probably something for @tonio when functionality is there

Please review current code. I'm especially worried about 1 and 2.

gjn · 2013-07-19T11:03:16Z

I analysed a little further. Scopes are all destroyed via $destroy call on the parent element. Anyhow, memory consumption still grows contiually maybe because the GC didn't kick in yet. Any way to force that to happen?

gjn · 2013-07-23T06:11:04Z

I did some rough analysis on memory consumption:

There seems to be a bug in Batarang (the chrome angularjs debug plugin) creating memory leaks, especially on ng-repeat directives. [1]
There was a bug in angularjs creating memory leaks on chrome. This has been fixed in our current version, but who knows ([2] and [3]).
I created a simple test app. This app changes catalogtree every 15 s (timeout triggered), switching between 400 node tree and 40 node tree. I let this run in Chrome and Firefox for 15 hours. In Chrome, there was an increase of 50kb of memory (might be causes by something else, as chrome has a very strange memory management model) and 10kb of memory increase in Firefox. So, this doesn't look as dramatic...

[1] angular/batarang#62
[2] angular/angular.js#1313 (comment)
[3] https://github.com/angular/angular.js/blob/master/src/ng/rootScope.js#L621

gjn · 2013-07-24T07:26:38Z

src/style/app.less

@@ -184,6 +185,8 @@
    }
 }
 .accordion-inner {
+    max-height: 480px;


I think this should probably be depending on screen-size (and orientation?). We'll let @tonio deal with this.

gjn · 2013-07-30T12:59:09Z

I thinks it would make sense to merge this into master, even though it's not fully complete.

Please review.

tonio · 2013-07-30T13:22:47Z

Just a detail, but there’s a mix between (2|4) spaces in your less files (app.less & catalogtree.less).

gjn · 2013-07-30T13:31:05Z

@tonio Good find.

Should be fixed now.

oterral · 2013-08-07T06:37:06Z

src/components/catalogtree/style/catalogtree.less

+.ga-catalogtree-entry {
+    margin-top: 0px;
+    margin-bottom: 0px;
+}


.ga-catalogtree-entry rule is defined twice.

loicgasser · 2013-08-08T12:25:43Z

Since you're using the service CatalogServer you could remove the file src/components/catalogtree/example/tree.json

gjn · 2013-08-08T12:33:16Z

I think it makes sense to not have a dependency to some external service in examples.

loicgasser · 2013-08-08T13:28:45Z

ok I get it

On Thu, Aug 8, 2013 at 2:33 PM, Gilbert Jeiziner
[email protected]:

I think it makes sense to not have a dependency to some external service
in examples.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/126#issuecomment-22320150
.

loicgasser · 2013-08-08T16:42:42Z

src/components/catalogtree/CatalogtreeDirective.js

+      map.removeLayer(layer);
+    }
+  };
+


In order to avoid code duplication (search and catalog, probably layertree later), functions like removeLayerFromMap, addLayerToMap, getMapLayer could all in a service like gaMapUtils. What do you think?

Absolutely. This is on Eric's desk as part of the layer list component.

Other existing directives will need to be changed once this components will
be ready.

Or the gaLayers could provide convenience functions to add and remove layers to maps.

gaLayers.addToMap(layerId, map); gaLayers.removeFromMap(layerId, map);

Could be added to gaLayers. Right now, this service is map agnostic.

Also, I think this new component will need to take care of other things as well. We have different kinds of layers: background, cataloglayers* (wms, wmts, aggregate), kml import layers_, wms import layers_, preview layers (in search, catalogtree, wms import), feature layers for redlining, feature layers for measurements, etc. Those denoted with * are part of the layer list. For all of these, order needs to be managed and maintained by this service.

cedricmoullet · 2013-08-14T19:10:08Z

I have a strange behaviour on Chrome/Mac -> I can't click on the radiobutton (but can click on the layer name) in order to add the layer. Is it on all plattform ?

cedricmoullet · 2013-08-14T19:12:01Z

The hover effect is active on the whole line, but the possibility to add the layer works only if the mouse is over the layer name. Is it possible to make that the possibilty to add a layer works like the hover effect ?

cedricmoullet · 2013-08-14T19:13:13Z

Regarding design:

Would it be possible to follow the spec ?

gjn · 2013-08-15T06:36:46Z

Regarding the icons (category icons, checkbox), I'm using Font Awsome, which don't have round category icons (folded, unfolded) and don't have squared checkbox icons (checked, unchecked). They have what's currently visible. I've discussed it with Dave and he's ok with it.

Regarding colors: yes, I'll need to adapt those.

gjn · 2013-08-15T06:48:14Z

Regarding hover: on my machine, it works as you wish to have it. I've tested on Chrome, FF and IE9.

Also, adding layers by clicking work in all 3.

Apparently, it's a problem on Mac/Chrome

gjn · 2013-08-15T08:47:00Z

Addressed all problems with latest commit.

There's still the question about the lines going all the way to the left. But before we hack something, I'd like to discuss this with Dave.

gjn · 2013-08-15T08:54:15Z

Another rebase done to have popup service.

gjn · 2013-08-15T09:38:49Z

Added usage of popup service to display metadata information when clicking on the information button.

Note: showing of metadata information will likely be refactored into a seperate service/directive, which can be used in other components (I think search component will also need it)

But I would like to have those merged first before doing the refactoring.

Please review and merge.

loicgasser · 2013-08-15T10:01:31Z

src/components/map/MapService.js

+        return 'http://mf-chsdi30t.bgdi.admin.ch/rest/services/' +
+               topicId + '/MapServer/' +
+               layerId + '/getlegend?lang=' +
+               lang + '&callback=JSON_CALLBACK';


could be updated thanks to f520973

We took out modifications of 'val' inside watch function for 'val' which resulted in infite loops (which were gracefully treated by angular until now, but it was still bad). In place, we put the corresponding variables to the scope. We put the node and leaf html snippets in partials. To load this partials, we use ng-include snippets at runtime (they are hard coded in js, which will still changes), which are global to the directive module.

This commit changes the css styling to better reflect the specifications. It removes unneeded <div> elements from the html, which improves interaction. Fix chrome but.

…o container directive

gjn · 2013-08-16T09:47:52Z

Took responsibility to update the tree on language/topic change out of controller and put it into a new container directive. Thanks to @elemoine for this input.

Changes based on other comments and did a new rebase.

Ready to be reviewed again and merged.

elemoine · 2013-08-16T09:51:33Z

src/components/catalogtree/CatalogtreeDirective.js

+          },
+          link: function(scope, element, attrs) {
+            var currentTopic;
+            scope.updateCatalogTree = function() {


Do we really need a reference to the updateCatalogTree function in the scope? Instead, can't updateCatalogTree just be local to the link function?

Absolutely. I'll adapt.

elemoine · 2013-08-16T10:02:45Z

In the example I see the catalog tree twice in the page? Is it expected?

elemoine · 2013-08-16T10:10:05Z

I'm seeing bugs related to the voidLayer:

select the swissALTI3D Hillshade layer in the catalog
then select the voidLayer using the background layer selector
select the color map background layer again using the background layer selector
BUG the swissALTI3D Hillshade layer is not visible anymore

This can be addressed with a separate PR, but I think an issue should be created right after merging this PR.

gjn · 2013-08-16T10:13:52Z

Regarding example: yes, it is expected.

Regarding bugs: there are quite a few other similar bugs too (for example, hovering over a background layer in the tree will remove it from the map). This is because we don't have a central instance doing our layer management (the layer manager we were speaking on Tuesday).

I take responsibility to update catalog_tree once we have the layer manager in place.

elemoine · 2013-08-16T10:14:05Z

Something I don't like in the catalog tree UI: after selecting a layer in the catalog the layer gets displayed in the map but it disappears when I start moving my mouse. I know it reappears when the mouse goes out of the layer tree item, but I still find it quite confusing. Can be addressed later as well I think.

gjn · 2013-08-16T10:15:21Z

This can easily be fixed.

elemoine · 2013-08-16T10:16:50Z

src/components/catalogtree/CatalogtreeDirective.js

+(function() {
+  goog.provide('ga_catalogtree_directive');
+
+  var module = angular.module('ga_catalogtree_directive', [


Your module should depend on ga_translation.

elemoine · 2013-08-16T10:17:53Z

Regarding example: yes, it is expected.

My apologies. I had missed that.

elemoine · 2013-08-16T10:19:19Z

This is super impressive work! I'll merge this PR when you tell me again it's ready for merging.

gjn · 2013-08-16T10:26:25Z

Thanks for the review and compliments.

I adapted based on your recent comments. I think it can be merged....and I'm off to Gampel Open Air!

Catalogtree

elemoine · 2013-08-16T11:46:48Z

Merged. Again, fantastic work!

gjn reviewed Jul 24, 2013
View reviewed changes

oterral reviewed Aug 7, 2013
View reviewed changes

loicgasser reviewed Aug 8, 2013
View reviewed changes

loicgasser reviewed Aug 15, 2013
View reviewed changes

procrastinatio and others added 5 commits August 16, 2013 11:40

first test with a catalogtree

d299aac

using a local file to configure the catalog

d2c6b8b

Fixing rebasing glitches

c43833a

Using templateCache for html snippets (non-working)

7f4eac8

Gilbert Jeiziner added 11 commits August 16, 2013 11:41

Adding service to get metadata and link it

fc76b19

Some refactorings to increase readability. Minor changes

b813229

Make directive workable after rebase to master

230583a

Adapt styling and html structure, fix chrome/MAC bug

a45aac4

This commit changes the css styling to better reflect the specifications. It removes unneeded <div> elements from the html, which improves interaction. Fix chrome but.

Adding popup for metadata/legend info window

504c4a9

Renaming of files to prepare for container directive

a617b73

Renaming of elements

29ff945

Adding new wrapper directive for catalogtree

8b0b860

Take out responsibility to update tree from controller and put it int…

a4ff512

…o container directive

Make example work with latest changes

3a87edd

Fix naming and ident

d750ab8

elemoine reviewed Aug 16, 2013
View reviewed changes

Remove update function from scope

4ed9129

elemoine reviewed Aug 16, 2013
View reviewed changes

Taking into account recent comments

ca0ae2a

elemoine mentioned this pull request Aug 16, 2013

Make layers config URL template configurable #261

Merged

elemoine pushed a commit that referenced this pull request Aug 16, 2013

Merge pull request #126 from geoadmin/dev_catalogtree

b28789b

Catalogtree

elemoine merged commit b28789b into master Aug 16, 2013

elemoine deleted the dev_catalogtree branch August 16, 2013 11:46

Catalogtree #126

Catalogtree #126

Conversation

procrastinatio commented Jul 12, 2013

gjn commented Jul 19, 2013

gjn commented Jul 19, 2013

gjn commented Jul 19, 2013

gjn commented Jul 23, 2013

Choose a reason for hiding this comment

gjn commented Jul 30, 2013

tonio commented Jul 30, 2013

gjn commented Jul 30, 2013

Choose a reason for hiding this comment

loicgasser commented Aug 8, 2013

gjn commented Aug 8, 2013

loicgasser commented Aug 8, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedricmoullet commented Aug 14, 2013

cedricmoullet commented Aug 14, 2013

cedricmoullet commented Aug 14, 2013

gjn commented Aug 15, 2013

gjn commented Aug 15, 2013

gjn commented Aug 15, 2013

gjn commented Aug 15, 2013

gjn commented Aug 15, 2013

Choose a reason for hiding this comment

gjn commented Aug 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elemoine commented Aug 16, 2013

elemoine commented Aug 16, 2013

gjn commented Aug 16, 2013

elemoine commented Aug 16, 2013

gjn commented Aug 16, 2013

Choose a reason for hiding this comment

elemoine commented Aug 16, 2013

elemoine commented Aug 16, 2013

gjn commented Aug 16, 2013

elemoine commented Aug 16, 2013