edit message : allow cancellation of edit

[agrawal-d]

Fixes #2757

Before:

Now:

[agrawal-d]

@gnprice @ray-kraesig please review.

[rk-for-zulip]

(The commit summary and message also need to be updated to refer to #2757. It does fix that bug, as the videos show.)

[rk-for-zulip]

This will call cancelEditMessage on every attempted back navigation, not just the ones where it's relevant. That's a huge hack. Calling this function should be limited to when the user is actually in the edit-message state.

Also, preferably, hitting back in that context should exit the "edit message" state, but not the current narrow.

(#2757 is pretty bad misbehavior, so I wouldn't be completely unwilling to accept such a hack, but it would need to be very well-documented – at the very least, a) here, b) at cancelEditMessage itself, and c) at any existing reducers for this dispatch. However, it's easy to check the relevant condition even from here, so that's a moot point.

Checking it here is still a hack that needs documenting, and one I expect we'll have issues with when we upgrade react-navigation; but that's going to require rethinking a number of things anyway.)

[agrawal-d]

Fixed!

[agrawal-d]

Pushed some changes. @ray-kraesig please review.

[rk-for-zulip]

As noted previously, both the code and the commit need documentation: the former in comments, the latter in the commit summary.

[agrawal-d]

@ray-kraesig have made the requested changes. Please review, thanks.

[rk-for-zulip]

The code change below is very much a hack.

Its associated comment should thus start with HACK: , or some similar marker of grave concern, and then continue with what the right thing to do is (or is believed to be) and why this has been done here instead of that there. Similarly, some documentation of this hack's existence should be present in ComposeBox, ActionSheet, and/or some other component responsible for the edit message state.

(Also, this is function-documentation-style comment formatting [0] [1] [2], rather than ordinary comment formatting.)

[agrawal-d]

@ray-kraesig, When you mentioned

what the right thing to do is (or is believed to be) and why this has been done here instead of that there.

I looked at other locations where we could dispatch cancelEditMessage().
If my suggested change is a hack, then why not make the proper change instead.

I think that the change can be instead made in the following places, to check if an edit is in progress, and cancel it, instead of navigating back:

• In chatNavBar's onPress handler.
• In backNavigationHandler's handleBackButtonPress().

So, should I revert changes in navActions.js and make changes in the above-mentioned files instead?
If yes, would comments explaining the code be required there too?

If not, I can not think of an answer to why this has been done here instead of that there and will heed your help in answering that. Thanks!

[rk-for-zulip]

I think that the change can be instead made in the following places, to check if an edit is in progress, and cancel it, instead of navigating back:

• In chatNavBar's onPress handler.
• In backNavigationHandler's handleBackButtonPress().

This would be strictly worse. Instead of one component that shouldn't know anything about editing checking our editing state, we'd have two disconnected components that shouldn't know anything about editing checking our editing state.

The right thing to do will be some form of making editing affect our navigation state (which is designed to allow other components to affect it), rather than making navigation affect our editing state (which should be entirely local to the editing components).

However...

If not, I can not think of an answer to why this has been done here instead of that there and will heed your help in answering that. Thanks!

... in retrospect, this is not something that should be asked of a new contributor. Unfortunately, it's the most important part of a patch like this.

Closing. (I'll try to ensure this PR is referenced if we end up with a temporary fix similar to this.)