Skip to content

PB-95: Added external KML support#593

Merged
ltshb merged 11 commits intodevelopfrom
feat-PB-95-kml-import
Jan 18, 2024
Merged

PB-95: Added external KML support#593
ltshb merged 11 commits intodevelopfrom
feat-PB-95-kml-import

Conversation

@ltshb
Copy link
Contributor

@ltshb ltshb commented Jan 4, 2024

To allow support of local KML file, the file name is used as URL and during the
parsing of the URL parameter if the url part of the layer doesn't starts with
http then it is ignored and not added to the layer list.

I also removed the isLoading = true during the parsing of the url parameter for
KML. I did this for 2 reasons:

  1. Avoid unnecessary spinner which makes the application feeling to be slow
  2. In the case of an external KML layer we cannot load the metadata and it would spin forever

Due to his isLoading changes I also updated the load kml plugin to only load
kml metadata when the kml is not external and it has missing kml metadata.

Test link

@cypress
Copy link

cypress bot commented Jan 4, 2024

Passing run #290 ↗︎

0 162 19 0 Flakiness 0

Details:

PB-95: Solve Vue 3 reactivity issues
Project: web-mapviewer Commit: 0167f1f731
Status: Passed Duration: 04:16 💡
Started: Jan 18, 2024 3:07 PM Ended: Jan 18, 2024 3:11 PM

Review all test suite changes for PR #593 ↗︎

@ltshb ltshb force-pushed the feat-PB-95-kml-import branch 3 times, most recently from a6d10b9 to 4b6e119 Compare January 5, 2024 14:20
@ltshb ltshb marked this pull request as draft January 8, 2024 11:13
@ltshb ltshb force-pushed the feat-PB-95-kml-import branch 6 times, most recently from 1009662 to d06a59e Compare January 10, 2024 14:23
@ltshb ltshb force-pushed the feat-PB-95-kml-import branch 7 times, most recently from 087a547 to 44a82c7 Compare January 16, 2024 20:25
@ltshb ltshb marked this pull request as ready for review January 16, 2024 20:25
@ltshb ltshb requested a review from pakb January 16, 2024 20:25
@ltshb ltshb force-pushed the feat-PB-95-kml-import branch from 4c0d783 to 44a82c7 Compare January 17, 2024 05:53
@ltshb
Copy link
Contributor Author

ltshb commented Jan 17, 2024

@pakb this PR is ready, it only has an issue with one drawing test that I haven't found yet the reason of this failing test. Maybe you can already have a look at my code and the test failure to see if you have some good input of the cause of this test failure ?

Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

very nicely done! couple questions/small improvements below

@ltshb ltshb force-pushed the feat-PB-95-kml-import branch 4 times, most recently from 16b1ba5 to 1cbce9f Compare January 18, 2024 10:03
@ltshb ltshb requested a review from pakb January 18, 2024 10:03
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

Looking good!

Two points that we can discuss :

  • I'm not sure you can do differently, but I don't really like the store as param to some utils file with updateKmlActiveLayer, if it is possible to move this logic to a store plugin or router plugin, that would be neat.
  • I find it can be hard to detect that the import UI has been added to the bottom of the screen when you are using a big screen. We could maybe (later or here?) add a little animation (swipe up?) to catch the eye, and make it easier to see that the UI is shown somewhere a bit "unexpected" in regards to the click location

ltshb added 11 commits January 18, 2024 16:03
Hovering the mouse on the third party disclaimer icon in a quick way showed
a choppy interface as the tooltip quickly appeared and disappeared. To avoid this
simply add a small delay.
This is needed in order to speed up boot procedure. For example for the kml
layer at startup we need to load the kml and its metadata based on its
url, this could be done asynchronously and then update only the kml data of
the layer and/or only the kml metadata of the layer. Before we needed to do
both together and update them together.
To allow support of local KML file, the file name is used as URL and during the
parsing of the URL parameter if the url part of the layer doesn't starts with
http then it is ignored and not added to the layer list.

I also changed a bit the isLoading logic during the parsing of the url parameter for
KML. I did this for 2 reasons:
 1. Avoid unnecessary spinner which makes the application feeling to be slow
 2. In the case of an external KML layer we cannot load the metadata and it would spin forever

Due to his isLoading changes I also updated the load kml plugin to only load
kml metadata when the kml is not external and it has missing kml metadata.

Simplified KMLLayer class constructor. The fileId can always be parsed from the
URL (for non external layer). In order to simplify the constructor which has
already quite a lots of parameter remove this one and parse it from the URL
whenever possible. Same for the isLoading and isExternal, those two can be
guessed from the other parameters.
…ade to vue 3.4

This feature has been adopted in Vue 3.4, therefore it is safe to use it now
within vue 3.3.

I tried to upgrade to Vue 3.4, but it breaks the drawing module ! Apparently
Vue 3.4 has a bug when returning a ref from a composable and watch this ref in
a component using the composable. In this case the watch is not fire when the
ref changes.

This was the case where in `useDrawingModeInteraction.composable.js` the returned
ref `lastFinishedFeature` did not fire change event on the watcher in `DawingMarkerInteraction.vue`
The menu section logic was as follow:

1. Open active layers section when a layer was added to an empty list
2. The active layers was open at startup only if the list was not empty
3. The active layers and geo catalogue section were close when opening
any other section

In this logic the active layers was never automatically re-opened unless it went
from empty to non empty.

Now the logic has been changed as follow:

1. By default the active layers is always open (at startup)
2. We have two type of sections:
  a. singleMode (formerly nonScrollable): when a section is open, all others are
     closed
  b. multiMode (formerly scrollable): when a section is open, only singleMode
     sections are closed
3. When the drawing and advanced tool section are closed, the active layer section
   is automatically open
4. When the import file tool is open, the active layer section is open (closing
  the advanced tool section)

This allow for a better user experience and feedback when adding drawing and/or
external layers
In order to debug using the browser debugger tool during cypress tests you
need to have the source map and for this you need to provide the vite config
to the cypress file pre-processor.

Also we need to have the server mode and the cypress mode in sync in order
to use our environment variable in tests, otherwise the test will get the
prod variable while the server is using the dev variables.
The Vue 3 and Vuex documentation specifies that we should only use javascript
plain object as reactive object.

Unfortunately we don't do this with our active layers that are javascript class
and put into the store. For this use case it is quite convenient and changing
this would require a big rework.

Therefore we keep the class but with a big warning. The main known issue is
that the class getter/setter cannot be reactive which leads to subtle bugs
hard to find. Reactivity could also have some issues if the class contains
side effect methods (method that set properties).

Here I removed all getter/setter and added the warning.
@ltshb ltshb force-pushed the feat-PB-95-kml-import branch from 1cbce9f to 0167f1f Compare January 18, 2024 15:04
@ltshb ltshb merged commit 913d604 into develop Jan 18, 2024
@ltshb ltshb deleted the feat-PB-95-kml-import branch January 18, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants