DeviceKit icon indicating copy to clipboard operation
DeviceKit copied to clipboard

Added more expansive orientation support

Open AdiAyyakad opened this issue 7 years ago • 10 comments

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.

AdiAyyakad avatar Dec 13 '18 14:12 AdiAyyakad

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

devicekit-danger-bot avatar Dec 13 '18 14:12 devicekit-danger-bot

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!

denisenepraunig avatar Apr 12 '19 14:04 denisenepraunig

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 avatar Apr 15 '19 09:04 Zandor300

@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 avatar Apr 15 '19 12:04 AdiAyyakad

@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.

Zandor300 avatar Apr 15 '19 14:04 Zandor300

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!

AdiAyyakad avatar Apr 15 '19 21:04 AdiAyyakad

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?

AdiAyyakad avatar Apr 15 '19 21:04 AdiAyyakad

@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 avatar May 02 '19 15:05 Zandor300

@Zandor300 no that sounds great to me!

AdiAyyakad avatar May 07 '19 17:05 AdiAyyakad

@AdiAyyakad Okay, great! Then feel free to implement that.

Zandor300 avatar May 07 '19 17:05 Zandor300