Review du code lié à itowns #274
Conversation
|
@elias75015 tu pourrais copier le dossier itowns pour que je puisse le commenter dans la PR? |
| @@ -67,7 +67,7 @@ | |||
| "@mapbox/mapbox-gl-style-spec": "13.11.0", | |||
| "eventbusjs": "0.2.0", | |||
| "geoportal-access-lib": "2.1.6", | |||
| "itowns": "2.19.0", | |||
| "itowns": "2.23.0", | |||
There was a problem hiding this comment.
pour vérifier que tu as bien la bonne version de THREE.
tu peux ajouter dans ton code
import { REVISION } from 'three';
import { THREE } from 'itowns';
if (REVISION !== THREE.REVISION) {
throw new Error(`THREE dependencies REVISION isn't ${THREE.REVISION}`)
}There was a problem hiding this comment.
OK merci je vais essayer.
| @@ -0,0 +1,204 @@ | |||
| var CRS = { | |||
There was a problem hiding this comment.
Il y a des fichiers dont le rôle est le même côté /Ol et /Leaflet, mais avec un des particularités liées à la librairie. Peut-être y aurait-il moyen d'en factoriser une partie, mais je pense qu'il restera des spécificités non factorisables
|
|
||
| // Adding of the listeners | ||
|
|
||
| // At every globe movement, attributions may be updated, |
There was a problem hiding this comment.
view.tileLayer.info.displayed.layers donne les calques affichés, tu l'utilises?
voir l'exemple https://github.com/iTowns/itowns/blob/master/examples/misc_colorlayer_visibility.html#L56
There was a problem hiding this comment.
Si je ne dis pas de bêtise (ce qui m'arrive souvent), l'event PRE_RENDER de GlobeViewExtended utilise ce que tu cites (l65-85 de GlobeViewExtended.js), ensuite utilisé pour les attributions.
There was a problem hiding this comment.
possible utilisation LayerUtils.getAttributions dans common/utils ?
|
|
||
| var ori = layer.source.attribution; | ||
|
|
||
| if (ori) { |
There was a problem hiding this comment.
Tu m'expliqueras cette partie de vive voix.
There was a problem hiding this comment.
🗣️ ca marche ! Mais en gros on parcourt un tableau d'objets des originators (ou "attributions") liés à la couche pour décider ou non de les afficher en fonction de la vue courante (emprise, niveau de zoom) et des contraintes (bbox/zoom) intrinsèques aux originators en question.
| * } | ||
| * )); | ||
| */ | ||
| function LayerSwitcher (lsOptions) { |
There was a problem hiding this comment.
Pourquoi LayerSwitcher itowns n'hérite pas d'une classe LayerSwitcher?
Il semble avoir du code en commun avec les autres extensions.
There was a problem hiding this comment.
A discuter, ca ne me parait pas évident...
| * } | ||
| * }) | ||
| */ | ||
| LayerSwitcher.prototype.addLayer = function (layer, config) { |
There was a problem hiding this comment.
Il pourrait avoir des classes intermédiaires pour communiquer avec les API métiers et les classes GUI.
Et mettre en commun du code avec les autres extensions (https://github.com/IGNF/geoportal-extensions/blob/develop/src/OpenLayers/Controls/LayerSwitcher.js#L193)
There was a problem hiding this comment.
Même réponse que plus haut, à discuter !
| * | ||
| * @param {Boolean} collapsed - True to collapse control, False to display it | ||
| */ | ||
| LayerSwitcher.prototype.setCollapsed = function (collapsed) { |
| // .. and the infos of the configuration if they exist (title, description, legends, quicklook, metadata) | ||
| var conf = this._initLayers[i].config || {}; | ||
| var layerOptions = { | ||
| title : conf.title || layer.title || this._initLayers[i].id, |
There was a problem hiding this comment.
une fonction pourrait encapsuler la création des layerOptions et être éventuellement utiliser au ligne 293, 557, 750 et 887.
There was a problem hiding this comment.
Yes je regarderai.
| * @param {Object} e - HTML event | ||
| * @private | ||
| */ | ||
| LayerSwitcher.prototype._onChangeLayerOpacity = function (e) { |
There was a problem hiding this comment.
Cette méthode pourrais être identique à la méthode openlayer#_onChangeLayerOpacity
There was a problem hiding this comment.
Dans le cas où on se lancerait dans le travail de factorisation que tu suggères, cette fonction pourrait être effectivement mise en commun. Techniquement, seul l'appel à la fonction de la librairie sous jacente "setLayerOpacity" diffère
| * @param {Event} e - MouseEvent | ||
| * @private | ||
| */ | ||
| LayerSwitcher.prototype._onOpenLayerInfoClick = function (e) { |
There was a problem hiding this comment.
Nope 😆 Il faut plutôt lire "on open LayerInfo click" : fonction qui se déclenche lors du click sur le picto d'information de la couche sur le layerswitcher.
| * units : ["DEC", "DMS"] | ||
| * }); | ||
| */ | ||
| function MousePosition (options) { |
There was a problem hiding this comment.
La méthode semble similaire à OpenLayer.mousePosition
| * @param {String} [system.label] - CRS label to be displayed in control. Default is system.crs alias | ||
| * @param {String} [system.type] - CRS units type for coordinates conversion (one of control options.units). Default is "Metric" | ||
| */ | ||
| MousePosition.prototype.addSystem = function (system) { |
| MousePosition.prototype._setElevation = function (coords) { | ||
| // management of the timer of the altitude service request | ||
| var delay = this.options.altitude.responseDelay; | ||
| this.GPdisplayElevation(coords, delay); |
There was a problem hiding this comment.
voir la fonction commune GPdisplayElevation.
C'est le temps d'immobilité du curseur de souris qu'il faut avant de lancer une requete d'alti au service. Par défaut c'est 500ms : ca évite de surcharger le service d'alti avec des requêtes à chaque mouvement de souris.
| @@ -0,0 +1,88 @@ | |||
| /** | |||
There was a problem hiding this comment.
Itowns devrait supporter ces formats
|
|
||
| // stockage de l'élément html porteur du globe | ||
| var globeViewDiv = document.createElement("div"); | ||
| globeViewDiv.style.width = "100%"; |
There was a problem hiding this comment.
A voir, mais je pense qu'on avait fait ca directement dans le js pour des problèmes d'actions asynchrones...
| groups : [11, 14] | ||
| } | ||
| }; | ||
| config.source = new ItWMTSSource({ |
| config.updateStrategy = { | ||
| type : 0, | ||
| options : { | ||
| groups : [11, 14] |
There was a problem hiding this comment.
Cette updateStrategy est utilisée pour tout les calques d'élévation.
There was a problem hiding this comment.
Oui. Tu proposes de la retirer ?
| networkOptions : { | ||
| crossOrigin : "omit" | ||
| }, | ||
| updateStrategy : { |
There was a problem hiding this comment.
Côté itowns tu veux dire ? Inutile de notre côté ?
| options : {} | ||
| }, | ||
| format : wmsParams.format, | ||
| extent : { |
There was a problem hiding this comment.
tu peux utiliser écrire
extent : wmsParams.extent| throw new Error("ERROR : contract key configuration has to be loaded to load Geoportal layers. See http://ignf.github.io/evolution-apigeoportail/ol3/ol3-autoconf.html"); | ||
| } | ||
|
|
||
| var layerId = Config.getLayerId(options.layer, "WMTS"); |
There was a problem hiding this comment.
il y a du code qui semble factorisable avec les différents calques.
|
|
||
| miniView.setBackground(); | ||
|
|
||
| var updateMiniGlobeHandler = function () { |
There was a problem hiding this comment.
Donner à @elias75015 une nouvelle méthode avec CameraUtils
| this._isInitialized = false; | ||
|
|
||
| // call constructor | ||
| this._globeView = new ItGlobeView(globeViewDiv, coordCarto, options); |
There was a problem hiding this comment.
Constructeur changé et possibilité de mettre la position de départ
var placement = {
coord: new itowns.Coordinates('EPSG:4326', 2.351323, 48.856712),
// range: 25000000,
tilt: 90,
heading: 50,
zoom: 12,
};
// iTowns namespace defined here
var viewerDiv = document.getElementById('viewerDiv');
var view = new itowns.GlobeView(viewerDiv, placement);
| * | ||
| */ | ||
| GlobeViewExtended.prototype.onCameraMoveStop = function (cb) { | ||
| var self = this; |
There was a problem hiding this comment.
A creuser, utilisation de globeInitialized avec page de chargement?
A REPRENDRE ICI
elias75015
requested review from
lowzonenose
and removed request for
lowzonenose
|
@gchoqueux @elias75015 review à continuer. |
Pull request checklist
Verifiez que votre Pull Request remplit les conditions suivantes :
npm run build) a été lancé localement et s'est correctement déroulénpm run samples) ont été testés et validés localementnpm run test) sont passés localementType de Pull request
PR pour review du code lié à itowns
Quel type de changement cette Pull Request introduit-elle :
Quel est le comportement actuel (avant PR) :
Numéro du ticket : N/A
Quel est le nouveau comportement :
Cette PR introduit-elle des breaking changes ?
Autres informations