Major refactoring, documenting and geoJSON viewing support #54

gentlegiantJGC · 2022-02-14T20:05:25Z

This is still work in progress but is mostly complete. I thought I would just open this so that you can see what I have been working on and give feedback.

This started out with the intention of adding geoJSON viewing support to the map plugin but has spiralled into more than that.

Adds support for the full geoJSON format within the front matter under the geoJSON key as well as within a code block of type geoJSON

Fixes #50

Added a large number of comments and docstrings.
Moved all images into an img directory to declutter the root directory.
Modified the build system so that npm run dev builds into the root directory so that the repository can be directly in the obsidian plugins directory.
Rewritten regexes to use named capture groups rather than relying or ordering.
Renamed a number of variables (and functions) containing location to coordinate to disambiguate from file location.
Moved map setState and getState methods out of the constructor and into the class.
Increased the max map zoom level so that you can zoom the map in further.
Capped the maxNativeZoom (map tile zoom) to stop OSM tiles going blank when zooming in too far.
- perhaps the maxNativeZoom should be configurable per map source.
Improved the front matter handling.
- The old implementation had bugs if the key exists but in a nested object.
- The new implementation finds and parses the data as yaml and calls a callback to edit the loaded data before writing back.
  - This gives more control than the simple setdefault function that existed before.

Won't Fix

Renamed quite a few functions to better describe what they do.
Reworked the behaviour of some functions.
Renamed urlConvertor to coordinateParser because the class is not limited to urls.

GeoJSON Support

Added the BaseGeoLayer class which FileMarker and GeoJSONLayer extend.
Changed all typing to use the BaseGeoLayer class from which implementations extend.
Added GeoJSONLayer class to handle the geoJSON type.
Moved marker UI creation into the FileMarker class.
Modified leaflet Polygon behaviour to make it cluster.

(and probably other things I have forgotten about)

Obsidian could not find the built main.js file in the dist folder

The old function name read like it would return the location of the front matter within the file. The new function name better describes what it does which is getting the coordinate from the front matter

Two more functions were a little ambiguous so renamed them to be less ambiguous Renamed location variable to coordinate

This class is capable of parsing more than just urls Also renamed some methods

Renamed a number of methods and added docstrings. parseCoordinateFromLine -> parseEditorLine parseCoordinateFromString -> parseString insertLocationToEditor -> editorInsertGeolocation convertUrlAtCursorToGeolocation -> editorLineToGeolocation

Split the yaml parsing and overwriting logic into a new function with a callback to do the modifying. This handles the yaml properly by parsing and then serialising it back. The previous implementation broke if the key exists only as a nested key. The value entry is now optional (defaulting to null) The value entry is now any arbitrary value which is then serialised to yaml. The only negative of this is that it populates null if no value is defined in the yaml

In order to implement multiple geographic data types (pins, polygons, polylines, ...) there needs to be a standard implementation that the map can use. This base class is that

Moved mapMarker to geoLayer Moved the marker init logic into the marker class so other geographic layer types can do the same. The BaseGeoLayer class defines the initGeoLayer which sets up the behaviour for that geographic type. It must populate the geoLayer field with a leaflet layer.

There may have been a reason for this but not that I can see

Settings can be found in the class attributes

The new name more concisely describes what the method does.

This better describes what the function does. Update implies extending but this will remove points if they are not in the added markers.

Renamed matchInlineLocation to matchInlineCoordinates to better describe what the function does. Rewritten regexes to use named capture groups to make them easier to maintain.

Added getBounds method so that it can work with points and shapes

pyCharm added this when I tried to type Array

This could probably be improved with better support for the other layer types.

esm7 · 2022-02-16T18:24:01Z

First and foremost, thanks for doing this, it will be a great contribution to Map View!
It will take me a few days to do a proper review and hopefully we can get it merged and released within an iteration or two 😎
From a very fast skim through the changes it seems like high-quality refactoring and additions; one thing I don't like though (which is a matter of taste I guess) are the full 3-line docstrings in so many places.
I completely agree that this code requires much more documentation and greatly thank you for doing so, but -

I prefer to omit documentation that explains in one line what one line of code does, e.g. IMHO a method like loadSettings doesn't need 3 lines of a docstring to say "load the settings from disk".
Even when really useful, I prefer not to prepend each member in a type by 3 lines of a docstring; at least to me it takes too much screen real-estate when trying to work on the actual code. For members that are not self-explanatory I'd appreciate if you convert the documentation to // comments.
Is there a reason you use tags like @private in the docstrings? Isn't that why we use the Typescript private keyword?

A more comprehensive review will hopefully follow in the upcoming days :)

gentlegiantJGC · 2022-02-16T18:35:05Z

The docstrings were populated mostly by PyCharm and I just filled them in.
I am a Python developer and I have done very little JavaScript before so there may be other things I am applying python logic to that is wrong for JavaScript.
In python you write the docstring and there are tools that can build documentation from the source code. I don't know if JavaScript can do that from single line comments.
I was mostly adding docstrings so that I could hover on the function call and pycharm would display the documented function.
Edit: PyCharm can collapse the full docstring but I don't know if other IDEs do that

Edit2: It looks like the three line docstrings can be compacted onto a single line

/**
 * Docstring
 */

/** Docstring */

gentlegiantJGC · 2022-03-18T09:56:01Z

I would like to get this sorted. I have some more features that I would like to implement that will build upon these changes.
I will have a go at merging your previous changes.
My thoughts for more additions are:

Map overlay - overlay other map images on the leaflet map
Image pins with FOV indicator and thumbnail on hover (Added the ability to automatically add images with location data to the map #16 might do some of this)
Embed the map into a file with the ability to specify attributes. The normal map is great but having presets in files would be great (eg a file containing a map with just images). This would also be good to visualise individual pins.
Adding the ability to filter pins by date (eg only show images between two dates)

Edit: I would also like to implement a typescript formatter and validator so that there are not issues like in #16

gentlegiantJGC · 2022-03-18T10:13:39Z

It might be easiest if I split this into multiple pull requests since I am going to have to rewrite a lot of it anyway

esm7 · 2022-03-18T13:01:58Z

I'm really sorry for the holdup; not sure how it slipped my mind, and that's surely not how your great contributions deserve to get treated :(
I'll do all it takes to review & merge next week 🤞

gentlegiantJGC · 2022-03-18T13:55:28Z

Thanks. I am looking into splitting this into multiple pull requests to make it easier to merge.
I also need to work out how your changes effect it.

gentlegiantJGC · 2022-03-18T20:03:27Z

I am part way into splitting this pull request into multiple pull requests. This should make it easier to review since each pull request will concentrate on a single component. Some components do depend on other things so I will need to wait until you have merged the previous ones to continue.

As of currently:
#62 moves images into a sub-directory to declutter the root directory
#64 makes the development build easier to use by building into the root directory
#65 adds the comments and docstings that were added here
#63 implements prettier commands to standardise code style. Pull requests will make sure it has been run. I suggest merging this one last.

gentlegiantJGC added 30 commits February 7, 2022 11:01

Made it easier to run the development build

d42a256

Obsidian could not find the built main.js file in the dist folder

Moved images into an img folder to declutter the root

48e960e

Renamed getFrontMatterLocation to getFrontMatterCoordinate

e5932b1

The old function name read like it would return the location of the front matter within the file. The new function name better describes what it does which is getting the coordinate from the front matter

Renamed another two methods and variables

416aacf

Two more functions were a little ambiguous so renamed them to be less ambiguous Renamed location variable to coordinate

Simplified if else

4406742

Renamed urlConverter to coordinateParser

d176ce3

This class is capable of parsing more than just urls Also renamed some methods

Added some comments to the main script

698c668

Cleaned up the coordinate parser

7141029

Renamed a number of methods and added docstrings. parseCoordinateFromLine -> parseEditorLine parseCoordinateFromString -> parseString insertLocationToEditor -> editorInsertGeolocation convertUrlAtCursorToGeolocation -> editorLineToGeolocation

Renamed the globe icon

91b2e54

Renamed variable fileLocation to characterIndex

d5d9df4

Cleaned up the newNote util function

ed795f4

Added docstrings and cleanup to mapView

bc6db93

Added some docstrings to marker.ts

b6a2812

Added a base class for all geo data

d97a042

In order to implement multiple geographic data types (pins, polygons, polylines, ...) there needs to be a standard implementation that the map can use. This base class is that

Increased max zoom and stopped tiles disappearing when zoomed in too far

c4cbf93

Move the get and set state methods out of the constructor

37ea66e

There may have been a reason for this but not that I can see

Removed input from isDarkMode

a448a10

Settings can be found in the class attributes

Added missing ViewStateResult import

e526d45

Reordered and documented the MapView attributes

8b4c3dc

renamed updateMapToState to setMapState

fe1289d

The new name more concisely describes what the method does.

Added more docstrings and comments to mapView

d5dc84f

Renamed updateMapMarkers to setMapMarkers

8bb779e

This better describes what the function does. Update implies extending but this will remove points if they are not in the added markers.

Rewritten regular expressions to use named capture groups

3538a5f

Renamed matchInlineLocation to matchInlineCoordinates to better describe what the function does. Rewritten regexes to use named capture groups to make them easier to maintain.

Moved some FileMarker attributes into the base class

887be2c

Switched typing to the base geo layer class

1b91b89

Fixed a compile error when accessing the location attribute

b149bd8

Added getBounds method so that it can work with points and shapes

Added geoJSON viewing support

4feae76

Removed incorrectly imported variable

9c86382

pyCharm added this when I tried to type Array

Added in the UI overlay for geoJSON object

343c6e6

This could probably be improved with better support for the other layer types.

Compacted single line docstrings

1147777

gentlegiantJGC marked this pull request as ready for review March 18, 2022 08:40

gentlegiantJGC marked this pull request as draft March 18, 2022 19:55

gentlegiantJGC deleted the branch esm7:master May 2, 2022 10:33

gentlegiantJGC closed this May 2, 2022

gentlegiantJGC deleted the master branch May 2, 2022 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Major refactoring, documenting and geoJSON viewing support #54

Major refactoring, documenting and geoJSON viewing support #54

Uh oh!

gentlegiantJGC commented Feb 14, 2022 •

edited

Loading

Uh oh!

esm7 commented Feb 16, 2022

Uh oh!

gentlegiantJGC commented Feb 16, 2022 •

edited

Loading

Uh oh!

gentlegiantJGC commented Mar 18, 2022 •

edited

Loading

Uh oh!

gentlegiantJGC commented Mar 18, 2022

Uh oh!

esm7 commented Mar 18, 2022

Uh oh!

gentlegiantJGC commented Mar 18, 2022

Uh oh!

gentlegiantJGC commented Mar 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Major refactoring, documenting and geoJSON viewing support #54

Major refactoring, documenting and geoJSON viewing support #54

Uh oh!

Conversation

gentlegiantJGC commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esm7 commented Feb 16, 2022

Uh oh!

gentlegiantJGC commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gentlegiantJGC commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gentlegiantJGC commented Mar 18, 2022

Uh oh!

esm7 commented Mar 18, 2022

Uh oh!

gentlegiantJGC commented Mar 18, 2022

Uh oh!

gentlegiantJGC commented Mar 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gentlegiantJGC commented Feb 14, 2022 •

edited

Loading

gentlegiantJGC commented Feb 16, 2022 •

edited

Loading

gentlegiantJGC commented Mar 18, 2022 •

edited

Loading