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.

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

Fixed Record Audio Waveform not visible #16258 #16372

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

shalenMathew
Copy link

@shalenMathew shalenMathew commented May 10, 2024

Purpose / Description

fixing #16258 the issue where audio wave disappears when orientation changes to landscape

Fixes

Approach

  • issue occurred due Audio Waveform being kept GONE when config changed ,
    solved the issue

  • audio form is now visible even in landscape mode

  • changed the height of audio wave a bit , to suit in both landscape and potrait mode

How Has This Been Tested?

  • tested in emulator

Screenshot

WhatsApp.Video.2024-05-10.at.11.45.32.PM.mp4

WhatsApp Image 2024-05-10 at 11 46 27 PM

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link

welcome bot commented May 10, 2024

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@@ -42,7 +42,7 @@
<com.ichi2.audio.AudioWaveform
android:id="@+id/audio_waveform_view"
android:layout_width="match_parent"
android:layout_height="300px"
android:layout_height="200px"
Copy link
Member

Choose a reason for hiding this comment

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

Could you test that with a height using dp? 200px may make sense for phones, but not on tablets

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label May 10, 2024
@david-allison
Copy link
Member

Thanks!!

There was a reason that the audio waveform was hidden in landscape (I believe due to limited vertical space on phones)

This doesn't address the issue that the flip from landscape to portrait does not reveal the control

@david-allison
Copy link
Member

Forgot to ping, @criticalAY has context for the landscape change hiding the control

@criticalAY
Copy link
Contributor

The audio wave form was kept hidden in landscape as the mobile screen don't have that much of space to accommodate both the waveform and the card content, when a user want to record while seeing the content of their card it is difficult if it's visible in landscape

Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

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

Thanks! but this is not the solution instead try fixing the original issue

@shalenMathew
Copy link
Author

So whats the fix here ? Should i make any further changes ?

@shalenMathew
Copy link
Author

Shall i change px to dp ?

@criticalAY
Copy link
Contributor

You will need to fix the behavior mentioned in the issue which is emulator only

@shalenMathew
Copy link
Author

You will need to fix the behavior mentioned in the issue which is emulator only

Ok 👌

@david-allison
Copy link
Member

@shalenMathew could you mark this as draft if it's not ready for review?

@shalenMathew
Copy link
Author

@shalenMathew could you mark this as draft if it's not ready for review?

I have changed the size of audio waveform to 100dp just notify me if i need to make any more changes

@criticalAY
Copy link
Contributor

This is not what I meant, I will repeat what I said, you need to figure out what is the issue that causes the emulator bug and fix that instead of changing the height

@shalenMathew
Copy link
Author

shalenMathew commented May 24, 2024

This is not what I meant, I will repeat what I said, you need to figure out what is the issue that causes the emulator bug and fix that instead of changing the height

ok , i think it not a bug the waveform disappears coz the logic is set up that way , when the orientation changes the waveform visiblity is set to gone , i think due to limited space of screen on landscape mode the logic is set up that way...

I think the logic is works as intended ... Maybe its not a bug

@david-allison
Copy link
Member

@shalenMathew first, you need to understand the problem:

  • We want the view hidden in landscape mode
  • We don't want the view hidden in portrait mode
  • If the user switches from portrait to landscape, then from landscape to portrait, the view is invisible in portrait, this is a bug

The first step is to reproduce the problem in an emulator

Then explain what the bug is

Then fix the bug in a pull request

@shalenMathew
Copy link
Author

@shalenMathew first, you need to understand the problem:

  • We want the view hidden in landscape mode
  • We don't want the view hidden in portrait mode
  • If the user switches from portrait to landscape, then from landscape to portrait, the view is invisible in portrait, this is a bug

The first step is to reproduce the problem in an emulator

Then explain what the bug is

Then fix the bug in a pull request

ok now i got it my bad, let me try to fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author New contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Emulator-only?] Record Audio Waveform not visible
4 participants