-
-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored Message input view into it's own Fragment #3792
Conversation
This comment was marked as spam.
This comment was marked as spam.
6cd4ed8
to
32a78cf
Compare
Hello there, 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! |
8b4b749
to
e9bbbd8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
c6c4e7b
to
dc6b0a3
Compare
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. |
bee5d19
to
90a68e2
Compare
Hello there, 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! |
As said, typing indicator is broken for now because of nextcloud/spreed#12252 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
b4c10f5
to
8d9ecef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see overall comments
bc8be97
to
ab34132
Compare
@rapterjet2004 #3792 (comment) 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 I forgot that when exiting -> entering an activity, the state will still be 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()
} |
But now the situation that GetCapabilitiesUpdateState remains is still the case right? I think
should be deleted from
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 |
- Added io folder for Abstracting away background work - AudioFocusRequestManager - MediaPlayerManager - MediaRecorderManager - AudioRecorderManager Included new View Models + Fragments to separate concerns - MessageInputFragment - MessageInputVoiceRecordingFragment Signed-off-by: rapterjet2004 <juliuslinus1@gmail.com>
1542a08
to
6a01ebf
Compare
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/3792-talk.apk |
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, soChatActivity
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
// TODO voice message input
// TODO clean up
🏁 Checklist
/backport to stable-xx.x