Fix force unwrap in BaseCollectionViewController

[alaija]

Force unwrap leads to crash in cases when ViewController addresses to collectionview before it was loaded. e.g. You can try to rotate device when chat is initialized inside unopened tab inside UITabBarController.

[codecov-io]

Codecov Report

Merging #538 into master will increase coverage by 0.01%.
The diff coverage is 73.21%.

@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
+ Coverage   62.94%   62.95%   +0.01%     
==========================================
  Files          77       77              
  Lines        4037     4063      +26     
==========================================
+ Hits         2541     2558      +17     
- Misses       1496     1505       +9

Impacted Files	Coverage Δ
...Controller/BaseChatViewController+Presenters.swift	46.47% <25%> (-2.06%)	⬇️
...hatController/BaseChatViewController+Changes.swift	83.93% <56.52%> (-2.32%)	⬇️
...Source/ChatController/BaseChatViewController.swift	76.17% <78.43%> (-0.25%)	⬇️
...tController/BaseChatViewController+Scrolling.swift	84.5% <82.35%> (+1.96%)	⬆️
...Chat Items/TextMessages/Views/TextBubbleView.swift	75.97% <0%> (+1.67%)	⬆️

---

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 baf2a76...40f4ead. Read the comment docs.

[AntonPalich]

@alaija Thank you for your contribution!

It makes sense to use optional values for views in view controllers. View hierarchy may not be initialized when some view controller functions are called.

However, a view controller contains many functions that are using views and expecting a view hierarchy to be initialized when they are called, for example, viewDidLayoutSubviews, viewWillAppear, etc.

So, a question should be asked: "Does it make sense to call a function that is using views when view hierarchy is not initialized?" If the answer is "No", then isViewLoaded check can be performed before the function is called.

Currently, all views in BaseChatViewController are defined using forced unwrapped optional type. All functions that are using those views expect a view hierarchy to be initialized when they are called. Potentially, every access to those values may lead to a crash at runtime.

So, let's take a look at some examples: isCloseToTop, isCloseToBottom, performBatchUpdates, etc. All of them are accessing forced unwrapped optionals. If you call those functions without initialized view hierarchy you will get a crash. But does it make sense to call them without initialized view hierarchy at all? I would say no.

So, my proposal is to add isViewLoaded guard statement to entry points that call functions that access a view hierarchy. To fix your crash it would be enough to add this statement to viewWillTransition(to:with:) function because it doesn't make sense to do anything inside if a view hierarchy is not initialized. But this won't protect us from a similar crash in the future.

Using optional values for views is a safer way, but it should be applied to all views at the same time for consistency. This would be a huge refactoring. Also, it will make access to a view hierarchy less convenient. I would say, that every function that is using a view defined as an optional type will need to add a guard statement to unwrap it. There should be a balance between convenience and safety. I didn't find it yet :)

What do you think?

[alaija]

Actually, we made this(isViewLoaded) workaround in our child viewcontroller, but it looks like workaround and not a solution, cause we speak about “Base” class and someone who will implement his child viewcontroller can be faced to the problem again.

Anyway layouting is just one case, there are too many of them, and I really surprised that no one met any before me, so in my opinion optional views is the only solution.

[AntonPalich]

Everything looks good. I left few comments. Can you take a look?

[alaija]

Please check this method carefully, cause it too complicated to get what blocks should I call on exit.

P.S. It is a good candidate for refactoring

[AntonPalich]

There are many places that requires refactoring :) Someday we will take care about them