Skip to content

BGDIINF_SB-2733: Removed drawing adminId from url and open drawing mode automatically#344

Merged
ltshb merged 10 commits intodevelopfrom
feat-BGDIINF_SB-2733-drawing-admin-id-part-2
Jan 25, 2023
Merged

BGDIINF_SB-2733: Removed drawing adminId from url and open drawing mode automatically#344
ltshb merged 10 commits intodevelopfrom
feat-BGDIINF_SB-2733-drawing-admin-id-part-2

Conversation

@ltshb
Copy link
Contributor

@ltshb ltshb commented Jan 12, 2023

Thee adminId is now removed from the URL and upon adminId detection, the drawing
menu is automatically opened.

Test link

Test link with legacy url kml adminId
Test link with kml adminId
Test link with kml

External WMS with mf-geoadmin3 URL syntax
External WMTS with new URL syntax

Test link

@ltshb ltshb changed the base branch from develop to bugfix_BGDIINF_SB-2709-drawings-not-interactive-out-of-drawing-mode January 12, 2023 12:44
@ltshb ltshb changed the base branch from bugfix_BGDIINF_SB-2709-drawings-not-interactive-out-of-drawing-mode to feat-BGDIINF_SB-2733-drawing-admin-id-part-1 January 12, 2023 12:45
@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch from acff01d to 64f0437 Compare January 12, 2023 13:46
@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-1 branch from b9d77e1 to 778df93 Compare January 12, 2023 13:48
Base automatically changed from feat-BGDIINF_SB-2733-drawing-admin-id-part-1 to develop January 12, 2023 15:06
@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch 7 times, most recently from 1d3f155 to 1408ea9 Compare January 17, 2023 07:47
@ltshb ltshb requested a review from pakb January 17, 2023 07:49
@ltshb ltshb marked this pull request as ready for review January 17, 2023 07:49
@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch 3 times, most recently from 07c1368 to 2268dab Compare January 17, 2023 12:41
@ltshb ltshb requested a review from jedef January 17, 2023 14:48
@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch from 2268dab to d8fb125 Compare January 19, 2023 16:53
Copy link
Contributor

@jedef jedef left a comment

Choose a reason for hiding this comment

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

Overall very good changes!

@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch 2 times, most recently from 4429dd7 to fc7d55a Compare January 23, 2023 08:27
@ltshb ltshb requested a review from jedef January 23, 2023 09:28
Copy link
Contributor

@jedef jedef left a comment

Choose a reason for hiding this comment

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

seems good, but there's one place where I am not sure if there is a race condition.

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.

discussing very minor changes
otherwise looks good!

@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch 4 times, most recently from 9a71f57 to a7df06b Compare January 25, 2023 13:05
if ('addToMap' in payload && 'layerId' in payload) {
const layer = rootGetters.getActiveLayerById(payload.layerId)?.clone()
if (layer) {
layer.addToMap = payload.addToMap
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this will not raise an error / warning, we should only edit the store in the context of a mutation (actions are out of scope too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pakb The store is not changed here, the layer is cloned and the clone is changed and pass to addLayer() where the store mutation happens in a mutation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note we have the same code in the layers store actions, where the store value is cloned in order to make changes in the action which then trigger a mutation with the new value.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh right, didn't catch the .clone() up there, so all is good

Copy link
Contributor

Choose a reason for hiding this comment

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

check my last review on #348 and see if it's not too hard to achieve, then we can merge this one

ltshb added 10 commits January 25, 2023 14:45
Store mutation in the router were notified twice because the
mutations were subscribed twice instead of one.
…de automatically

Thee adminId is now removed from the URL and upon adminId detection, the drawing
menu is automatically opened.

Re-worked the KML metadata for simplification, now it is set together with
the layer in the layer store action. This simplify the url to layer store
logic.
Also avoid to have duplicate layer object in the store, now the active/selected
kml layer for drawing is only kept in the layer store, previously it was cloned
and also kept in the drawing store. The drawing menu has a copy of the layer
when the drawing mode is open, but this copy is only used by the drawing module
and is not in a store !

To avoid having the kml default opacity hardcoded in several places in the code
the KML layer class constructor has been changed in order to have the opacity
optional. So now the default opacity is only hardcoded in one place in the
KMLLayer class.
…ing menu

The adminId is kept in memory until next reload. Also keep the layerId in the
url when inside the drawing mode in order not to loose if the page is reloaded.

We also uses the last KML in the active list (independently of visible flag) as
the kml to edit/copy. This is need because if the user is reloading the page
while editing the layer will be invisible after reloading, so the user still
have the possibility (without making the layer visible again) to copy edit
his drawing.
Let Vue router to handle URL encoding and parsing without a special hack
for the layers parameter. This caused troubled with the E2E tests.

Moreover we should be consistent in the encoding and not use any special case
for one parameter. This complicate the code and make it more error prone.
Waiting on the activelayers array size is not enough after closing the drawing
mode as the drawing layer is added in invisible mode before closing and its
visible flag is toggled on close.
When a KML admin ID was detected in the layer and the active layers was updated,
the drawing mode was automatically open. This was expected upon parsing the
KML admin ID from url, but this also happened when the active layers changed
like for example toggling any layer visibility or config or re-ordering layers.

Now we explicitely open the drawing mode on admin id parsed from url parsing.
Now while drawing the kml layer visible flag is kept to true, this allow the
user to reload the page while drawing without loosing the visible flag. This
commit also fixed the issue that a drawing copy would be added to the active
layer list instead of replacing the copied drawing.

Re-order the layers store actions and mutation to have logic order, this
make the code easier to read/navigate.
The auto open drawing mode based on KML admin ID from Url had a possible race
condition, which never occurred during testing, but to be safe this race
condition is handled here.
@ltshb ltshb force-pushed the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch from 17f99ab to 35cc121 Compare January 25, 2023 13:45
@ltshb ltshb requested a review from pakb January 25, 2023 13:46
@ltshb ltshb merged commit 315edaa into develop Jan 25, 2023
@ltshb ltshb deleted the feat-BGDIINF_SB-2733-drawing-admin-id-part-2 branch January 25, 2023 14:00
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.

3 participants