Fixes #4370 - History metadata enums and struct access #4371
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4371 +/- ##
=======================================
Coverage 75.94% 75.94%
=======================================
Files 46 46
Lines 4191 4191
=======================================
Hits 3183 3183
Misses 1008 1008 Continue to review full report at Codecov.
|
| @@ -10,6 +10,15 @@ import Foundation | |||
| public enum DocumentType: Int32 { | |||
| case regular = 0 | |||
| case media = 1 | |||
| case unknown = -1 | |||
|
|
|||
| public init(rawValue: Int32) { | |||
There was a problem hiding this comment.
So what's your use case for this? Can't you just do DocumentType.media when creating instances? I suppose this may be helpful when deserializing, but I don't think you'd need to persist a DocumentType yourself manually.
Also, these numbers aren't entirely random - they need to align w/ what other platforms are using. And currently we don't have a -1 present elsewhere. So unless you absolutely need this change, I'd remove at least the unknown case.
| @@ -28,6 +43,16 @@ public enum HistoryMetadataObservation { | |||
| case titleObservation(String) | |||
| case viewTimeObservation(Int32) | |||
| case documentTypeObservation(DocumentType) | |||
| case unknown | |||
There was a problem hiding this comment.
What is an unknown observation, and why do you need it? Does seem necessary to me.
There was a problem hiding this comment.
I really don't need unknown but removing that causes swift errors for our initializers so I added additional case for default as unknonw. Happy to change / rename it as needed.
There was a problem hiding this comment.
What about function overloading? Separate init functions for each data type? Then you can remove unknown, i think.
|
|
||
| public init(value: Any) { | ||
| switch value { | ||
| case let title as String: self = .titleObservation(title) |
There was a problem hiding this comment.
What if we have string observations that aren't about title, or Int32 that aren't time? Also, what's the use case here for the init, can't you just do HistoryMetadataObservation.titleObservation("hello")?
There was a problem hiding this comment.
The major issue is that we need some sort of initializer for using it on Firefox iOS side otherwise structs and enums are inaccessible. This is just something how swift hasn't gotten to or a limitation I would say as all this is extra work 😓
There was a problem hiding this comment.
Oh wow, so that's actually a thing! https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html#//apple_ref/doc/uid/TP40014097-CH41-ID3 (memberwise init section). That's pretty annoying :)
| @@ -910,6 +910,8 @@ public class PlacesWriteConnection: PlacesReadConnection { | |||
| msg.documentType = documentType.rawValue | |||
| case let .titleObservation(title): | |||
| msg.title = title | |||
| case .unknown: | |||
There was a problem hiding this comment.
Looks like a bug that may be easy to miss - e.g. somehow you end up init-ing HistoryMetadataObservation wrong, then pass it along here, and never notice that nothing actually happens.
Partially why I don't like being able to represent invalid states! E.g. if unknown isn't a thing, you can't pass invalid data into this method at all.
mergify
bot
dismissed
grigoryk’s stale review
The pull request has been modified, dismissing previous reviews.
mergify
bot
dismissed
grigoryk’s stale review
The pull request has been modified, dismissing previous reviews.
Enums and Structs inside a library don't have accessible initializer and require public inits