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

[wip] mobile-query #540

Merged
merged 1 commit into from
Feb 8, 2016
Merged

[wip] mobile-query #540

merged 1 commit into from
Feb 8, 2016

Conversation

adube
Copy link
Contributor

@adube adube commented Jan 21, 2016

This PR introduces the query service and mobile-query directive.

It's still work in progress, so here's a check list of the items remaining to do to consider this task completed (I added what's already done as well):

  • one query per server
  • support WMTS -> WMS queries
  • support GML as info format
  • show total
  • respect order
  • activation management
  • fix the outrageous number of watchers in the 2 mobilequery examples (in ngeo and gmf)
  • tests
  • documentation
  • see why Travis fails
  • review

The GeoJSON info format will not be added.

We will NOT add the query tool in the "mobile" example page as part of this task since the result is not yet completed.

Live examples

@adube
Copy link
Contributor Author

adube commented Jan 21, 2016

@fgravin Here's a list of things I'd like you to check so far:

  • see the e-mail about the watchers - I would very much like to reduce their number
  • see if how I manage the activation of the directive is okay to you

@adube adube force-pushed the query branch 4 times, most recently from d3e7242 to bcb5d64 Compare January 26, 2016 14:09
@adube
Copy link
Contributor Author

adube commented Jan 26, 2016

@fgravin: @pgiraud and I would like to talk to you about the "MobileQuery" directive and the way the activation works. If you recall correctly, we discussed about this and you introduced me to $scope.$watch. We would like to review this approach. I'll talk to you about this tomorrow.

@adube
Copy link
Contributor Author

adube commented Jan 27, 2016

@fgravin Travis fails because of the template in contrib/gmf/examples/partials. I'm pretty sure it's because there's something missing in the Makefile to manage those.

@adube
Copy link
Contributor Author

adube commented Jan 29, 2016

@fgravin You can look at the Travis error now. Thanks.

@ger-benjamin
Copy link
Member

@adube When we create a directive in contrib/gmf we add (and use in our examples) a new default template located in the contribs/gmf/src/directive/partials folder. (Look at: https://github.com/camptocamp/ngeo/blob/master/contribs/gmf/src/directives/search.js#L18-L27 and https://github.com/camptocamp/ngeo/blob/master/contribs/gmf/src/directives/search.js#L66). And offer the possibility to override it.

I think that we should not create specific templates for gmf examples.

@adube
Copy link
Contributor Author

adube commented Feb 1, 2016

@ger-benjamin, the directive is not agmf one, but rather a ngeo one. But, we need to have 2 examples because of the gmf.QueryManager service.

Both the examples in ngeo and gmf currently use a template that is in the example directory. The one in ngeo should stay there in my opinion because we don't plan to create a directive for the results in ngeo. For the one in gmf, we do so we could flush it. I don't think it's currently ready, though.

What should I do with this ? Could we talk about this ?

@adube
Copy link
Contributor Author

adube commented Feb 1, 2016

FYI, it is as I suspected: there was something missing in the Makefile. Waiting for travis, but I expect it to build properly now.

no one-time binding here -->
<li ng-repeat="source in qrCtrl.result.sources"
role="presentation"
ng-if="source.features.length">
Copy link
Member

Choose a reason for hiding this comment

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

try a one time binding here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, when doing that the result no longer shows. It needs to stick as it is.

@fgravin
Copy link
Member

fgravin commented Feb 1, 2016

Some minor comments but it's globally excellent work.
Please see the comments and squash all commits in one.

I would like to have to links to the examples as well to see the result.

@adube
Copy link
Contributor Author

adube commented Feb 1, 2016

@fgravin I need help for 2 issues in the 'builded' mode:

  • (minor issue) - how can I include the boostrap css file ? The hosted-example doesn't pick the one in lib, i.e. the path stays to ../node_modules/...
  • (major issue) - on click, I get an error from angular, which I have no clue what to do with. How can I debug this ?
TypeError: Cannot set property namespaceURI of #<Element> which has only a getter
    at py.kc (http://localhost:3000/.build/examples-hosted/lib/ngeo.js:690:139)
    at hu (http://localhost:3000/.build/examples-hosted/lib/ngeo.js:548:88)
    at py.l.Ha (http://localhost:3000/.build/examples-hosted/lib/ngeo.js:547:738)
    at oB.<anonymous> (http://localhost:3000/.build/examples-hosted/lib/ngeo.js:839:217)
    at Array.forEach (native)
    at oB.<anonymous> (http://localhost:3000/.build/examples-hosted/lib/ngeo.js:839:175)
    at http://localhost:3000/.build/examples-hosted/lib/angular.min.js:119:113
    at n.$eval (http://localhost:3000/.build/examples-hosted/lib/angular.min.js:133:221)
    at n.$digest (http://localhost:3000/.build/examples-hosted/lib/angular.min.js:130:233)
    at n.$apply (http://localhost:3000/.build/examples-hosted/lib/angular.min.js:133:512)

@adube
Copy link
Contributor Author

adube commented Feb 2, 2016

About the above error, I create a PR in ol3: openlayers/openlayers#4772. I'll switch the ol3 version to use that branch to be able. Once merged, I'll go back to ol3#master

@adube adube force-pushed the query branch 2 times, most recently from e19b3c9 to eca1ccc Compare February 3, 2016 13:25
@adube
Copy link
Contributor Author

adube commented Feb 3, 2016

Travis fails after I rebased onto master. I get the following errors (here's only the first few ones):

make dist
mkdir -p dist/
node buildtools/build.js .build/ngeo.json dist/ngeo.js
info ol Parsing dependencies
info ol Compiling 396 sources
ERR! compile /opt/ngeo/adube/src/directives/desktopgeolocation.js:124: ERROR - actual parameter 1 of goog.events.listen does not match formal parameter
ERR! compile found   : ol.Geolocation
ERR! compile required: (EventTarget|goog.events.Listenable|null)
ERR! compile       this.geolocation_,
ERR! compile       ^
ERR! compile 
ERR! compile 
ERR! compile /opt/ngeo/adube/src/directives/desktopgeolocation.js:134: ERROR - actual parameter 1 of goog.events.listen does not match formal parameter
ERR! compile found   : ol.Geolocation
ERR! compile required: (EventTarget|goog.events.Listenable|null)
ERR! compile       this.geolocation_,
ERR! compile       ^

Any idea where those come from ?

@pgiraud
Copy link
Contributor

pgiraud commented Feb 3, 2016

Not your fault. Master build is broken.
We're currently trying to fix this.

On Wed, Feb 3, 2016 at 2:32 PM, Alexandre Dubé notifications@github.com
wrote:

Travis fails after I rebased onto master. I get the following errors
(here's only the first few ones):

make dist
mkdir -p dist/
node buildtools/build.js .build/ngeo.json dist/ngeo.js
info ol Parsing dependencies
info ol Compiling 396 sources
ERR! compile /opt/ngeo/adube/src/directives/desktopgeolocation.js:124: ERROR - actual parameter 1 of goog.events.listen does not match formal parameter
ERR! compile found : ol.Geolocation
ERR! compile required: (EventTarget|goog.events.Listenable|null)
ERR! compile this.geolocation_,
ERR! compile ^
ERR! compile
ERR! compile
ERR! compile /opt/ngeo/adube/src/directives/desktopgeolocation.js:134: ERROR - actual parameter 1 of goog.events.listen does not match formal parameter
ERR! compile found : ol.Geolocation
ERR! compile required: (EventTarget|goog.events.Listenable|null)
ERR! compile this.geolocation_,
ERR! compile ^

Any idea where those come from ?


Reply to this email directly or view it on GitHub
#540 (comment).

Pierre GIRAUD
Géomaticien, Analyste

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac, Cedex

Tel : 00 33 4 79 44 44 93
Mail : pierre.giraud@camptocamp.com
http://www.camptocamp.com

@adube adube force-pushed the query branch 2 times, most recently from 8768394 to 38de733 Compare February 8, 2016 13:32
@adube
Copy link
Contributor Author

adube commented Feb 8, 2016

@fgravin This is ready to merge, once Travis passes.

@fgravin
Copy link
Member

fgravin commented Feb 8, 2016

So it works on minified mode ?

@adube
Copy link
Contributor Author

adube commented Feb 8, 2016

@fgravin Yes. See the live demos in the PR description, above.

* @param {gmf.Themes} gmfThemes The gme themes service.
* @param {gmf.QueryManager} gmfQueryManager The gmf query manager service.
*/
app.MainController = function(gmfThemes, gmfQueryManager) {
Copy link
Member

Choose a reason for hiding this comment

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

Where gmfQueryManager is used ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's required here. If we don't inject this, then it's not created and it doesn't do its magic.

@fgravin
Copy link
Member

fgravin commented Feb 8, 2016

ngeo example doesn't work, CORS issue :/

@adube
Copy link
Contributor Author

adube commented Feb 8, 2016

@fgravin I made the fix to use the new architecture of the ToolActivate, which is way more simpler now! Nice stuff.

I also updated the github.io pages.

Ready for merge, once Travis passes.

@adube
Copy link
Contributor Author

adube commented Feb 8, 2016

ngeo example doesn't work, CORS issue :/

It does for me right now. Would you please check again ?

@fgravin
Copy link
Member

fgravin commented Feb 8, 2016

Very good thanks

fgravin added a commit that referenced this pull request Feb 8, 2016
@fgravin fgravin merged commit 879cf20 into camptocamp:master Feb 8, 2016
@adube adube deleted the query branch February 9, 2016 16:26
@sbrunner sbrunner added this to the Older milestone Aug 23, 2019
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

5 participants