[CIS-528] Add connection status banner to DemoApp #970

dmigach · 2021-04-09T07:34:32Z

Mostly this change is about:

Adding a view for displaying the banner
Fixing an issue with internetIsOfflineError that prevented web socket client from reconnecting
Fixing URLSession configuration to make reconnection logic work
Implementing banner logic in DemoApp
Rehauling navigation in DemoApp a bit to support presenting banner on any screen

codecov · 2021-04-09T07:41:11Z

Codecov Report

Merging #970 (bfa4aca) into main (d8f43bb) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head bfa4aca differs from pull request most recent head bbe17ae. Consider uploading reports for the commit bbe17ae to get more accurate results

@@            Coverage Diff             @@
##             main     #970      +/-   ##
==========================================
+ Coverage   89.62%   89.68%   +0.06%     
==========================================
  Files         210      211       +1     
  Lines        8739     8772      +33     
==========================================
+ Hits         7832     7867      +35     
+ Misses        907      905       -2

Flag	Coverage Δ
llc-tests	`89.68% <100.00%> (+0.06%)`	⬆️
llc-tests-ios12	`85.88% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
Sources/StreamChat/ChatClient.swift	`96.58% <100.00%> (ø)`
...nternetConnection/Error+InternetNotAvailable.swift	`100.00% <100.00%> (ø)`
Sources/StreamChat/Utils/Cached.swift	`76.47% <0.00%> (-5.89%)`	⬇️
Sources/StreamChat/Database/DTOs/MessageDTO.swift	`95.44% <0.00%> (-0.24%)`	⬇️
Sources/StreamChat/Models/Channel.swift	`73.91% <0.00%> (ø)`
Sources/StreamChat/Models/Message.swift	`88.09% <0.00%> (ø)`
Sources/StreamChat/Utils/CoreDataLazy.swift	`87.50% <0.00%> (ø)`
Sources/StreamChat/Database/DTOs/ChannelDTO.swift	`98.95% <0.00%> (+<0.01%)`	⬆️
...ocketClient/Engine/URLSessionWebSocketEngine.swift	`53.48% <0.00%> (+16.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc0c534...bbe17ae. Read the comment docs.

dmigach · 2021-04-09T07:50:21Z

DemoApp/DemoAppCoordinator.swift

+import UIKit
+
+final class DemoAppCoordinator {
+    static var shared: DemoAppCoordinator!


It is this ugly for several reasons:

Initial nav controller lives in a storyboard

LoginViewController needs that to present Chat

We throw out LogicViewController as soon as we present Chat

If it's ugly why don't you fix it? 🤔 🤷‍♂️

What I meant is that we don't have much choice here, so it's ugly comparing to conventional coordinators.
Or do you mean fixing the reasons I enumerated? I believe you mentioned something about storyboard being used on purpose

Reworked it

VojtaStavik

Looks good 👍 I left a couple of comments and questions...

VojtaStavik · 2021-04-13T10:42:50Z

Sources/StreamChat/ChatClient.swift

-        config.waitsForConnectivity = true
+        config.waitsForConnectivity = false


This change should be covered by a test since it's a requirement for the system to work correctly.

Added a test

VojtaStavik · 2021-04-13T10:44:31Z

WHAT? Almost all my comments disappeared 😭

VojtaStavik · 2021-04-13T10:46:07Z

DemoApp/BannerView.swift

+    override func didMoveToSuperview() {
+        super.didMoveToSuperview()
+        setUpLayout()
+        defaultAppearance()
+    }


This a pattern from the SDK and I don't think we should use it here. The SDK for example takes care of the fact that this is called just once, which here doesn't happen. I think it's better to call these methods from a method which is guaranteed to be called just once.

Now it's called once from init

VojtaStavik · 2021-04-13T10:46:29Z

DemoApp/BannerView.swift

+
+        addSubview(label)
+
+        translatesAutoresizingMaskIntoConstraints = false


The view's superview should decide about this, not the view itself.

VojtaStavik · 2021-04-13T10:47:07Z

DemoApp/BannerShowingConnectionDelegate.swift

+    func attachToTopViewIfNeeded() {
+        guard let topView = navigationController.topViewController?.view,
+              topView != bannerView.superview else { return }
+
+        topView.addSubview(bannerView)
+
+        let guide = topView.safeAreaLayoutGuide
+        NSLayoutConstraint.activate(
+            [
+                bannerView.topAnchor.constraint(equalTo: guide.topAnchor)
+            ]
+        )
+    }


Rather than messing up with the view, can't we just add the banner to the navbar view?

Now it works like this. I was having doubts on this approach when I first considered how to implement it because it is based on an assumption that navbar has clipsToBounds == false and also that it has the highest Z position, but probably it should be fine for the DemoApp. Anyway it's easy to rework if there will be issues

VojtaStavik · 2021-04-13T10:47:31Z

DemoApp/DemoAppCoordinator.swift

+import UIKit
+
+final class DemoAppCoordinator {
+    static var shared: DemoAppCoordinator!


If it's ugly why don't you fix it? 🤔 🤷‍♂️

VojtaStavik

Great 🎉

…o make reconnection logic work

…er on any screen

VojtaStavik · 2021-04-15T07:11:46Z

@dmigach please merge when you're ready 👍

dmigach commented Apr 9, 2021

View reviewed changes

dmigach force-pushed the CIS-528-connection-status-banner branch from 63922f4 to 4a0b0a3 Compare April 9, 2021 07:52

VojtaStavik changed the title ~~WIP CIS-528 Add connection status banner to DemoApp~~ [CIS-528] Add connection status banner to DemoApp Apr 9, 2021

b-onc marked this pull request as draft April 9, 2021 08:52

dmigach force-pushed the CIS-528-connection-status-banner branch from 90c45a1 to a870c06 Compare April 13, 2021 08:06

dmigach marked this pull request as ready for review April 13, 2021 08:08

dmigach requested a review from VojtaStavik April 13, 2021 08:08

dmigach force-pushed the CIS-528-connection-status-banner branch 2 times, most recently from 4e9f08c to 1fc3ad6 Compare April 13, 2021 09:22

VojtaStavik suggested changes Apr 13, 2021

View reviewed changes

VojtaStavik reviewed Apr 13, 2021

View reviewed changes

VojtaStavik approved these changes Apr 14, 2021

View reviewed changes

dmigach added 4 commits April 14, 2021 19:06

Fix logic determining whether an error is isInternetIsOfflien error t…

004d074

…o make reconnection logic work

Add a view for displaying reconnection banner

e6bb286

Add logic to react on connectivity events with banner

8247271

Rehaul DemoApp navigation so it is possible to show reconnection bann…

bbe17ae

…er on any screen

dmigach force-pushed the CIS-528-connection-status-banner branch from bfa4aca to bbe17ae Compare April 14, 2021 16:07

dmigach merged commit 123223f into main Apr 15, 2021

dmigach deleted the CIS-528-connection-status-banner branch April 15, 2021 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIS-528] Add connection status banner to DemoApp #970

[CIS-528] Add connection status banner to DemoApp #970

dmigach commented Apr 9, 2021 •

edited

Loading

codecov bot commented Apr 9, 2021 •

edited

Loading

dmigach Apr 9, 2021 •

edited

Loading

VojtaStavik Apr 13, 2021

dmigach Apr 13, 2021 •

edited

Loading

dmigach Apr 14, 2021

VojtaStavik left a comment

VojtaStavik Apr 13, 2021

dmigach Apr 14, 2021

VojtaStavik commented Apr 13, 2021

VojtaStavik Apr 13, 2021

dmigach Apr 14, 2021

VojtaStavik Apr 13, 2021

dmigach Apr 14, 2021

VojtaStavik Apr 13, 2021

dmigach Apr 14, 2021

VojtaStavik Apr 13, 2021

VojtaStavik left a comment

VojtaStavik commented Apr 15, 2021

		config.waitsForConnectivity = true
		config.waitsForConnectivity = false


		addSubview(label)

		translatesAutoresizingMaskIntoConstraints = false

[CIS-528] Add connection status banner to DemoApp #970

[CIS-528] Add connection status banner to DemoApp #970

Conversation

dmigach commented Apr 9, 2021 • edited Loading

codecov bot commented Apr 9, 2021 • edited Loading

Codecov Report

dmigach Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmigach Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VojtaStavik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VojtaStavik commented Apr 13, 2021

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

VojtaStavik left a comment

Choose a reason for hiding this comment

VojtaStavik commented Apr 15, 2021

dmigach commented Apr 9, 2021 •

edited

Loading

codecov bot commented Apr 9, 2021 •

edited

Loading

dmigach Apr 9, 2021 •

edited

Loading

dmigach Apr 13, 2021 •

edited

Loading