Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes update implementation, testing, and documentation #170

Merged
merged 3 commits into from
May 18, 2023

Conversation

TimPurdum
Copy link
Collaborator

Closes #168

Attributes now are a new AttributesDictionary type. I avoided a breaking change by adding an implicit conversion operator so users could still provide a plain Dictionary<string, object> to the attributes parameter. However, they would need to get the strongly typed dictionary to call the new async methods for updating attributes.

I actually did set up the blazor binding to actually update the attributes on next render if the parameter dictionary is altered. This is accomplished by overriding SetParametersAsync. I think this is a pattern that might work to also support changing for example the Lat/Long of an extent after a user drags the view, which before I thought was impossible. You would have to store the previously-entered values, separate from values being tracked by user manipulation.

Anyway, back to attributes, I'm not sure whether I should be marking/warning that providing Dictionary<string, object> is deprecated, or if we should just keep supporting this as a convenience. The one thing that would not work is in C#, after render, to call _graphic.Attributes = new Dictionary<string, object>.... This would never update, unless you manually also called so makes me think we should keep the warnings.

@TimPurdum TimPurdum requested review from seahro and AndersenBell May 13, 2023 13:39
@TimPurdum TimPurdum self-assigned this May 13, 2023
@@ -124,7 +118,7 @@ public bool Equals(Graphic? other)
return (other?.Id == Id) ||
(other is not null &&
(other.Geometry?.Equals(Geometry) == true) &&
(other.Attributes?.Equals(Attributes) == true));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there are always attributes in graphic layers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, no, but the ? means this would not equal true if other.Attributes returns null.

{
if (kvp.Value is JsonElement jsonElement)
{
object? typedValue = jsonElement.ValueKind switch
Copy link
Collaborator

Choose a reason for hiding this comment

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

had to do some research on Valuekind (https://learn.microsoft.com/en-us/dotnet/api/system.text.json.jsonelement.valuekind?view=net-7.0)
So this is what sets up the "conversion" of the attribute dictionary to allow the change comparison to occur.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is kludgy but yes, we're basically force-checking the type from JSON to C#.

public object this[string key] => _backingDictionary[key];
}

[ProtoContract(Name = "Attribute")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Had to do some research on protobuf to get a little better understanding. Is speed the primary reason for serializing with protobuf in this situation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, if you look at the protobuf serialization, you get highly-compressed data, where each class/property name is replaced by a number based on the position. This means less data per graphic, which equals to faster transfer when you have a large set.

Copy link
Collaborator

@seahro seahro left a comment

Choose a reason for hiding this comment

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

Lots of Documentation adjustments.
Couple questions/comments. This definately kicked up some additional research.

@TimPurdum TimPurdum merged commit f879ad5 into develop May 18, 2023
@TimPurdum TimPurdum deleted the bug/168_graphic_attributes branch May 18, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Graphic.Attributes update method
2 participants