iOS Ping functionality #602

robmaceachern · 2019-02-22T21:13:37Z

Fixes #463

This adds a rudimentary SCPingUtility that is triggered via the (i) menu -> Ping Cogeco Peer 1. Peer 1 is supposed to supply us with some hostnames to ping but in the meantime I've added three for test purposes (note: cogecopeer1.com does not respond to pings):

("Cogeco Peer 1", "www.cogecopeer1.com"),
("Google", "www.google.com"),
("Yahoo", "www.yahoo.com")

When an entry is selected by the user, we do some lookups: hostname -> IP -> ANS. If the ANS is found and we know about it, we select the node on the map and automatically trigger the ping to the IP found during the lookup process.

@apike I don't know how soon we'll have the peer1 hostnames. If it might be a while, we could rename the menu item from "Ping Cogeco Peer1" to "Ping Popular Hosts" or something else and add a few more hostnames that we choose.

ssawchenko · 2019-02-25T21:10:37Z

Testing it out, when "Cogeco Peer 1" is selected from the Ping Us menu, the node for "Zenedge LLC" is shown.

I'm not sure this is correct, since if you search for Cogeco Peer 1 in the Search window there are multiple ASNs that correspond to that name.

I would assume the Ping for one of those would be what we would want to show? Perhaps it is the case here that the URL www.cogecopeer1.com is being hosted at a location that is being serviced by a different ASN?

robmaceachern · 2019-02-25T21:28:53Z

Perhaps it is the case here that the URL www.cogecopeer1.com is being hosted at a location that is being serviced by a different ASN?

Yeah I've seen that hostname resolve to 152.70.44.18 and 205.147.88.248, both of which are part of the ZENEDGE ASN according to this online lookup tool.

$ nslookup www.cogecopeer1.com
Server:		192.168.1.1
Address:	192.168.1.1#53

Non-authoritative answer:
www.cogecopeer1.com	canonical name = cogecopeer1.com.
Name:	cogecopeer1.com
Address: 152.70.44.18

The current ping behaviour is consistent with what a user would see if they looked up the www.cogecopeer1.com hostname with the 'Find host...' search functionality.

nbrooke

Mostly looks reasonable to me, couple of very minor comments, but they are all pretty judgment-call / optional sort of things, so I don't think merge should be blocked on any of them.

nbrooke · 2019-02-25T21:51:57Z

Library/SCPingUtility.swift

+    @objc var delegate: SCPingUtilityDelegate?
+
+    @objc public var packetRecords: [SCPacketRecord] {
+        let records = icmpUtility.packetRecords as! [SCPacketRecord]


Force cast here seems like it shouldn't be necessary, as the ObjC code is annotated with the right type. Was it maybe from before the annotation as added and never got removed?

Alternatively, is it maybe complaining because it things it might be nullable, and there needs to be a nullable annotation on that as well?

Yeah I was confused by this too. For some reason the generic type isn't bridging to Swift. The interface swift sees is:

var packetRecords: NSMutableArray! { get }

A bit of google searching led me to this Swift bug: Swift does not honor NSMutableArray generics from objective-c classes. I could try the workaround of changing the public interface to NSArray<SCPacketRecord *> as long as nothing else depends on modifying that array.

nbrooke · 2019-02-25T22:01:40Z

iOS/CreditsViewController.swift

@@ -109,22 +109,19 @@ public class CreditsViewController: UIViewController, UIWebViewDelegate {
        doneButton.layer.cornerRadius = doneButton.frame.size.height / 2
        view.addSubview(doneButton)

-        // iPhoneX support
-        if #available(iOS 11.0, *) {


I assume this availbility is going away because the minimum platform is iOS 11 now, It looks like there is one other one that is still there, should maybe remove that as well?

👍

It looks like there is some inconsistency with the target and project ios deployment target settings so I'll try to fix that too.

nbrooke · 2019-02-25T22:07:47Z

iOS/PingLocationsViewController.swift

+        backgroundImageView.leftAnchor.constraint(equalTo: view.leftAnchor).isActive = true
+        backgroundImageView.topAnchor.constraint(equalTo: view.topAnchor).isActive = true
+        backgroundImageView.rightAnchor.constraint(equalTo: view.rightAnchor).isActive = true
+        backgroundImageView.bottomAnchor.constraint(equalTo: view.bottomAnchor).isActive = true


Not sure how important it is, but it looks like there are at least 3 places (maybe more not included in this change) that we do this "turn of autoresizing mask and then pin to the same edges of the superview" thing. Might be worth having a utility function or category to do this as a one liner?

Not sure if it's worth it though, 3 copy-and-pastes is right no the edge of where I'd say it's a no-brainer to have a utility function, so if that's all there is maybe it's not actually worth it.

Other option would be to wait until NEXT time we have to do something like this, and just add SnapKit.

Yeah I would like to bring in Snapkit but I figured we should sort out all the other out of date dependencies (#596) before bringing in a new one. If you feel strongly that this boilerplate-y autolayout code should be improved now, then I can bring it in sooner. Developing our own helpers would be kind of fun but I feel like I'd just end up reimplementing Snapkit's nice interface:

box.snp.makeConstraints { (make) -> Void in make.edges.equalTo(superview) }

😍

…t target

…me calculations

robmaceachern · 2019-02-27T17:57:17Z

@nbrooke let me know what you think about 2bc5783

I can't really tell what's best practice in Obj-C for this. Alternatively we could change the type of the packetRecords instance variable to NSMutableArray but then we need to access it directly when adding items, which felt wrong.

nbrooke

Looks reasonable to me, it's a bit of a shame that it gets so gnarly to expose a reasonable interface for the packet records to Swift, but I think what you've got is a reasonable work around, and if we are going to have annoying warts better to have them in the ObjC code then the Swift code.

🚀

robmaceachern added 8 commits June 20, 2018 16:12

Added ping button and rearranged traceroute button

1d5b4a5

Progress on ping. Refactored some traceroute ui config to be reusued.

3d64a5d

Merge branch 'master' into rnm/463-ping

5edb20c

Started working on adding the ping locations view

6a1ae1c

Progress on ping functionality

0cfb7ec

Automatically start ping after selection

b080cea

Cleanup

b268177

Improve delegate method name

c40c835

robmaceachern requested review from nbrooke and ssawchenko February 22, 2019 21:13

apike self-assigned this Feb 24, 2019

nbrooke approved these changes Feb 25, 2019

View reviewed changes

apike mentioned this pull request Feb 26, 2019

Make transition from node selection to pinging smoother #603

Open

apike assigned robmaceachern and unassigned apike Feb 26, 2019

robmaceachern added 5 commits February 27, 2019 09:13

Remove target-specific deployment target and update project deploymen…

688c6a2

…t target

Update credits.html

0553482

Remove unnecessary availability check and remove some unnecessary fra…

7fea593

…me calculations

Remove CarPlay, Mac, and Apple Watch icon entries

c2fa693

Expose immutable array of packet records

2bc5783

robmaceachern requested a review from nbrooke February 27, 2019 17:53

nbrooke approved these changes Feb 27, 2019

View reviewed changes

robmaceachern merged commit d49d746 into master Feb 27, 2019

robmaceachern deleted the rnm/463-ping branch February 27, 2019 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS Ping functionality #602

iOS Ping functionality #602

robmaceachern commented Feb 22, 2019

ssawchenko commented Feb 25, 2019

robmaceachern commented Feb 25, 2019

nbrooke left a comment

nbrooke Feb 25, 2019

robmaceachern Feb 27, 2019

nbrooke Feb 25, 2019

robmaceachern Feb 27, 2019

nbrooke Feb 25, 2019

robmaceachern Feb 27, 2019

robmaceachern commented Feb 27, 2019

nbrooke left a comment

iOS Ping functionality #602

iOS Ping functionality #602

Conversation

robmaceachern commented Feb 22, 2019

ssawchenko commented Feb 25, 2019

robmaceachern commented Feb 25, 2019

nbrooke left a comment

Choose a reason for hiding this comment

nbrooke Feb 25, 2019

Choose a reason for hiding this comment

robmaceachern Feb 27, 2019

Choose a reason for hiding this comment

nbrooke Feb 25, 2019

Choose a reason for hiding this comment

robmaceachern Feb 27, 2019

Choose a reason for hiding this comment

nbrooke Feb 25, 2019

Choose a reason for hiding this comment

robmaceachern Feb 27, 2019

Choose a reason for hiding this comment

robmaceachern commented Feb 27, 2019

nbrooke left a comment

Choose a reason for hiding this comment