Ignore mouse click if compose box is open.

[kaustubh-nair]

Clicking on messages/user names when the compose box is open causes the
current message to be lost. This commits ignores the mouse click to
prevent the typed data to be lost, though better handling can be done
when the drafts feature is implemented.

Also, stored controller as an attribute for UsersView. Tests amended for
this.

Test added for mouse click event.

Fixes #652.

[neiljp]

@kaustubh-nair This avoids the crash from compose->compose, at least from manual testing 👍

As you noted in the chat however, similar issues apply for being in a (any?) search box and entering compose (ie. search -> compose), which it would be good to address at the same time, if possible?

I understand the thought of being general in the code, but for the purposes of the original solution, wrapping the functionality slightly differently in the controller may simplify it for the calling code, as per the inline comment. For the more general case I could understand the approach you took initially, but can we achieve the same result with only is_in_editor_mode()?

@preetmishra mentioned similar issues between search boxes too, at least with regard to focus, though not crashing, which we might resolve in a similar way?

Some of these are added extras for consistency and avoiding strange behavior, but focusing on needing GO_BACK to leave search text entry or message composition/editing feels like a good baseline? It might be worth analyzing use of that key and clarifying the token name and description, based on the usage, rather than having simply 'go back'. We could work on having compose open while navigating, but that feels like a separate feature at this stage?

[neiljp]

This is common to both cases, which suggests this could be in a common controller.is_composing_message_now() or similar?

[kaustubh-nair]

@neiljp making a method in the controller sounds like a good idea.

I needed is_in_editor_mode_with() because we might require different behavior depending on which editor is active ( for example search boxes ) but if simply requiring a GO_BACK is enough for now then we think about this in follow-ups.

[kaustubh-nair]

@neiljp Thanks for the review, PR updated 👍

[neiljp]

@kaustubh-nair This looks good 👍 I'm happy to merge this with a few minor changes.

Did this previously give data-loss with no crash, but gives a crash after my refactoring to add the assert? The commit message suggests the former, whereas the current code reflects the latter? In both cases we lose data, but 'causes message to be lost' doesn't really reflect the code at this point, so let's have the commit text reflect the current behavior - though we can of course mention how the bug changed with the refactoring if you like.

Other minor points:

• You could indicate that this is a bugfix: area, which we have used in the past as an additional area in the commit titles. We could update the developer docs to reflect the commit text style further, since we don't mention that
• In future we could maybe improve the nearby tests, given the left/right click notes, but that may be covered in an existing older PR by @preetmishra in any case, or at least is very similar?

While not overly significant since it's not a major issue and involves mouse usage, the focus still changes even with clicks - that's noticeable given the cursor stops blinking in the box we're typing in. That applies to searching too, so could be another spinoff issue/PR, a little like #724?

[neiljp]

minor: this would be more transparent as in_editor_mode or similar?

Consider using ids since eg. True isn't that obvious in the test summary. Maybe a local fixture given it's used twice?

[kaustubh-nair]

@neiljp, thanks for the review, updated PR!
Re. Opened an issue for dealing with the cursor. #728

[neiljp]

@kaustubh-nair Thanks for the fix! Just manually merged 🎉 (you had a different file in your commit title than in the commit)