BGDIINF_SB-2729: Added multiple KML edit and simplify KML drawing logic#326
BGDIINF_SB-2729: Added multiple KML edit and simplify KML drawing logic#326
Conversation
fee2ed3 to
7ece86a
Compare
2229f73 to
2a8ced7
Compare
7ece86a to
f3f8cc5
Compare
f3f8cc5 to
5548495
Compare
5548495 to
800cc20
Compare
When creating a KML add the author version. Added get KML metadata from backend API helper
Before when the from the backend failed you couldn't close the drawing menu and the error status was not displayed. Also when an error happened outside the toggling of the show drawing menu, the exception was not catch. Now we simply set the status to ERROR and re-trigger an update later on.
800cc20 to
7b2e701
Compare
jedef
left a comment
There was a problem hiding this comment.
Hello
I could not finish the review today, as there are many changes. But I will still publish the comments that I have already added.
I have seen that you have not added any comments to explain that the savingStatus is for indicative uses only and is not always correct (see the comment in #325). Do you think that it is not necessary?
| throw e | ||
| } | ||
| if (this.savingStatus !== SavingStatus.UNSAVED_CHANGES) { | ||
| this.savingStatus = SavingStatus.SAVED |
There was a problem hiding this comment.
There is no grarantee that during the saving of the drawing, the user does not do any modifications that lead to unsaved changes. So I would not remove this if statement. (But you can put it in the try statement if you want)
| await this.addKmlDrawingLayer(layer) | ||
| // Remove the layer to not have an overlap with the drawing from | ||
| // the drawing manager | ||
| this.removeLayer(layer) |
There was a problem hiding this comment.
Now that you have moved out all the logic of this function (addKmlLayerToDrawing), why not fusion it with addKmlDrawingLayer? This would also remove the confusion with the two very similar names.
| if (this.abortedToggleOverlay) { | ||
| this.abortedToggleOverlay = false | ||
| return | ||
| } |
There was a problem hiding this comment.
Removing this protection while retrying the saving automatically when the prior saving failed (line 316) is very dangerous. Because now if you loose the internet connection and quit the drawing, not only will you loose your changes, but in the background the drawing module will still continuousely try to save the drawing. So when the internet connection comes again, as the drawing layer is empty when not in drawing mode, it will overwrite your kml drawing with a blank one.
So I would either improve the saving retry policy so that it stops to try to save the drawing when quitting the drawing mode, or readd the above protection, or both. Or/and clear the kml metadata when quitting drawing mode to prevent any accidental overwriting of the kml.
There was a problem hiding this comment.
I removed this protection as it did not work correctly. It simply prevented to close the menu without retry of the failed saved and during my test (I changed the url of the backend to force a failed saving) the error message was not displayed. So that was not very user friendly as you could not do the action that you want without any indication why. I did not debug to see why the saving status of error was not displayed though.
I think we should not try to make decision for the user, this will not be understood. Automatic retry on auto save is the best IMHO.
However you brought a very good point with the auto retry continuing when the drawing menu is closed ! I'll fix this, see #331
BGDIINF_SB-2729: Fixed save KML drawing error handling - from post review of #326
Test link