model: Inform user if message was not sent successfully.

[amanagr]

[sumanthvrao]

This looks like a nice addition! Just tried this and the 3 seconds seems fine for now, though we can probably reduce it a bit (based on more feedback of-course)

[amanagr]

Actually this was wip, I forgot to mention, Yeah the first commit is from another PR. Thanks for the review. Nice points, I will try and fix them.

[neiljp]

I won't review until you're ready, but whether in this PR or a follow-up, we could also have something similar which managed other actions too; this was just made clear when czo was updated this afternoon :)

[amanagr]

@neiljp I feel this is mostly ready. For the short network outrage issue, do we really need to deal with it since the message is being successfully sent in the end?

[neiljp]

@neiljp I feel this is mostly ready. For the short network outrage issue, do we really need to deal with it since the message is being successfully sent in the end?

To clarify, the issue here is that the message was sent, but the notice given to the user in the footer is that it wasn't.

[amanagr]

@neiljp thanks for the find. I was able to reproduce. Seems like a tricky case. We send the request to client and client returns a response, that should end the thread right since we are not longpolling here or not doing any error_retries?

[amanagr]

re: last commit contains two changes? -> Do you want to split the send_lock part? re: I'm a little wary of having the model access the view via the controller, which wasn't the case when this occurred in the keypress method? -> We can add a method for updating footer to the controller so that it's easy for various functions in model (even in future) to update the footer. re:The lock could resolve the timing issue -> Yeah it's a tricky case would love to have additional ideas.

[neiljp]

• Re 2 changes, it felt like you had restructured and added the lock in the same commit? If the code stays in that form, it felt like the restructure should be in a separate commit, or just be part of the first one
• I understand that you want to place the lock in the model; it feels like it could also work with the lock in a controller method, and then that method could update the UI?

[amanagr]

I removed the lock and asynch, it works fine without them both.

[neiljp]

@amanagr I'm observing the same behavior here as before; trying to send with no network:

• doesn't update the UI with 'Sending...' at all
• pauses the UI for the non-longpolling 15s
• shows the the failure message
• if reconnected "quick enough", does actually send the message?

I expect we want the 15s timeout for actually sending, which suggests we do want something with asynch?

With network, it's fine, in that it updates the bar with a temporary success message, but that's not the complex part.

[sumanthvrao]

@amanagr A small extension here would be to have the footer display when a message is sent out the current narrow (somewhat similar to the web app message we get).

[neiljp]

Another extension could include to indicate that the message is below the current view, though that might be difficult to determine, I'm not sure?

[amanagr]

Thanks for the suggestions!

[amanagr]

@neiljp I am unable to figure out why the message is being successfully sent after we get an error response from the server in this case. Do you have any ideas as to why this might be happening given your history with the python-zulip-api?

[zulipbot]

Heads up @amanagr, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.