Skip to content

Fixes #14018 - Improve InputPanel Views State management #14023

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Sagar0-0
Copy link
Contributor

First time contributor checklist

Contributor checklist

  • Device A, Android X.Y.Z
  • Device B, Android Z.Y
  • Virtual device W, Android Y.Y.Z
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

@@ -569,17 +567,6 @@ public void onRecordPermissionRequired() {
@Override
public void onRecordPressed() {
if (listener != null) listener.onRecorderStarted();
recordTime.display();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to updateComposeViews function, which is called from ConversationFragment.

@@ -613,15 +600,20 @@ public void onRecordMoved(float offsetX, float absoluteX) {
@Override
public void onRecordCanceled(boolean byUser) {
Log.d(TAG, "Recording canceled byUser=" + byUser);
onRecordHideEvent();
if (listener != null) listener.onRecorderCanceled(byUser);
long elapsedTime = onRecordHideEvent();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do not save draft of less than one second

});

return elapsedTime;
return recordTime.hide();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All recording-related views will be updated from updateComposeViews

public @Nullable DraftTable.Draft getVoiceNoteDraft() {
return voiceNoteDraftView.getDraft();
}
public void updateComposeViews() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this a single source of truth for updating the views prevents any overlapping of different states.

@@ -873,7 +911,7 @@ private RecordTime(@NonNull TextView recordTimeView, @NonNull View microphone, l
public void display() {
this.startTime = System.currentTimeMillis();
this.recordTimeView.setText(DateUtils.formatElapsedTime(0));
ViewUtil.fadeIn(this.recordTimeView, FADE_TIME);
recordTimeView.setVisibility(View.VISIBLE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason: A consecutive fadeOut and fadeIn call to this makes the timeView invisible. Directly setting the visibility fixed the bug with the minimum amount of work.

@@ -1663,6 +1645,22 @@ class ConversationFragment :

conversationItemDecorations = ConversationItemDecorations(hasWallpaper = args.wallpaper != null)
binding.conversationItemRecycler.addItemDecoration(conversationItemDecorations, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The listener should be set as soon as possible, if not, this was the main reason for overlapping of the states.
The default views were visible, and the listener was not set yet when the recorder was locked quickly.

@@ -1766,6 +1764,7 @@ class ConversationFragment :
}

private fun updateToggleButtonState() {
inputPanel.updateComposeViews()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only place that calls this function.
updateToggleButtonState is called more than updateComposeViews() needs. But, this is a worth tradeoff.

@greyson-signal
Copy link
Contributor

Wanted to provide an update that this is a lot of code change to address the original bug report, and I'm nervous to make too many big changes here since this is one of the core interaction points in the app. Could be fine, but we'll just need to take more time to review and test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants