Added more expansive orientation support
The current Orientation enum isn't really accurate - there are many types of orientations that iOS supports so I think those should be properly represented in the enum.
| 4 Warnings | |
|---|---|
| :warning: | Plist changed, don’t forget to localize your plist values |
| :warning: | Source/Device.generated.swift#L290 - Function body should span 40 lines or less excluding comments and whitespace: currently spans 63 lines |
| :warning: | Source/Device.generated.swift#L607 - TODOs should be resolved (Longterm we need a better solu…). |
| :warning: | Source/Device.generated.swift#L1149 - Prefer empty collection over optional collection. |
SwiftLint found issues
Warnings
| File | Line | Reason |
|---|---|---|
| Device.generated.swift | 1149 | Prefer empty collection over optional collection. |
| Device.generated.swift | 607 | TODOs should be resolved (Longterm we need a better solu...). |
| Device.generated.swift | 290 | Function body should span 40 lines or less excluding comments and whitespace: currently spans 63 lines |
Generated by :no_entry_sign: Danger
Hello @AdiAyyakad! A few days ago we have finally created the 2.0 release and I have taken over the maintenance of DeviceKit from Dennis. I will take a look at your pull request and discuss it with @Zandor300 - our main contributor - on how to proceed further. Many thanks in advance for your time and effort!
These can also be used for the implementation of isPortrait and isLandscape.
var isPortrait: Bool {
return UIDevice.current.orientation.isPortrait
}
var isLandscape: Bool {
return UIDevice.current.orientation.isLandscape
}
var isFlat: Bool {
return UIDevice.current.orientation.isFlat
}
@Zandor300 So my hesitation with adding those at all is because they're not binary values. Like orientation isn't either portrait, or it isn't. You can be flat, but still have to orient for a portrait view.
https://github.com/AdiAyyakad/TestUIDeviceOrientation
I just whipped up a sample project that listens to orientation changes and it doesn't always report the right value.
@AdiAyyakad Oh yeah! I've installed your project on my iPhone.
What I think we should do, is listen to value changes, and remember what the last option was: Portrait and Landscape. When we call isLandscape, and UIDevice.current.orientation.isFlat == true, instead, we should return what the last known landscape vs portrait value was.
Personally think this should be an opt-in functionality since this isn't relevant for some apps and we shouldn't be wasting unnecessary resources on this. Additionally, the above functionality contradicts the way UIDevice works and DeviceKit is supposed to be a value-type replacement for UIDevice. Not everybody wants .isLandscape == true + .isFlat == true at the same time.
So when you opt-in for this functionality, you can enable the monitoring for the value:
var isOrientationMonitoringEnabled: Bool {
didSet {
if isOrientationMonitoringEnabled {
NotificationCenter.default.addObserver(...)
} else {
NotificationCenter.default.removeObserver(...)
}
}
}
to match UIDevice.current.isBatteryMonitoringEnabled
PS, Or is this making it too complicated? Would like to know @denisenepraunig's opinion on this.
Hmm I see where you're going with this, but yeah to me it just feels like DeviceKit should be more value typed and this might add some extra logic that it doesn't really need. But overall up to y'all!
I have found though that UIApplication.shared.statusBarOrientation.isLandscape is a more consistent API for landscape detection since it's related to the content orientation on the screen rather than the physical device. Maybe we can just use that?
@denisenepraunig and I discussed this and we think we should split up the two types of orientations:
-
.physicalOrientation -
.screenOrientation
That way we reduce confusion and allow the user to choose which one they want/need.
The current variable .orientation would become an alias of .physicalOrientation and then be deprecated to push users to move to one of the two.
I think that would be the best option moving forward. Obviously this is still up for discussion if you come up with a better implementation @AdiAyyakad.
@Zandor300 no that sounds great to me!
@AdiAyyakad Okay, great! Then feel free to implement that.