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

#1407 improvements to audio settings #1429

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

Conversation

sempsmahad
Copy link

1.When I play a Surat then I go into settings to chose to play from what Surat to what Surat and I click play only above verses, the audio of the Surat will restart. It really shouldn't have to restart and should just apply the settings and keep playing. It makes it harder for the kids to focus because it repeats when I apply the settings.

A. removed the part where audio playback is stopped if playing now it pauses and continues on pressing play if in selected range restarts to new start ayah if not in selected range

2.There should be an option to remember the last selected setting being "play only above verses". I'm always having to manually select it.

A.modified 'refreshview()' to read from former used settings which where stored in sharedprefferences on pressing apply button

1.When I play a Surat then I go into settings to chose to play from what Surat to what Surat and I click play only above verses, the audio of the Surat will restart. It really shouldn't have to restart and should just apply the settings and keep playing. It makes it harder for the kids to focus because it repeats when I apply the settings.

A. removed the part where audio playback is stopped if playing now it pauses and continues on pressing play if in selected range restarts to new start ayah if not in selected range

2.There should be an option to remember the last selected setting being "play only above verses". I'm always having to manually select it.

A.modified 'refreshview()' to read from former used settings which where stored in sharedprefferences on pressing apply button
Copy link
Contributor

@ahmedre ahmedre left a comment

Choose a reason for hiding this comment

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

jazakumAllah khairan, this is a much needed change. left some minor comments.

@@ -172,6 +172,7 @@ dependencies {
androidTestImplementation "androidx.test.espresso:espresso-core:${espressoVersion}"
androidTestImplementation "androidx.test.espresso:espresso-intents:${espressoVersion}"
implementation 'com.crashlytics.sdk.android:crashlytics:2.10.1'
implementation 'com.google.code.gson:gson:2.8.6'
Copy link
Contributor

Choose a reason for hiding this comment

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

we already use moshi so let's not add gson

Comment on lines +1 to +12
package com.quran.labs.androidquran.dao.audio

import android.os.Parcelable
import com.quran.data.model.SuraAyah
import kotlinx.android.parcel.Parcelize

@Parcelize
data class AudioPlaybackSettings(val start: SuraAyah,
val end: SuraAyah,
val verseRepeatCount: Int = 0,
val rangeRepeatCount: Int = 0,
val enforceRange: Boolean = false) : Parcelable
Copy link
Contributor

Choose a reason for hiding this comment

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

what if, instead of doing this, we remove this class (and the corresponding changes to read/write this) - and instead do the work in the AudioService?

Comment on lines -418 to -422
if (player != null) {
player.stop();
}
state = State.Stopped;
Crashlytics.log("stop if playing...");
Copy link
Contributor

Choose a reason for hiding this comment

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

stopping is sometimes the right thing to do - but this is where we should check. you have the old audioRequest (before we override it in line 409). we can check for example if the current verse is within the range of playback (i.e. between oldAudioRequest.getStart() and oldAudioRequest.getEnd()), then don't stop. otherwise stop, and in either case, we'll replace the audioRequest.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the reply @ahmedre I will work on the changes as soon as possible
Ma'Assalam

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.

2 participants