added Extension for UIDevice #102 #104

thorstenstark · 2016-12-19T22:55:39Z

I'm not sure if the screen sizes fit in this extension, as both use the UIScreen class to get the informations.

brototyp · 2016-12-20T17:15:22Z

I'm not sure if the screen sizes fit in this extension, as both use the UIScreen class to get the informations.

Yeah. I understand that. Have you seen my thought over in the #102 ? Maybe the best idea is to move all those method in a separate Class Device. Therefore we don't fiddle around with an existing class and we can keep the screen size method/property.

thorstenstark · 2016-12-29T15:56:26Z

As mentioned in the #102 I moved these methods to a Context class.

brototyp

Can you squash all commits into one?

brototyp · 2016-12-29T16:33:21Z

PiwikTracker/Context.swift

+        }()
+
+        // Returns the screen size in pixels
+        static let physicalScreenSize: String = {


I would rather like to name it nativeScreenSize to match it with the nativeBounds. For physicalScreenSize I would expect it to be in centimeters or inch.

Good point. Changed it.

mattab · 2016-12-29T20:46:37Z

Can you squash all commits into one?

@brototyp Fyi Github now has this feature to let you squash all commits with the UI. So usually it's not needed to ask contributors to squash. Learn more here: https://github.com/blog/2141-squash-your-commits

brototyp · 2016-12-30T10:41:10Z

Fyi Github now has this feature to let you squash all commits with the UI.

Yeah, right. Forgot that. I rarely use GitHub.

mattiaslevin · 2016-12-30T23:16:31Z

PiwikTracker/Context.swift

@@ -0,0 +1,115 @@
+import UIKit
+
+public class Context {


imho, having nested classes with only static members (all internally available) does not add any value.
We are using namespace to avoid clashes and Device and Application does not share any fileprivate code not visible to the outside.

I would just make a stand-alone Device struct (prefer struct over class, or make class final)

So we make two structs 'Device' and 'Application' not nested in a 'Context'?! What do you think @brototyp ?

The only value nesting both into a "Context" is for the developer. This way we declare both as being some Kind of Context and grouping both together. I am fine with both.

In this case I would go with the stand-alone Device struct.

mattiaslevin · 2016-12-30T23:19:47Z

PiwikTracker/Context.swift

+    public class Device {
+
+        /// The platform name of the device i.e. "iPhone1,1" or "iPad3,6"
+        static let platform: String = {


Using static methods make access to members more easy without passing around an instance.
However it is a bit like having singletons and might bite back if you ever need to mock the class in a unit test.

But I do not think that will be an issue here, so I would keep as is and refactor later the need arise.

Good point. So refactoring wise we should add something like a current singleton which then returns an Object with all the current values. This way we cloud instantiate a Device with mocked values.

I will make a proposal for the variant with the singleton.

mattiaslevin · 2016-12-30T23:20:06Z

PiwikTracker/Context.swift

+        // TODO: implement me
+    }
+
+    public class Device {


I would make this a struct

Consider adding access control information (internal) to the top level type to be consistent with the other PR.
I think this code style guide is pretty good.
https://github.com/raywenderlich/swift-style-guide
https://github.com/github/swift-style-guide

thorstenstark · 2017-01-08T13:38:26Z

PiwikTracker/Context.swift

+            case "iPad6,4":      return "iPad Pro 9.7 (Cellular)"
+            case "iPad6,7":      return "iPad Pro 12.9 (WiFi)"
+            case "iPad6,8":      return "iPad Pro 12.9 (Cellular)"
+


Is the Piwik SDK supposed to run on WatchOS and tvOS, too? Then we should add the proper device names here.

That's a good idea. Yes add them please.

thorstenstark · 2017-01-08T14:14:30Z

I added a version with a current singleton that holds the properties for the current device.

brototyp · 2017-01-08T15:20:51Z

PiwikTracker/Device.swift

+
+    /// A human readable version of the platform name i.e. "iPhone 6 Plus" or "iPad Air 2 (WiFi)"
+    /// Or the platform name in case no human readable name was found.
+    let humanReadablePlatformName: String


The human readable name could be a function that calculates the human readable version from the platform. And it could return an optional that is nil if no human readable version exists.

Agree that returning an optional String is probably a good thing.
If the caller really needs a string it can do

let platformName = device.humanReadablePlatformName ?? device.platform

But that is the callers responsibility not this struct.

brototyp · 2017-01-08T15:21:47Z

PiwikTracker/Device.swift

+    let osVersion: String
+
+    // The screen size in points
+    let screenSize: String


I think the size should be a CGSize.

brototyp · 2017-01-08T15:23:33Z

PiwikTracker/Device.swift

+    private static func currentNativeScreenSize() ->  String {
+        let bounds = UIScreen.main.nativeBounds
+        return "\(bounds.size.width)x\(bounds.size.height)"
+    }


Those static methods with a current prefixed to irritate me a bit. A possible alternative would be a static method on Device that returns a new Device populated with values for the current device. What do you think?

I think a factory method would be great.
Even so, I think it is good practice to write small functions. The methods above does exactly what they promise :)

I changed the method names a bit to remove irritation ;)

I like that!

brototyp · 2017-01-08T15:23:56Z

PiwikTracker/Device.swift

+    let screenSize: String
+
+    // The screen size in pixels
+    let nativeScreenSize: String


This should be a CGSize as well.

minitrue22 · 2017-01-08T16:13:36Z

PiwikTracker/Device.swift

+internal struct Device {
+
+    /// A shared instance holding the properties for the current device
+    static let current:Device = Device(platform: currentPlatform(),


I would suggest removing the static current and have a factory method that returns the current device

static func makeCurrentDevice() -> Device { let platform = currentPlatform() ... return Device(platform: platform, ...) }

I would only create a global shared instance later on if it becomes needed.
Most probably the "tracker class" can have a simple instance variable it can instantiate at construction by calling the static method above (reducing coupling and making testing more simple).

brototyp · 2017-01-14T16:25:23Z

PiwikTracker/Device.swift

+internal struct Device {
+
+    /// Creates an returns a new device object representing the current device
+    static func makeCurrentDevice() ->  Device {


Not overly happy with the word make here but it's not a huge issue.

thorstenstark requested a review from brototyp December 19, 2016 22:55

brototyp added the swift label Dec 20, 2016

brototyp added this to the Swift 3 Rewrite milestone Dec 20, 2016

brototyp reviewed Dec 29, 2016

View reviewed changes

thorstenstark force-pushed the feature/swift-device-extensions branch from 83c18bb to 4b54950 Compare December 30, 2016 18:59

mattiaslevin reviewed Dec 30, 2016

View reviewed changes

thorstenstark commented Jan 8, 2017

View reviewed changes

added Device struct with singleton for current device

da55a53

thorstenstark force-pushed the feature/swift-device-extensions branch from 4b54950 to da55a53 Compare January 8, 2017 14:10

brototyp reviewed Jan 8, 2017

View reviewed changes

minitrue22 reviewed Jan 8, 2017

View reviewed changes

Merge branch 'swift3' into feature/swift-device-extensions

c5d8dfc

thorstenstark force-pushed the feature/swift-device-extensions branch from fa3fbe9 to c5d8dfc Compare January 12, 2017 19:00

Merge branch 'swift3' into feature/swift-device-extensions

c3027cf

brototyp mentioned this pull request Jan 14, 2017

Added missing platform names (Apple TV, iPhone 6s, 6s+, iPod Touch 6G, iPad Mini 4, iPad Pro) #84

Closed

brototyp approved these changes Jan 14, 2017

View reviewed changes

brototyp merged commit d5570bd into matomo-org:swift3 Jan 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added Extension for UIDevice #102 #104

added Extension for UIDevice #102 #104

thorstenstark commented Dec 19, 2016

brototyp commented Dec 20, 2016

thorstenstark commented Dec 29, 2016

brototyp left a comment

brototyp Dec 29, 2016

thorstenstark Dec 30, 2016

mattab commented Dec 29, 2016

brototyp commented Dec 30, 2016

mattiaslevin Dec 30, 2016

thorstenstark Dec 31, 2016

brototyp Dec 31, 2016

thorstenstark Jan 8, 2017

mattiaslevin Dec 30, 2016

brototyp Dec 31, 2016

thorstenstark Jan 8, 2017

mattiaslevin Dec 30, 2016

mattiaslevin Dec 30, 2016 •

edited

thorstenstark Jan 8, 2017

brototyp Jan 8, 2017

thorstenstark commented Jan 8, 2017

brototyp Jan 8, 2017

minitrue22 Jan 8, 2017 •

edited

brototyp Jan 8, 2017

thorstenstark Jan 12, 2017

brototyp Jan 8, 2017

minitrue22 Jan 8, 2017

thorstenstark Jan 12, 2017

brototyp Jan 14, 2017

brototyp Jan 8, 2017

thorstenstark Jan 12, 2017

minitrue22 Jan 8, 2017 •

edited

thorstenstark Jan 12, 2017

brototyp Jan 14, 2017

added Extension for UIDevice #102 #104

added Extension for UIDevice #102 #104

Conversation

thorstenstark commented Dec 19, 2016

brototyp commented Dec 20, 2016

thorstenstark commented Dec 29, 2016

brototyp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattab commented Dec 29, 2016

brototyp commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattiaslevin Dec 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorstenstark commented Jan 8, 2017

Choose a reason for hiding this comment

minitrue22 Jan 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minitrue22 Jan 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattiaslevin Dec 30, 2016 •

edited

minitrue22 Jan 8, 2017 •

edited

minitrue22 Jan 8, 2017 •

edited