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

Unfork/Update JSQMessagesViewController #1255

Merged
merged 9 commits into from
Jul 15, 2016
Merged

Unfork/Update JSQMessagesViewController #1255

merged 9 commits into from
Jul 15, 2016

Conversation

michaelkirk
Copy link
Contributor

@michaelkirk michaelkirk commented Jul 13, 2016

Description

Getting back on upstream JSQMessagesViewController fixes a couple bugs (see ##Bugfixes), and makes future upgrades easier.

The unforking process was basically this:

  • Starting from our fork: https://github.com/WhisperSystems/JSQMessagesViewController
  • move custom message types (Calls and DisplayedMessages) classes from our
    custom JSQMVC fork into Signal-iOS.
  • Move any method customization into our subclass. Including
    CollectionView protocol stuff, styling, bubble sizing, and gesture behavior

BugFixes

FIXES: #1111 - incorrectly sized bubbles for incoming attachments:
FIXES: Bubble-size re-flow when first loading thread.
FIXES: #1062?

Maybe Fixes: #838 - Rendering Glitches
Maybe Fixes: #1181 - JumbledBubbles

Also, by getting back on upstream we pave the way for #792 and generally make keeping up-to-date easier.

This required a lot of changes. Code review appreciated. ❤️

@2-4601
Copy link
Contributor

2-4601 commented Jul 14, 2016

Probably also fixes #1062?

Geting back on upstream fixes a couple bugs (see ##Bugfixes), and also
will make future updates easier.

The unforking process was basically this:

* move custom message types (Calls and DisplayedMessages) classes from our
  custom JSQMVC fork into Signal-iOS.
* Move any method customization into our subclass. Including
  ColletionView stuff, bubble sizing, and gesture behavior

Bug Fixes
---------
* Fix mis-sized incoming media bubbles.

Bubble size was being cached by interaction id. Which broke when
receiving an attachment. The problem is that incoming media messages
were initially the height of a "Downloading Attachment" info message.
Instead we use the mediaHash for media messages to expire the bubble
size when the media changes.

* fix missized bubble when MVC did appear

The MessagesViewController isn't sized correctly until ViewWillAppear.
This caused the first round of bubbles to be rendered incorrectly (they
assumed a larger container than they had).  I think is reflected in the
current version of the app by a reflow occurring shortly after the view
appears.

Chores
------
* bump travis to build with xcode8
* specify RQV development team for device build. required by xcode 8 beta

Cleanup
------
* Refactor messageing XIB so that elements are hangning outside of
  the views frame
* Fix compiler warning with explicit cast
* delete deprecated lineBreakmode, it's the default value anyway.

// FREEBIE
This is part of unforking JSQMVC, but I'm only reformatting files
otherwise changed rather than reformatting the entire project for git
sanity.

This is intentionally a second commit so we can separate formatting
changes from code changes

* brace should drop after method definition (this is common across
  almost all objc projects, and allows you to quickly differentiate
  multi-line method signatures from their implementation.
* aligning consecutive assignments ongoingly muddies git history

// FREEBIE
This was done as part of unforking JSQMessagesViewController, but is
intentionally a separate commit so we can separate formatting changes
from code changes in git history.

* Import frameworks like:
    #import <FrameworkName/HeaderName.h>
* instead of:
    #import "HeaderName.h"

// FREEBIE
* Use NS_DESIGNATE_INTIALIZER to ensure we're setting all the variables we need to be in OWSCall
* no need to nil properties in dealloc on ARC
* use declarative dictionaries for legability

// FREEBIE

use declarative dictionaries for clarity
* formatting
* referencing TODOS
* remove unnecessary logging

// FREEBIE
Using upstream button and change notification allows us to delete some
code.

// FREEBIE
The history here is that the radius I'm removing was added in #884 after
having straight edges added in our custom fork for a while.

So, rather than override our override, I'm just reverting to the
upstream value, which is close in proximity, and closer to iMessages
radius.

// FREEBIE
@michaelkirk michaelkirk modified the milestones: 2.3.6, On Roadmap, 2.4 Jul 19, 2016
@Aureleme
Copy link

Hi,
Will you implement the JSQMessagesViewController's next update (8.0) when it will be release?

@michaelkirk
Copy link
Contributor Author

@Aureleme - That's the plan at this point, but when it happens depends on how many breaking changes are incorporated in the upstream release.

It looks like some good (and ambitious) work is happening up there.

❤️ /cc @jessesquires

@jessesquires
Copy link
Contributor

See: jessesquires/JSQMessagesViewController#1216 for more details.

Also, I would be more than happy to help Signal-iOS with the migration to v8.0. 😊

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