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

🚀 [Feature Request]: Better handling of null values in Telemetry Data #1027

Open
2 of 3 tasks
jake-b opened this issue Dec 19, 2024 · 0 comments
Open
2 of 3 tasks
Labels
enhancement New feature or request

Comments

@jake-b
Copy link
Contributor

jake-b commented Dec 19, 2024

OS

iOS, iPadOS, macOS

Description

Problem statement:

My understanding is that relatively recently, the protobufs were updated to handle nil values by pairing a “has” attribute with the value attribute (eg. hasTemperature and temperature). I think that much of the code in this app predates this change.

Currently the zero-values are loaded into all database fields rather than nils. This results in bad data being presented to the user. For example: Pip-Boy, below, has no temperature sensor, but the app presents a 0-degC value. (In this case there is only a radiation value in the telemetry, but the app doesn’t know about it yet.)

image

Possible Solutions

Leave it as is

The app filters out zero values even though zero values may be valid. For example, you don’t see windSpeed in Pip-Boy example because the value is 0.0 and the app does not display a widget. There is utility to knowing wind is being measured but there is currently no wind. This approach continues the pattern where zero values cannot be distinguished from nil values.

âž• No impact to code
âž– Not able to distinguish zeros from nil

Update the app to use NSNumbers

The CoreData schema already flags these attributes as optional, meaning the database can store null values. These attributes, however, are also flagged as “use Scalar type”. This means the auto-generated implementations of the NSManagedObject Swift classes use scalar types (Int, Float, Double etc) directly. Unfortunately, they are not generated as Swift optionals. (@objc code can’t return optionals)

If you turn off the “Use Scalar Type” option, then Xcode will auto-generate these as NSNumber? optionals. Doing so would require them to be handled/converted everywhere they are used.

2024-12-19 at 10 13 AM

Pros/Cons:

âž• Proper handing of nil values allows you to differentiate them in the UI.
âž– You have lots of places you need to update in the app to handle NSNumber, and this clutters the app with lots of converting back and forth.

Create a custom NSManagedObject swift implementation for TelemetryEntity

Another step after switching to NSNumber? would be a custom swift implementation of the TelemetryEntity rather than the auto-generated NSManagedObject. Custom getters/setters could be written to unwrap the NSNumber to optional scalar types.

Example:

	public var channelUtilization: Float? {
		get {
			return (primitiveValue(forKey: "channelUtilization") as? NSNumber)?.floatValue
		}
		set {
			let newValue = newValue.map { NSNumber(value: $0) }
			setPrimitiveValue(newValue, forKey: "channelUtilization")
		}
	}

(This could be done with a property wrapper to keep the code somewhat cleaner)

Pros/Cons

âž• Keeping the code somewhat similar, just adding optionality
➕ Don’t need to change the “Use Scalar Type” field on each attribute (overriding the accessors in code)
➖ Do need to change the CodeGen configuration to “Manual/None” for TelemetryEntity and add files to the project to hold the manual implementations. (does this need a new version of the CoreData model? not sure... I don't think its a migration, just a change to some flags that control code generation)
âž– Still need to update everywhere in the codebase that uses these attributes to handle the new optionality.
âž– Lose the automatic code generation. Now have a custom implementation to maintain every time new attributes are added to TelemetryEntity.
âž– TelemetryEntity would be the only manually generated entity, making it oddly different from all other CordData entities.
➖ Slightly incompatible with Pull Request #1016. These custom getters/setters are not objective-C compatible, so can’t use `NSExpression to convert the keyPath to string anymore. Basically can’t derive the Identifiable ID from the keyPath anymore (not a huge deal, can use the column name to identify instead)

Conclusion

Given the amount of work and the various ways to approach the issue, I figure it should be discussed:
(a) whether this should be done (maybe its a "won't fix")
(b) which way to implement it if it something we want to do.

Participation

  • I am willing to pay to sponsor this feature.
  • I am willing to submit a pull request for this feature.

Additional comments

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@jake-b jake-b added the enhancement New feature or request label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant