nextcloud
Refactored Message input view into it's own Fragment
- fixes #3784
Introduction
Shouldn't be merged until after the stable release.
MessageInput is very complex, spanning 1300+ lines of code. It deals with sending, recording, emojis, files, editing, mentioning, etc. It should be placed into it's own fragment, so ChatActivity becomes easier to maintain in the long run. Bit of an investment cost though and not necessarily a priority in case other features require attention. But it's something that should be taken care of before it gets out of control. Chatting is the core functionality of talk-android, and needs to be structured well.
This also gives me an opportunity to clean up the architecture of the view. Separate UI from business logic, and deal with asynchronous work in a less "Spaghetti" manner. Less threads, more coroutines. Less logic in the UI, more logic in the model layer, etc
The plan is the separate the voice recording from the typical messaging. To have two different fragments to handle the behavior. A significant amount of code (admittedly my own 😅 ) is involved in the voice recording logic, which can be pretty complicated. Fragments transaction events will be observed by the ChatViewModel and handled.
As a rule of thumb, I'm going for 25/15/60 (view, view model, model) when implementing logic in MVVM. Not everything is implemented in MVVM, just the functionality that relate to message input (with the exception of typing status).
PR Diagram
Arrows and colors represent the flow and scope of information traveling down from user input.
// TODO normal message input
- [x] sending messages
- [x] sending attachments
- [x] emoji button
- [x] replying
- [x] editing
- [x] mentioning
- [x] typing status?
- [x] recording
// TODO voice message input
- [x] Abstracting over MediaRecorder (Note to self: Make mediarecorder state livedata and immutable, good design)
- [x] Abstract over MediaPlayer
- [x] Abstract over AudioRecord
- [x] MicInputCloud
- [x] previewing // note pause mediaplayer before seeking and resume after
- [x] Preview SeekBar
- [x] pause/Play
- [x] sending preview
- [x] deleting preview
- [x] Implement Audio Focus Request (perhaps make this a manager too?)
- [x] Media Player ( in ChatActivity )
- [x] Voice Message preview player
- [x] Media Recorder
- [x] MicInput
// TODO clean up
- [x] Delete all the old code
- [x] Bugs
- [x] save messageInputView text and cursor state
- [x] save position and voicemessage on state change in saved ( this is an issue with the lifecycle being recreated )
- [x] checkShowMessageInputView
- [x] checkLobbyState
- [x] onCreateOptionsMenu (used for context???)
- [x] handleOnBackPressed
- [x] Make PreviewSeekbar seekable
- [x] Fix Voice record bugs
- [x] use upload file from chatView
- [x] Micinput restart ui
- [x] If the preview playback is interrupted, the seekbar will not seek on following recordings, might be related to AudioFocusRequest
- [x] AudioFocus crash, need to handle receiver illegal arg exception
🏁 Checklist
- [ ] ⛑️ Tests (unit and/or integration) are included or not needed
- [ ] 🔖 Capability is checked or not needed
- [ ] 🔙 Backport requests are created or not needed:
/backport to stable-xx.x - [ ] 📅 Milestone is set
- [x] 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)
Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.
We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.
Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6
Thank you for contributing to Nextcloud and we hope to hear from you soon!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.
We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.
Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6
Thank you for contributing to Nextcloud and we hope to hear from you soon!
Edit message is broken:
- [x] Original message is not shown
- [x] send and save button shown at the same time
- [x] It crashed when opening voice recording:
2024-05-06 15:30:29.820 22667-22667 MessageInputFragment com.nextcloud.talk2 D ACTION_UP
2024-05-06 15:30:29.828 22667-22667 ChatViewModel com.nextcloud.talk2 D Recording stopped
2024-05-06 15:30:29.828 22667-22667 InputEventReceiver com.nextcloud.talk2 E Exception dispatching input event.
2024-05-06 15:30:29.829 22667-22667 MessageQueue-JNI com.nextcloud.talk2 E Exception in MessageQueue callback: handleReceiveCallback
2024-05-06 15:30:29.838 22667-22667 MessageQueue-JNI com.nextcloud.talk2 E java.lang.IllegalArgumentException: Receiver not registered: com.nextcloud.talk.chat.data.io.AudioFocusRequestManager$noisyAudioStreamReceiver$1@da4e131
at android.app.LoadedApk.forgetReceiverDispatcher(LoadedApk.java:1770)
at android.app.ContextImpl.unregisterReceiver(ContextImpl.java:1899)
at android.content.ContextWrapper.unregisterReceiver(ContextWrapper.java:829)
at com.nextcloud.talk.chat.data.io.AudioFocusRequestManager.handleBecomingNoisyBroadcast(AudioFocusRequestManager.kt:103)
at com.nextcloud.talk.chat.data.io.AudioFocusRequestManager.audioFocusRequest(AudioFocusRequestManager.kt:94)
at com.nextcloud.talk.chat.viewmodels.ChatViewModel.stopAudioRecording(ChatViewModel.kt:586)
at com.nextcloud.talk.chat.viewmodels.ChatViewModel.stopAndSendAudioRecording(ChatViewModel.kt:594)
at com.nextcloud.talk.chat.MessageInputFragment.initVoiceRecordButton$lambda$9(MessageInputFragment.kt:359)
- Edit a message and save it
- Enter permanent voice recording mode
- Cancel it Bug: Editing mode is active
Same for "reply" mode like for message editing
- Edit a message with some extra spaces at the end or don't modify the original message.
- Send the message.
- Edit is performed on the message - bug
- Type a message in the input message field and rotate the screen.
- App crashes.
2024-05-08 12:06:50.346 14641-14641 AndroidRuntime com.nextcloud.talk2 E FATAL EXCEPTION: main Process: com.nextcloud.talk2, PID: 14641 java.lang.NullPointerException at com.nextcloud.talk.chat.MessageInputFragment.updateOwnTypingStatus$sendStartTypingSignalingMessage(MessageInputFragment.kt:554) at com.nextcloud.talk.chat.MessageInputFragment.updateOwnTypingStatus(MessageInputFragment.kt:566) at com.nextcloud.talk.chat.MessageInputFragment$initMessageInputView$1.onTextChanged(MessageInputFragment.kt:203) at android.widget.TextView.sendOnTextChanged(TextView.java:11785) at android.widget.TextView.setText(TextView.java:6965) at android.widget.TextView.setText(TextView.java:6761) at android.widget.EditText.setText(EditText.java:145) at android.widget.TextView.setText(TextView.java:6713) at com.nextcloud.talk.chat.MessageInputFragment.restoreState(MessageInputFragment.kt:157) at com.nextcloud.talk.chat.MessageInputFragment.onCreateView(MessageInputFragment.kt:125) at androidx.fragment.app.Fragment.performCreateView(Fragment.java:2963) at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:518) at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:282) at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:2189) at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:2106) at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:2002) at androidx.fragment.app.FragmentManager$5.run(FragmentManager.java:524) at android.os.Handler.handleCallback(Handler.java:938) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loop(Looper.java:246) at android.app.ActivityThread.main(ActivityThread.java:8595) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:602) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1130)
It seems the typing indicator is broken in general, unrelated to this PR, but I'm receiving the same error messages that I encountered on Master, so I'm assuming it should be sending the messages at least.
Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.
We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.
Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6
Thank you for contributing to Nextcloud and we hope to hear from you soon!
It seems the typing indicator is broken in general, unrelated to this PR, but I'm receiving the same error messages that I encountered on Master, so I'm assuming it should be sending the messages at least.
As said, typing indicator is broken for now because of https://github.com/nextcloud/spreed/issues/12252 Nothing to fix in your PR...
Looks good to me. One minor modification - let's put the cursor to the end of the message when someone edits a message.
- type some text
- wait 30 sec
- bug: text disappears
or
- have some text as draft
- change text
- wait 30 sec
- bug: text is overwritten by old draft
It's kind of expected as https://github.com/nextcloud/talk-android/pull/3918 is not applied because of this PR's changes. So as mentioned in https://github.com/nextcloud/talk-android/pull/3918#pullrequestreview-2063613411 it would make sense to fix it in better ways..
one minor note: "Slide to cancel" for voice messages is sliding away much faster now
The chat is not updated anymore (after ~1min ?)
🤦🏾 I forgot to refresh the chat
is ChatViewModel.GetCapabilitiesUpdateState -> {
spreedCapabilities = state.spreedCapabilities
chatApiVersion = ApiUtils.getChatApiVersion(spreedCapabilities, intArrayOf(1))
participantPermissions = ParticipantPermissions(spreedCapabilities, currentConversation!!)
invalidateOptionsMenu()
checkShowCallButtons()
checkShowMessageInputView()
checkLobbyState()
updateRoomTimerHandler()
-
+ chatViewModel.refreshChatParams(
+ setupFieldsForPullChatMessages(
+ false,
+ 0,
+ false
+ )
+ )
}
The chat is not updated anymore (after ~1min ?)
🤦🏾 I forgot to refresh the chat
is ChatViewModel.GetCapabilitiesUpdateState -> { spreedCapabilities = state.spreedCapabilities chatApiVersion = ApiUtils.getChatApiVersion(spreedCapabilities, intArrayOf(1)) participantPermissions = ParticipantPermissions(spreedCapabilities, currentConversation!!) invalidateOptionsMenu() checkShowCallButtons() checkShowMessageInputView() checkLobbyState() updateRoomTimerHandler() - + chatViewModel.refreshChatParams( + setupFieldsForPullChatMessages( + false, + 0, + false + ) + ) }
This works of course (but might introduce bugs!), but it feels like this unveils something is wrong:
Pulling chat messages should be triggered from inside pulling chat messages itself as that is the idea of long polling. As far as i see all "recursive calls" to call pullChatMessages via chatViewModel.refreshChatParams(setupFieldsForPullChatMessages( are in place, but maybe there is something wrong?
As a side note: Signaling messages should be used in the future to update the chat and Long polling should only be a fallback when external signaling server is not available. But this is nothing for this PR.
@rapterjet2004
- open the chat on phone
- send a message from web to phone. message is shown correctly
- while having chat opened, bring the app the background
- open the app again
- send another message from web to phone. message is shown as push notification instead to update the chat
When leaveRoom#onNext is executed in ChatViewModel, the activity is already gone, so ChatViewModel.LeaveRoomSuccessState is not triggered anymore.
However when the app comes to foreground again, ChatViewModel.LeaveRoomSuccessState is executed and executes all the code to leave a room (the websocket session is reset...). So the room is not joined at all and that's why the chat not updated but a push notification is received.
One main problem here is that the session handling is still done in the activity! In this branch, i would say we should just find a way to fix the observer behaviour. However we have to move the session handling to viewmodel or other places, but definitely not in the activity..
@rapterjet2004 https://github.com/nextcloud/talk-android/pull/3792#issuecomment-2141996064 is solved right? As the commits are already squashed: can you give a short summary what you changed to solve it?
At first I hardcoded the leave room logic, but the error persisted. After reverting the git, I think the reason for this, is because the bug was caused by my earlier changes to the PR, where I added an additional GetCapabilitiesUpdateState so that the message state wouldn't get recreated every time capabilities was called.
I forgot that when exiting -> entering an activity, the state will still be GetCapabilitiesUpdateState instead GetCapabilitiesInitialLoadState because it's held in the ChatViewModel. Therefore the room would not be joined, and refreshChatParams wouldn't be called to the start the recursive chain.
is ChatViewModel.GetCapabilitiesUpdateState -> {
spreedCapabilities = state.spreedCapabilities
chatApiVersion = ApiUtils.getChatApiVersion(spreedCapabilities, intArrayOf(1))
participantPermissions = ParticipantPermissions(spreedCapabilities, currentConversation!!)
invalidateOptionsMenu()
checkShowCallButtons()
checkShowMessageInputView()
checkLobbyState()
+ if (!validSessionId()) {
+ joinRoomWithPassword()
+ chatViewModel.refreshChatParams(
+ setupFieldsForPullChatMessages(
+ false,
+ 0,
+ false
+ )
+ )
+ } else {
+ Log.d(TAG, "already inConversation. joinRoomWithPassword is skipped")
+ }
-
updateRoomTimerHandler()
}
https://github.com/nextcloud/talk-android/pull/3792#issuecomment-2147501832
But now the situation that GetCapabilitiesUpdateState remains is still the case right? Which is confusing because then GetCapabilitiesUpdateState also contains code for initialization (additionally to the code in GetCapabilitiesInitialLoadState)
I think
+ if (!validSessionId()) {
+ joinRoomWithPassword()
+ chatViewModel.refreshChatParams(
+ setupFieldsForPullChatMessages(
+ false,
+ 0,
+ false
+ )
+ )
+ } else {
+ Log.d(TAG, "already inConversation. joinRoomWithPassword is skipped")
+ }
should be deleted from
is ChatViewModel.GetCapabilitiesUpdateState
and instead we reset some states in ChatViewModel#leaveRoom? E.g.
_getCapabilitiesViewState.value = GetCapabilitiesStartState
_getRoomViewState.value = GetRoomStartState
so the next time the activity is opened the states for initialization are triggered..
PS: it think both solutions should just be a workaround until session is managed in viewModel or wherever. Then a check like
if (!validSessionId()) {
should be done to decide which actions need to be triggered.
Codacy
Lint
| Type | master | PR |
| Warnings | 79 | 79 |
| Errors | 10 | 124 |
SpotBugs
| Category | Base | New |
|---|---|---|
| Bad practice | 6 | 6 |
| Correctness | 11 | 11 |
| Dodgy code | 84 | 84 |
| Internationalization | 3 | 3 |
| Malicious code vulnerability | 3 | 3 |
| Performance | 6 | 6 |
| Security | 1 | 1 |
| Total | 114 | 114 |
Lint increased!
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/3792-talk.apk
To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud Talk app.