Results history, Adaptive UI for Test runner, Settings Screen #31

deepumukundan · 2020-05-08T02:30:14Z

This PR covers a lion's share of #30.

I redid the Test Runner UI based on the previous one, mainly changing it to adapt to different screen sizes and also potentially for future Catalyst install.
Added a Settings screen as well to configure some extra functionalities (Maybe the Help button can move here in future).
Added localization support for the screens (Should fix Localization #29 and Localize the app #19)
Some perks like test success sounds and haptics

Things which are pending (The above changes were already overwhelming, so wanted to break things out)

Actual persistence to disk (TBD, Currently just saves test results to in-memory array)
Sort/Filtering/Delete for results (Depends on 1)

- Remove old controller - localize the app - Reuse results view - Fix some minor bugs

deepumukundan · 2020-05-08T02:34:16Z

Source/Backend/IPFTestRunnerConfiguration.h

+@property (nonatomic, copy) NSString *hostname;
+@property (nonatomic) NSUInteger port;
+@property (nonatomic) NSUInteger duration;
+@property (nonatomic) NSUInteger streams;
+@property (nonatomic) IPFTestRunnerConfigurationType type;


These were made read-write so it can be easily configured from the view. The test runner class still gets a read only copy of it later when the test starts, so this change shouldn't be an issue. Let me know if you think otherwise

These are generated remodel value files:
https://github.com/facebook/remodel

They are immutable but have builders so you can mutate or create them. It is a bit of a hassle, and probably overkill for this little app, but I like the concept, and they're easily serializable for persistence too!

If you'd like to make changes, I would modify IPFTestRunnerConfiguration.value and then run make in that folder (you will have to install remodel via npm first if I remember correctly).

Remodel seems to only generate immutable structures. With the form mutating the structure now whenever one of the configurations is tweaked (in the closures like onTextChange and onSegmentChange), it needs to be mutable. Agree that immutable models reduce future bugs, but since this is a config which is then passed into Test Runner (And screen is not editable while a test is running), I don't think there is a huge value add in going the remodel way. If serialization is a consideration for future, we should convert this to swift and make it Codable, so it can be easily converted using a JSONEncoder or PropertyListEncoder for storage.

deepumukundan · 2020-05-08T02:36:08Z

Source/UI/Miscellaneous/IPFSettingsViewController.swift

+            view.backgroundColor = .white
+        }
+
+        form


Form and FormViewController are from a very light weight forms approach I created for building the UI). Its added under the UI/Support section of the project

If these things are only used once I would prefer not to pull in extra code, and the style might look esoteric to other contributors, so I would stick to vanilla UI code here: can we just instantiate SwitchRow and Section instances, configure them and add them to the UI tree?

I switched the API to be non esoteric. Rows are added now via a standard function call

deepumukundan · 2020-05-08T02:40:20Z

Source/UI/Test Results/IPFTestResultsManager.swift

+    var results = [IPFTestResult]()
+
+    func add(_ result: IPFTestResult) {
+        results.insert(result, at: 0)


Currently just saving the results in-memory. I would like to use Core Data to allow a more robust and scalable persistence, and maybe iCloud sync using NSCloudPersistentContainer (iOS 13+). Majority of the devices should be on iOS 13 and plus. To use even NSPersistenContainer, we might have to drop iOS 9 from supported devices. Even for testing the UI currently, I could only find iOS 10.3.1 simulator embedded in Xcode. Let me know what you think on this one

Yep, I've been a bit aggressive with supporting old iOS versions, I'll need to check what device usage looks like to know if iOS 9 can go or not. For persistence, I like the idea of iCloud syncing but we'd need to introduce a device tag to know which one made the individual tests, make sure they don't step on each other's toes...

Ideally we'd have an append-only file synced over iCloud, that way we reduce the possibility of conflicts. And given tests really are read only (you only ever add them or delete them) we could completely eliminate failure cases there. I remember there was an iCloud documents API, but I forgot exactly what that is called.

All of that would involve a fair bit of complexity, so just keeping the backing store a dumb local plist for now would probably be best.

Considering the backwards compatibility the app needs, have used a dumb backing store for now. I used json though, based on some performance tests and data sizes. Have added some database tests in the test target but below is a summary of the results.

Large Dataset test: 10000 results
JSON advantages over Plist
File Size: 1.1MB vs 3.1MB - 1/3rd the file size
Save: 0.009seconds vs 0.018seconds - Half the time to save
Read: 0.086 vs 0.1 seconds - 14% faster read

Also JSON is more portable and understood by non-Apple platforms.
If you still want to keep Plist encoding instead, it is a small change and we just have to switch out the encoder-decoders.

Source/UI/Test Results/IPFTestResultsView.swift

Source/UI/Test Runner/IPFTestRunnerViewController.swift

deepumukundan · 2020-05-08T02:44:15Z

iperf.xcodeproj/project.pbxproj

 				INFOPLIST_FILE = "$(SRCROOT)/Resources/Info.plist";
 				IPHONEOS_DEPLOYMENT_TARGET = 9.3;
 				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks";
-				PRODUCT_BUNDLE_IDENTIFIER = com.teapotapps.iperf;
+				PRODUCT_BUNDLE_IDENTIFIER = com.deepumukundan.iperf;


I can revert these once the PR is fully reviewed. This helps me test in device quickly

ndfred · 2020-05-08T07:04:37Z

Thanks for putting this up! Just want to make it clear it might take me a bit to review all of it, and specifically make sure the performance of the tests are not affected by the UI. This means a bit of profiling and profile-guided optimisations, watching Switch / Objective-C inter and so on. I also want to make sure we stay compatible with iOS as far as reasonable, so will need to review all the UIKit APIs we are using here.

ndfred

Thanks for your contribution! My overall impression is that that's quite a big chunk of code for just two screens, and we might be going a bit too generic on the forms. Plus I would rather move everything to Swift (except the thin Objective-C iPerf library interface) if we are going down that path.

As I mentioned before performance is also a big deal, so reducing the amount of code will help reduce the overhead on startup and during tests, I would love to see a before and after of binary size with all these changes.

Finally I try and have everything be defined by code so we don't get stuck having to redo all the assets should a @4x screen density show up, so the assets would have to be produced or rendered directly through CoreGraphics. Same for the remodel class, I would suggest reading up on how they work and embrace that way of doing things rather than exposing setters, and using remodel objects to serialism and persist data.

ndfred · 2020-05-08T07:17:46Z

Source/Backend/IPFTestRunnerConfiguration.h

+@property (nonatomic, copy) NSString *hostname;
+@property (nonatomic) NSUInteger port;
+@property (nonatomic) NSUInteger duration;
+@property (nonatomic) NSUInteger streams;
+@property (nonatomic) IPFTestRunnerConfigurationType type;


These are generated remodel value files:
https://github.com/facebook/remodel

They are immutable but have builders so you can mutate or create them. It is a bit of a hassle, and probably overkill for this little app, but I like the concept, and they're easily serializable for persistence too!

If you'd like to make changes, I would modify IPFTestRunnerConfiguration.value and then run make in that folder (you will have to install remodel via npm first if I remember correctly).

ndfred · 2020-05-08T07:20:28Z

Resources/Assets.xcassets/Results.imageset/Contents.json

+      "scale" : "2x"
+    },
+    {
+      "filename" : "Results.png",


For images, I try and generate them with CoreGraphics so we don't have to embed lots of files and are always pixel perfect regardless of screen density, look at IPFIconGenerator.m as an example. I also tend to reuse system icons as much as possible to go for a vanilla look, that way there is less maintenance for OS updates.

I have replaced these ones with 3 pdf versions of system icons provided by SFSymbols.
Specifically used this tool to export 3 icons which seemed to match
https://github.com/davedelong/sfsymbols
Vector pdf's will scale to any size and have been around since Xcode 6, so should be backwards compatible to all OS versions supported by iPerf

Resources/LaunchScreen.storyboard

Resources/iperf-Bridging-Header.h

Source/AppDelegate.m

Source/UI/Miscellaneous/IPFHelpViewController.m

ndfred · 2020-05-08T07:30:10Z

Source/UI/Miscellaneous/IPFSettingsViewController.swift

+            view.backgroundColor = .white
+        }
+
+        form


If these things are only used once I would prefer not to pull in extra code, and the style might look esoteric to other contributors, so I would stick to vanilla UI code here: can we just instantiate SwitchRow and Section instances, configure them and add them to the UI tree?

Source/UI/Support/UserDefaultsExtension.swift

ndfred · 2020-05-08T07:34:00Z

Source/UI/Support/UserDefaultsExtension.swift

+    case enableSounds
+}
+
+public extension UserDefaults {


I would rather have our own configuration object that then knows how to persist itself than have a tight coupling on UserDefaults (and that class would just talk to NSUserDefaults). What if we want to move to something else in the future? Testing would also be simpler that way.

How about we split this into another PR? I think the current PR as its own is huge.

Also this needs a little bit more hashing out. If we make the current IPFTestRunnerConfiguration into a swift Codable struct, we can get away with having to have tight coupling with user defaults and serialize and save the whole config in any way (JSON, Plist, UserDefaults, or something else). And even save the user's preference settings in there as those are also kind of a configuration for the test. But this will mean not using remodel.

Let me know

Source/UI/Test Results/IPFTestResult.swift

deepumukundan · 2020-07-17T06:21:04Z

Got busy with work and life. I have fixed some of the comments above. Please review and resolve the ones which seem good. Will take a look at the pending ones once I have some time

deepumukundan · 2020-07-19T23:06:09Z

Most comments taken care of. Please check and let me know. I am pausing for a bit for you to review but in my local usage and tests everything looks good.

ndfred · 2020-07-20T04:43:36Z

Thanks for keeping at it! It might take a bit for me to go through this again, and I also have a pending iperf library update that I need to fix a couple bugs in, so please don't take it personally if this languishes a bit. What do you think about taking this as a separate branch until it all settles down?

deepumukundan · 2020-07-20T05:48:49Z

Thanks for keeping at it! It might take a bit for me to go through this again, and I also have a pending iperf library update that I need to fix a couple bugs in, so please don't take it personally if this languishes a bit. What do you think about taking this as a separate branch until it all settles down?

No worries. Take your time. It is serving my current needs of saving results and being able to track speeds over time, so all is good.

Since my changes are on the fork, it should be as good as being in another branch right?

deepumukundan · 2021-02-01T07:07:18Z

@ndfred Updated with latest master.

deepumukundan · 2021-05-17T21:53:12Z

@ndfred Just checking back. Let me know if this is needed anymore or if long term plans for the UI have changed.

deepumukundan added 15 commits May 6, 2020 14:06

Allow test results to be displayed in a new tab

0fef061

Tweaks after using it a bit

6570c24

Reorganize code, prep for localization

9e34cb8

Localize current strings

112f868

Haptics and sound feedback

36b0091

Initial rewrite of Test runner UI in Swift

a12054e

Add test results view, add settings screen

a9fe11e

Multiple cleanup

02aac21

- Remove old controller - localize the app - Reuse results view - Fix some minor bugs

UI tweaks for small and large dynamic types

1dd9df9

Complete localization

cd34023

Lint and format

2e3acc6

Minor method cleanup

9b5133c

Fixing some issues in iOS 10

31eb9ab

Fix some bugs found during testing

649e4cb

Remove file headers to match current project structure

9591ba9

deepumukundan commented May 8, 2020

View reviewed changes

ndfred requested changes May 8, 2020

View reviewed changes

deepumukundan added 5 commits July 15, 2020 23:15

Fix a bug, UI tweaks

80365a6

Replace objective-c AppDelegate with Swift

2fcac00

Make form API non-esoteric

4198fcc

Replace Help View controller with Swift version

15d4562

Replace tab bar icons with pdf versions for future proofing

2caf195

deepumukundan requested a review from ndfred July 17, 2020 06:21

deepumukundan added 5 commits July 18, 2020 11:17

Make IPFTestResult struct and coddle to serialize via property list

4b71438

Switch store from singleton to protocol

a1f4ec0

Initial implementation of decode and encode to property list

fbc0504

Change Settings to non-esoteric code

a5fa3b9

Minor cleanup

0246621

deepumukundan added 6 commits July 18, 2020 23:09

Test the store performance and measure metrics

c76da7c

Fix window getting deallocated, make store private

b1c9af2

Add a delete all results functionality

576ee90

Add swipe to delete functionality

53d0691

Export data as proper text, render as icon

e4a1282

Renamed methods

5d02009

Merge remote-tracking branch 'upstream/master'

420270f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Results history, Adaptive UI for Test runner, Settings Screen #31

Results history, Adaptive UI for Test runner, Settings Screen #31

deepumukundan commented May 8, 2020

deepumukundan May 8, 2020

ndfred May 8, 2020

deepumukundan Jul 17, 2020

deepumukundan May 8, 2020

ndfred May 8, 2020

deepumukundan Jul 17, 2020

deepumukundan May 8, 2020

ndfred May 8, 2020

deepumukundan Jul 19, 2020 •

edited

Loading

deepumukundan May 8, 2020

ndfred commented May 8, 2020

ndfred left a comment

ndfred May 8, 2020

ndfred May 8, 2020

deepumukundan Jul 17, 2020

ndfred May 8, 2020

ndfred May 8, 2020

deepumukundan Jul 19, 2020

deepumukundan commented Jul 17, 2020

deepumukundan commented Jul 19, 2020

ndfred commented Jul 20, 2020

deepumukundan commented Jul 20, 2020

deepumukundan commented Feb 1, 2021

deepumukundan commented May 17, 2021

Results history, Adaptive UI for Test runner, Settings Screen #31

Are you sure you want to change the base?

Results history, Adaptive UI for Test runner, Settings Screen #31

Conversation

deepumukundan commented May 8, 2020

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

deepumukundan Jul 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndfred commented May 8, 2020

ndfred left a 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

deepumukundan commented Jul 17, 2020

deepumukundan commented Jul 19, 2020

ndfred commented Jul 20, 2020

deepumukundan commented Jul 20, 2020

deepumukundan commented Feb 1, 2021

deepumukundan commented May 17, 2021

deepumukundan Jul 19, 2020 •

edited

Loading