Add kml:Link, kml:Icon #28

k-mack · 2022-07-28T06:07:01Z

Attempt to add support for kml:Link and kml:Icon.

A couple of points to discuss.

First

Since the spec's kml:Icon (or kml:BasicLinkType) is already implemented and exported by the library as Kml::Icon, I used an enum to model the spec's kml:LinkType. Its variants are Kml::Link and Kml:Icon. Both are constructed with kml::types::LinkModel, the common data model used by the spec's kml:LinkType.

If kml:Model was to be implemented using this PR, then it's link field would be of type kml::types::LinkType and it would need to be guarded so as not to have it be set to kml::types::LinkType::Icon:

pub struct Model {
    link: Option<kml::types::LinkType>,
    // ...
}

impl Default for Model {
    fn default() -> Model {
        Model { link: None }
    }
}

impl Model {
    pub fn get_link(&self) -> Option<&kml::types::LinkType> {
        self.link.as_ref()
    }

    pub fn set_link(&mut self, link_model: &kml::types::LinkModel) {
        self.link = Some(kml::types::LinkType::Link(link_model.clone()));
    }
}

Let me know how you feel about this approach.

Alternatively, I could use two identical structs with different names. I could expose Icon as Kml::LinkIcon to not have it interfere with the existing Kml::Icon.

What makes the most sense for an approachable, intuitive API?

Second

I used empty strings for the optional String fields that have no default value. Let me know if you'd prefer to have them be Options instead.

Closes #10.

pjsier

Thanks so much for the PR and thorough write up! I think this approach makes sense, but I'm a bit worried about needing a setter for kml:Model. Other than the duplicate code do you see any issues creating separate structs?

Otherwise it would be great to convert the fields with empty string to Option<String> since that's what we're using for other elements, and thanks for flagging that.

pjsier · 2022-07-28T22:40:37Z

src/writer.rs

+            LinkType::Link(model) => (b"Link", &*model),
+        };
+        self.writer
+            .write_event(Event::Start(BytesStart::owned_name(tag_name.to_vec())))?;


We'll need to add this here to capture the attributes

.with_attributes(self.hash_map_as_attrs(&link_model.attrs))

k-mack · 2022-07-29T04:14:18Z

As much as I like the idea of using an enum, it doesn't make sense from an API ergonomics perspective. If Rust supported enum variants as first-class types, then I think it'd be a clean approach.

I'll break the enum into separate structs.

Instead of using enum variants to represent `kml:Link` and `kml:Icon`, which makes it awkward to use them since enum variants are not first-class types in Rust, they are implemented using two separate structs. Since the library already exports the public type `Kml::Icon` from the `crate::types::style` module, `Kml:LinkTypeLink` and `Kml:LinkTypeIcon` are exported. These are aliases for the internal types `kml::types::link::{Link,Icon}`.

k-mack

Separate structs feels more Rusty. I left a couple of comments to call out some of the decisions I made that are worth a review.

k-mack · 2022-07-29T06:00:40Z

src/reader.rs

+                refresh_mode: Some(types::RefreshMode::OnChange),
+                view_refresh_mode: Some(types::ViewRefreshMode::OnStop),
+                view_format: Some(String::new()),
+                attrs,


Do we want to store the attributes? I see some other types do, but most ignore them. The spec only allows three here: id, targetId, and a user-defined one. Maybe I should ignore them to conform to how it is done for other types?

As an aside, I could see how many attributes could be an issue with memory. Perhaps that's a separate issue to be solved in another PR.

I think enough of the other types include attrs to keep it for now, but worth flagging for the future

k-mack · 2022-07-29T06:03:32Z

src/reader.rs

+                href: Some("/path/to/local/resource".to_string()),
+                refresh_mode: Some(types::RefreshMode::OnChange),
+                view_refresh_mode: Some(types::ViewRefreshMode::OnStop),
+                view_format: Some(String::new()),


In this test, viewFormat is an empty element, and this is treated as a value. Any desire to identify this and assign the optional String value to None?

I think the test and behavior work as-is, since I would assume None would mean the tag isn't present at all

k-mack · 2022-07-29T06:06:47Z

src/types/mod.rs

@@ -36,6 +36,10 @@ mod geometry;

 pub use geometry::Geometry;

+mod link;
+
+pub use link::{Icon as LinkTypeIcon, Link as LinkTypeLink, RefreshMode, ViewRefreshMode};


Not a fan of these aliases, but I couldn't think of anything better. Decided to alias Link only because Icon was aliased. WDYT?

I think I'd prefer we only alias Icon to LinkTypeIcon and remove the LinkTypeLink to keep things as consistent with the spec as possible

pjsier

This is looking great, thanks for the changes! The last two things to update would be implementing FromStr for the enums and removing the Link alias, then I think this is ready to merge

pjsier · 2022-07-30T13:36:34Z

src/reader.rs

+                href: Some("/path/to/local/resource".to_string()),
+                refresh_mode: Some(types::RefreshMode::OnChange),
+                view_refresh_mode: Some(types::ViewRefreshMode::OnStop),
+                view_format: Some(String::new()),


I think the test and behavior work as-is, since I would assume None would mean the tag isn't present at all

pjsier · 2022-07-30T13:37:43Z

src/types/mod.rs

@@ -36,6 +36,10 @@ mod geometry;

 pub use geometry::Geometry;

+mod link;
+
+pub use link::{Icon as LinkTypeIcon, Link as LinkTypeLink, RefreshMode, ViewRefreshMode};


I think I'd prefer we only alias Icon to LinkTypeIcon and remove the LinkTypeLink to keep things as consistent with the spec as possible

pjsier · 2022-07-30T13:40:13Z

src/reader.rs

+                        icon.refresh_mode = match self.read_str()?.as_str() {
+                            "onChange" => Some(RefreshMode::OnChange),
+                            "onInterval" => Some(RefreshMode::OnInterval),
+                            "onExpire" => Some(RefreshMode::OnExpire),
+                            _ => None,
+                        }
+                    }


Instead of including this here, could we implement FromStr for RefreshMode as well as the other added enums? We've done this for AltitudeMode and ColorMode as well and it keeps things a little cleaner

pjsier · 2022-07-30T13:41:03Z

src/types/link.rs

+
+/// `kml:viewRefreshModeEnumType`, [16.27](https://docs.opengeospatial.org/is/12-007r2/12-007r2.html#1270) in the KML specification.
+#[derive(Clone, Debug, PartialEq)]
+pub enum ViewRefreshMode {


It would be great to implement FromStr here as well

Enable and simplify reading these types using str's `parse` method.

pjsier

This looks great, thanks again!

k-mack added 3 commits July 27, 2022 21:48

Add initial support for Link and Icon

371945e

Refactor kml:Link & kml:Icon to be enum variants

6e83cff

Add read support for Link and Icon

4ec2c4e

pjsier reviewed Jul 28, 2022

View reviewed changes

k-mack commented Jul 29, 2022

View reviewed changes

pjsier reviewed Jul 30, 2022

View reviewed changes

k-mack force-pushed the km/link-icon-linktype branch from 024e424 to 5f70a96 Compare July 31, 2022 21:24

k-mack added 2 commits July 31, 2022 14:24

Remove alias for kml::types::Link

8b0a6ad

Impl FromStr for RefreshMode and ViewRefreshMode

5f70a96

Enable and simplify reading these types using str's `parse` method.

pjsier approved these changes Jul 31, 2022

View reviewed changes

pjsier merged commit eaa6db7 into georust:main Jul 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kml:Link, kml:Icon #28

Add kml:Link, kml:Icon #28

k-mack commented Jul 28, 2022

pjsier left a comment

pjsier Jul 28, 2022

k-mack Jul 29, 2022

k-mack commented Jul 29, 2022

k-mack left a comment

k-mack Jul 29, 2022

pjsier Jul 30, 2022

k-mack Jul 29, 2022

pjsier Jul 30, 2022

k-mack Jul 29, 2022

pjsier Jul 30, 2022

pjsier left a comment

pjsier Jul 30, 2022

pjsier Jul 30, 2022

pjsier Jul 30, 2022

k-mack Jul 31, 2022

pjsier Jul 30, 2022

k-mack Jul 31, 2022

pjsier left a comment

Add kml:Link, kml:Icon #28

Add kml:Link, kml:Icon #28

Conversation

k-mack commented Jul 28, 2022

First

Second

pjsier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-mack commented Jul 29, 2022

k-mack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjsier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjsier left a comment

Choose a reason for hiding this comment