Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Mark "attempting out" messages as "unsent" on app launch. #113

Merged
merged 1 commit into from Feb 13, 2017
Merged

Mark "attempting out" messages as "unsent" on app launch. #113

merged 1 commit into from Feb 13, 2017

Conversation

charlesmchen
Copy link
Contributor

See: signalapp/Signal-iOS#1035

I tried to use OWSDisappearingMessagesJob and OWSDisappearingMessagesFinder as a model.

  • I used an index & query rather than a view. Thoughts?
  • I significantly elaborated the logging around message sending, failures and retries.
  • Currently, if the device has no network access, message sending still waits through repeated retry attempts to timeout so messages don't fail for a couple of minutes. I wonder if we should abort retry in this case.

PTAL @michaelkirk

@charlesmchen charlesmchen mentioned this pull request Feb 9, 2017
Merged
@charlesmchen
Copy link
Contributor Author

See signalapp/Signal-iOS#1741

charlesmchen
charlesmchen commented Feb 9, 2017
View reviewed changes
src/Messages/OWSMessageSender.m
@@ -554,6 +560,9 @@ - (void)handleMessageSentLocally:(TSOutgoingMessage *)message
{
[self saveMessage:message withState:TSOutgoingMessageStateSent];
if (message.shouldSyncTranscript) {
// TODO: I suspect we shouldn't optimistically set hasSyncedTranscript.
// We could set this in a success handler for [sendSyncTranscriptForMessage:].
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelkirk thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's weird to optimistically set it like this. But make sure than any changes don't result in it being sent once per group recipient.

E.g. if I'm in a group with Alice, Bob, and Georgia, I should still send only one sync message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened a new issue where we can track/discuss this issue: signalapp/Signal-iOS#1761

@charlesmchen
Copy link
Contributor Author

PTAL @michaelkirk

michaelkirk
michaelkirk approved these changes Feb 13, 2017
View reviewed changes
Copy link
Contributor

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/Messages/OWSMessageSender.m
@@ -554,6 +560,9 @@ - (void)handleMessageSentLocally:(TSOutgoingMessage *)message
{
[self saveMessage:message withState:TSOutgoingMessageStateSent];
if (message.shouldSyncTranscript) {
// TODO: I suspect we shouldn't optimistically set hasSyncedTranscript.
// We could set this in a success handler for [sendSyncTranscriptForMessage:].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's weird to optimistically set it like this. But make sure than any changes don't result in it being sent once per group recipient.

E.g. if I'm in a group with Alice, Bob, and Georgia, I should still send only one sync message.

@charlesmchen charlesmchen mentioned this pull request Feb 13, 2017
Closed
@charlesmchen charlesmchen merged commit 821c96c into master Feb 13, 2017
@charlesmchen charlesmchen deleted the charlesmchen/markUnsentMessages branch February 13, 2017 22:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@michaelkirk michaelkirk michaelkirk approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@charlesmchen @michaelkirk