Show QR Code scanner as push view, not modal (#782) #789

ghost · 2018-06-29T12:57:23Z

@vikmeup , referencing #782 issue. I have started to alter the code. I have moved logic for creating and navigating QR Code scanner from SendViewController to SendCoordinator, since it seems to me, it is coordinators responsibility, and then having a controller reference, I update it using a new dedicated function.

I will proceed to implement this change in all the other places. If you have any comments, feel free.

Edit: I could not test a browser, because it is not working right now.

BlackDuckCoPilot · 2018-06-29T13:20:34Z

Black Duck Security Report

Merging #789 into master will not change security risk.

Click here to see full report

OlegGordiichuk · 2018-06-29T14:06:33Z

@BloodyPixy could you pls fix lint warnings.

ghost · 2018-06-29T14:17:41Z

@OlegGordiichuk , I am not done yet.

ghost · 2018-06-29T14:46:25Z

@OlegGordiichuk , @vikmeup , I have more or less finalized this. In some cases, this is impossible to handle via coordinator, as there in no any.

vikmeup · 2018-06-29T15:11:58Z

@BloodyPixy You gotta build a generic solution to push coordinator, I can work on this later, but there is an example on the internet how it's done, it removes reference to the navigation.

vikmeup · 2018-06-29T15:14:00Z

@BloodyPixy take a look at this: https://irace.me/navigation-coordinators, we only need logic for pushCoordinator() make it as protocol?

* master: Remove explore collectibles Add gesture handle for words Move navigation bar on pushing backup Version Bump Version Bump Version Bump Version Bump Change copy Make private key image view fixed size Disable import from file Improve backup phrase expirience Revert to use previos old provider version Add CFBundleAllowMixedLocalizations Rename to Backup Phrase Add skip functionality

ghost · 2018-07-01T08:05:07Z

@vikmeup , check new commits. I believe that's much better and can be applied to all coordinators.

vikmeup · 2018-07-01T09:03:57Z

@BloodyPixy Nice work! I will take another look tomorrow!

vikmeup · 2018-07-01T09:02:06Z

Trust/UI/NavigationController.swift

+    }
+}
+
+protocol RootViewControllerProvider: class {


can you move into separate file?

Even in two

vikmeup

Are you sure that's correct way to solve it?

I think coordinator need to conform instead, so you can push coordinator from the coordinator! It should not be navigation controller

vikmeup · 2018-07-01T09:13:03Z

Trust/Tokens/ViewControllers/NewTokenViewController.swift

+extension NewTokenViewController: ScanQRCodeCoordinatorDelegate {
+    func didCancel(in coordinator: ScanQRCodeCoordinator) {
+        navigationController?.setNavigationBarHidden(false, animated: false)
+        (navigationController as? NavigationController)?.popCoordinator(coordinator)


navigationController should be NavigationController in here?

Not all of our screens that present QR Scanner are coordinators, instead they are simple controllers of MVC patterns. They don't have a way to simply add a coordinator somewhere, so in order to create a unified interface for pushing coordinators without extra work on each file, this looks like a solution:
Now NavigationController now has a property viewControllersToChildCoordinators to hold a reference to coordinators, that could be cleaned up by ARC otherwise.

Take a look at the link I shared, it pushed coordinator from the coordinator itself

Maybe we could make a quick call somewhere?

@vikmeup I know what you are talking about, but in our case, we don't always have a coordinator. Take a look on how NewTokenViewController & ImportWalletViewController are created. They don't have coordinators, but somehow, they need to present the QR Scanner. So it seems to me as a viable solution. Please, let me know if I am missing something.

View controllers can call delegate to the coordinator and you push from the coorditor then, that would be the right way to do it!

I agree. On it.

Awesome, this is gonna be a big improvement to our codebase once it’s done

* master: Change background queue to userInterective when generating QR. (#793) Add market price label to token view Add Retry button for collectibles Version Bump

…oller

…toolbar.

vikmeup · 2018-07-02T09:37:56Z

Trust/Browser/Coordinators/BrowserCoordinator.swift

@@ -41,6 +41,9 @@ class BrowserCoordinator: NSObject, Coordinator {
        controller.delegate = self
        return controller
    }()
+    var providedRootController: UIViewController {


why providedRootController? just call it rootViewController?

+, it intersected with the existing name of controllers in some places, but I will just change them as well.

vikmeup · 2018-07-02T09:38:41Z

Trust/Foundation/PushableCoordinator.swift

+        }
+    }
+    func popCoordinator(_ coordinator: RootCoordinator, navigationBarHidden: Bool = false) {
+        if let presentedNVC = providedRootController.presentedViewController as? UINavigationController {


why do you need to pop? it should be done automatically once view is closed

OlegGordiichuk · 2018-07-03T17:35:53Z

@BloodyPixy could you pls resolve conflicts?

This reverts commit 0224982.

* master: (24 commits) use WallInfo all over Add WalletInfo and WalletObject factory Remove mnemonic localization Rename mnemonic to phrase Add GoChain support Add wallet name Add import files. Enabled iCloud entitlements Fix navigation bar items positioning in browser on iPhone X (#796) Upgrade Eureka to 4.2.0 Add config.yml (#795) Use R.string Version Bump Remove unused items Remove GoogleService-Info.plist Remove Lokalise framework Remove header image on import wallet Remove an extra checking Remove duplicate value Remove extra localization key Use R module to fetch localized strings ... # Conflicts: # Trust/Browser/Coordinators/BrowserCoordinator.swift # Trust/Wallet/ViewControllers/ImportWalletViewController.swift

* master: (24 commits) use WallInfo all over Add WalletInfo and WalletObject factory Remove mnemonic localization Rename mnemonic to phrase Add GoChain support Add wallet name Add import files. Enabled iCloud entitlements Fix navigation bar items positioning in browser on iPhone X (#796) Upgrade Eureka to 4.2.0 Add config.yml (#795) Use R.string Version Bump Remove unused items Remove GoogleService-Info.plist Remove Lokalise framework Remove header image on import wallet Remove an extra checking Remove duplicate value Remove extra localization key Use R module to fetch localized strings ... # Conflicts: # Trust/Browser/Coordinators/BrowserCoordinator.swift # Trust/Wallet/Coordinators/WalletCoordinator.swift # Trust/Wallet/ViewControllers/ImportWalletViewController.swift

…-wallet-ios into bug/push-qr-code-reader * 'bug/push-qr-code-reader' of github.com:BloodyPixy/trust-wallet-ios: Remove trailing whitespace Fix for merge issues Revert "Removed navigation logic for now" Removed navigation logic for now # Conflicts: # Trust/Browser/Coordinators/BrowserCoordinator.swift # Trust/UI/NavigationController.swift # Trust/Wallet/ViewControllers/ImportWalletViewController.swift

trust-bot · 2018-07-03T22:26:07Z

SwiftLint found issues

Warnings

File	Line	Reason
InCoordinator.swift	75	Function body should span 80 lines or less excluding comments and whitespace: currently spans 85 lines

Generated by 🚫 Danger

ghost · 2018-07-07T09:56:21Z

Closing this PR, as there was a lot of discussion and changes involved. Creating new branch for this issue.

Show QR Code scanner as push view, not modal (#782)

93b3259

Clean up whitespaces.

36923b0

taraspasichnyk added 2 commits June 29, 2018 17:40

Change present to push to Send and Import wallet. (#782)

456e364

Add Push instead on present for NewToken (#782)

d03c5ee

taraspasichnyk added 2 commits July 1, 2018 10:05

Improve pushing coordinators logic

8dd627d

Fix whitespaces

5f32083

vikmeup reviewed Jul 1, 2018

View reviewed changes

taraspasichnyk added 2 commits July 1, 2018 12:33

Separate files for new protocols

76474fa

Merge branch 'master' into bug/push-qr-code-reader

b78c42a

* master: Change background queue to userInterective when generating QR. (#793) Add market price label to token view Add Retry button for collectibles Version Bump

vikmeup mentioned this pull request Jul 2, 2018

Custom token #772

Closed

Taras Pasichnyk added 5 commits July 2, 2018 11:05

Modify push logic to work on Coordinators instead of Navigation Contr…

e09138a

…oller

Rename protocol property for PushableCoordinator

7a85d49

Add pushable QR Scanner to browser

0f680c1

Add push QR Scanner to ImportWallet functionality

849d51b

Remove Navigation Controller protocol conformance. Add logic to hide …

d51c91e

…toolbar.

vikmeup reviewed Jul 2, 2018

View reviewed changes

taraspasichnyk added 2 commits July 2, 2018 13:29

Change naming to rootViewController. Added logic to NavigationController

7efead7

Removed navigation logic for now

0224982

taraspasichnyk added 7 commits July 3, 2018 21:13

Revert "Removed navigation logic for now"

c84e3a7

This reverts commit 0224982.

Fix for merge issues

0194f3d

Remove trailing whitespace

43dd08f

Add logic for pushing coordinators

33546b8

ghost closed this Jul 7, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show QR Code scanner as push view, not modal (#782) #789

Show QR Code scanner as push view, not modal (#782) #789

ghost commented Jun 29, 2018 •

edited by ghost

BlackDuckCoPilot commented Jun 29, 2018 •

edited

OlegGordiichuk commented Jun 29, 2018

ghost commented Jun 29, 2018

ghost commented Jun 29, 2018

vikmeup commented Jun 29, 2018 •

edited

vikmeup commented Jun 29, 2018

ghost commented Jul 1, 2018

vikmeup commented Jul 1, 2018

vikmeup Jul 1, 2018

ghost Jul 1, 2018

vikmeup left a comment

vikmeup Jul 1, 2018

ghost Jul 1, 2018

vikmeup Jul 1, 2018

ghost Jul 1, 2018

ghost Jul 1, 2018

vikmeup Jul 1, 2018

ghost Jul 1, 2018

vikmeup Jul 1, 2018

vikmeup Jul 2, 2018

ghost Jul 2, 2018

vikmeup Jul 2, 2018

OlegGordiichuk commented Jul 3, 2018

trust-bot commented Jul 3, 2018

ghost commented Jul 7, 2018

Show QR Code scanner as push view, not modal (#782) #789

Show QR Code scanner as push view, not modal (#782) #789

Conversation

ghost commented Jun 29, 2018 • edited by ghost

BlackDuckCoPilot commented Jun 29, 2018 • edited

Black Duck Security Report

OlegGordiichuk commented Jun 29, 2018

ghost commented Jun 29, 2018

ghost commented Jun 29, 2018

vikmeup commented Jun 29, 2018 • edited

vikmeup commented Jun 29, 2018

ghost commented Jul 1, 2018

vikmeup commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikmeup 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

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

OlegGordiichuk commented Jul 3, 2018

trust-bot commented Jul 3, 2018

SwiftLint found issues

Warnings

ghost commented Jul 7, 2018

ghost commented Jun 29, 2018 •

edited by ghost

BlackDuckCoPilot commented Jun 29, 2018 •

edited

vikmeup commented Jun 29, 2018 •

edited