Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add updating logic to text message #178

Merged
merged 5 commits into from Jul 31, 2016

Conversation

maxkonovalov
Copy link
Contributor

Added support for willBeShown() and wasHidden() functions in TextMessageViewModel. The main purpose of this is to give chance to the client to load the avatar images when needed. Most of the code was copied from the corresponding functions in PhotoMessagePresenter.
Both Text and Photo message models now look pretty similar, so maybe you guys consider moving some parts to the base classes?

Also added the missing observe calls for the avatar image in view models.

And the essential part of this commit is the call to the main queue in updateClosure:

dispatch_async(dispatch_get_main_queue(), {
    self?.updateCurrentCell()
})

It looks strange, but if you try to set the observable value in willBeShown(), although the model itself has been updated, in BaseMessageCollectionViewCell.updateViews() it still gets the old value, so the update has no visible effect. Deferring the update with an async call to the main queue seems to fix the issue here.

@codecov-io
Copy link

codecov-io commented Jul 9, 2016

Current coverage is 63.55% (diff: 42.85%)

Merging #178 into dev will decrease coverage by 0.32%

@@                dev       #178   diff @@
==========================================
  Files            62         62          
  Lines          3430       3452    +22   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2191       2194     +3   
- Misses         1239       1258    +19   
  Partials          0          0          

Powered by Codecov. Last update 7541a5a...a0c1ec9

@diegosanchezr
Copy link
Contributor

Hi @maxkonovalov,

Yes, I think we should move the avatar thing to the BaseMessagePresenter and clean the text and photo ones a little bit. We would appreciate if you do this here (not required).

I was tracking down your need for the dispatch_async. Check the root cause here: https://lists.swift.org/pipermail/swift-users/Week-of-Mon-20160711/002580.html. If you don't mind, could you make Observable a class and remove the dispatch_async?

Thank you!

@maxkonovalov
Copy link
Contributor Author

Hey @diegosanchezr, sorry for the delay, sure, I'll try add these changes as soon as possible.

@diegosanchezr
Copy link
Contributor

Hi @maxkonovalov, are you still able to work on this? We'd definitely like to merge this. Thank you!

@maxkonovalov
Copy link
Contributor Author

Hey @diegosanchezr, sorry again - very busy with my other projects, but will get back to this issue in a few days!

@diegosanchezr
Copy link
Contributor

Cool, thank you!

@maxkonovalov
Copy link
Contributor Author

Hey @diegosanchezr! As requested, I updated the Observable - it now seems to work without issues.

For refactoring, I only moved the declarations of the methods willBeShown() and wasHidden() to their common protocol. I think it may still be worth combining TextMessageViewModel and PhotoMessageViewModel to some common superclass instead of inheriting both from a protocol.

Also changed the avatar show/hide logic, because the previous implementation triggered Observable updates resulting in infinite loop.

@diegosanchezr diegosanchezr merged commit 1ef46a0 into badoo:dev Jul 31, 2016
@diegosanchezr
Copy link
Contributor

@maxkonovalov Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants