Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor: split TimelineFragment into MessageComposerFragment and VoiceRecorderFragment #7285

Merged
merged 7 commits into from
Oct 5, 2022
Merged

Refactor: split TimelineFragment into MessageComposerFragment and VoiceRecorderFragment #7285

merged 7 commits into from
Oct 5, 2022

Conversation

jmartinesp
Copy link
Member

Type of change

  • Feature
  • Bugfix
  • Technical: refactor.
  • Other :

Content

Split TimelineFragment into 3 separate entities:

  • TimelineFragment.
  • MessageComposerFragment.
  • VoiceRecorderFragment.

Motivation and context

TimelineFragment is a large class that has many responsibilities: rendering timeline events, handling composer input, handling voice recording, etc. This is an attempt to separate those into separate components that can be worked on independently.

Screenshots / GIFs

UI hasn't changed.

Tests

  • Go into any room.
  • Try sending a message, see that the UI behaves as expected.
  • Try also replies, quotes, attachments, editing events, etc.
  • Try autocompletion.
  • Try tapping on usernames in the timeline.
  • Try voice recordings.
  • In general, try anything that's related to either the message composer or voice recording.

Tested devices

  • Physical
  • Emulator
  • OS version(s): 11, 12.

Checklist

@jmartinesp jmartinesp requested a review from a team October 4, 2022 11:32
@jmartinesp jmartinesp self-assigned this Oct 4, 2022
@jmartinesp jmartinesp requested review from ganfra and removed request for a team October 4, 2022 11:32
@jmartinesp jmartinesp marked this pull request as ready for review October 4, 2022 11:33
@jmartinesp jmartinesp force-pushed the tech/split-timelinefragment branch 2 times, most recently from b6c58b8 to b1aa29b Compare October 4, 2022 14:45
@jmartinesp jmartinesp mentioned this pull request Oct 4, 2022
Merged
15 tasks
@jmartinesp jmartinesp force-pushed the tech/split-timelinefragment branch 2 times, most recently from 4f35ade to b571efb Compare October 5, 2022 05:46
@jmartinesp jmartinesp force-pushed the tech/split-timelinefragment branch from b571efb to e1cad01 Compare October 5, 2022 09:22
bmarty
bmarty reviewed Oct 5, 2022
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will do some tests. Please add a changelog.

...or/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerFragment.kt
super.onDestroyView()

autoCompleter.clear()
messageComposerViewModel.endAllVoiceActions()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move this line to VoiceRecorderFragment.onDestroyView() ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think that line is not needed. Leaving the screen by either going back, leaving the app or going to a new screen sends the MessageComposerAction.OnEntersBackground action, that always stops either the playback or the recording of a voice message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think I left if there because it would also stop any sent/received voice messages that were being played from the timeline items.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for checking.

...rc/main/java/im/vector/app/features/home/room/detail/composer/voice/VoiceRecorderFragment.kt Outdated
@Inject lateinit var audioMessagePlaybackTracker: AudioMessagePlaybackTracker
@Inject lateinit var clock: Clock

private val timelineViewModel: TimelineViewModel by existingViewModel()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about existingViewModel()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could change that to activityViewModel() if we want the child fragments to work even if there's not a TimelineFragment.

ElementBot
ElementBot reviewed Oct 5, 2022
View reviewed changes
vector/src/main/res/layout/fragment_composer.xml
android:id="@+id/composerLayout"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:background="?android:colorBackground"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • ⚠️ Possible overdraw: Root element paints background ?android:colorBackground with a theme that also paints a background (inferred theme is @android:style/Theme.Holo)
  • ⚠️ Possible overdraw: Root element paints background ?android:colorBackground with a theme that also paints a background (inferred theme is @android:style/Theme.Holo)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 5, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@bmarty
Copy link
Member

bmarty commented Oct 5, 2022

I have done a quick smoke test and it seems to work fine. I will merge the PR after the release though. Thanks!

@jmartinesp
Copy link
Member Author

I have done a quick smoke test and it seems to work fine. I will merge the PR after the release though. Thanks!

Thanks! I'll rebase #7288 then since it's based on this PR, someone will probably take it over while I'm away.

@bmarty bmarty merged commit 9335242 into develop Oct 5, 2022
@bmarty bmarty deleted the tech/split-timelinefragment branch October 5, 2022 15:31
@bmarty bmarty mentioned this pull request Oct 7, 2022
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElementBot ElementBot ElementBot left review comments

@bmarty bmarty bmarty approved these changes

@ganfra ganfra Awaiting requested review from ganfra ganfra was automatically assigned from element-hq/element-android-reviewers

Assignees

@jmartinesp jmartinesp

Labels
A-Composer A-Timeline A-Voice-Messages Z-Merge After Release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmartinesp @bmarty @ElementBot