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

Share Audio Feature #2072

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

Share Audio Feature #2072

wants to merge 19 commits into from

Conversation

ahmedre
Copy link
Contributor

@ahmedre ahmedre commented Sep 11, 2022

No description provided.

@ahmedre ahmedre changed the title feat share audio Share Audio Feature Sep 11, 2022
@ahmedre
Copy link
Contributor Author

ahmedre commented Sep 11, 2022

This is @DoozyDoz's pull request from #2011, but continued here so I can have write access. I've done a set of fixes and moved some things around, but it's not ready yet.

@ahmedre
Copy link
Contributor Author

ahmedre commented Sep 11, 2022

Things that are left before merging this PR:

  • better handling for cases when the audio isn't downloaded (today, it requests download, but doesn't try to share afterwards - we should either have proper messaging or fix this).
  • sharing is only for gapless audio today, so either disable for gapless qaris or support them
  • there is no "loading spinner" while the data is being exported
  • the majority of the code added to PagerActivity should move out
  • cleanup of AudioShareUtils
  • reconsider text sharing icon (the new icon, while nice, is not intuitive - why does it represent "sharing text?")

@github-actions
Copy link

OLD: app-madani-debug.apk (signature: V1, V2)
NEW: app-madani-debug.apk (signature: V1, V2)

          │            compressed             │           uncompressed            
          ├───────────┬───────────┬───────────┼───────────┬───────────┬───────────
 APK      │ old       │ new       │ diff      │ old       │ new       │ diff      
──────────┼───────────┼───────────┼───────────┼───────────┼───────────┼───────────
      dex │  11.6 MiB │  11.6 MiB │ +14.9 KiB │  29.3 MiB │  29.4 MiB │ +30.3 KiB 
     arsc │   1.8 MiB │   1.8 MiB │  +2.2 KiB │   1.8 MiB │   1.8 MiB │  +2.2 KiB 
 manifest │   5.6 KiB │   5.6 KiB │      +1 B │  27.6 KiB │  27.6 KiB │       0 B 
      res │   1.1 MiB │   1.1 MiB │ +20.3 KiB │   1.3 MiB │   1.3 MiB │ +19.5 KiB 
    asset │ 404.2 KiB │ 404.2 KiB │       0 B │ 678.6 KiB │ 678.6 KiB │       0 B 
    other │ 173.5 KiB │ 174.3 KiB │    +845 B │ 336.9 KiB │ 339.1 KiB │  +2.3 KiB 
──────────┼───────────┼───────────┼───────────┼───────────┼───────────┼───────────
    total │  15.1 MiB │  15.1 MiB │ +38.2 KiB │  33.4 MiB │  33.5 MiB │ +54.3 KiB 


@github-actions
Copy link

New Dependencies
project :common:util
project :common:util (*)
project :feature:audioshare

@ahmedre
Copy link
Contributor Author

ahmedre commented Sep 11, 2022

A bigger question though is whether or not we should continue with this PR and merge it in the first place - while the above issues can be worked through and resolved, there remains one major issue that's difficult to work around - and that is the audio timings are inherently inaccurate. This can easily be tested by sharing a single ayah - it feels unnatural unless the timings are precise.

My worry is that introducing this feature will greatly increase support requests due to inaccurate timings causing shared audio to be cut.

curious to what you think @nacer80 / @benomaire

@ahmedre
Copy link
Contributor Author

ahmedre commented Sep 11, 2022

One more question - today, this code supports sharing across suras (i.e. share a file between sura 113 ayah 1 through the end of sura 114) - if we are to continue with this, I am thinking we should restrict this to sharing within one sura only. In addition to making the code simpler, guessing in most cases, it makes sense to share an ayah or set of ayat from one sura rather than a range across suras.

@github-actions
Copy link

OLD: app-madani-debug.apk (signature: V1, V2)
NEW: app-madani-debug.apk (signature: V1, V2)

          │            compressed             │           uncompressed            
          ├───────────┬───────────┬───────────┼───────────┬───────────┬───────────
 APK      │ old       │ new       │ diff      │ old       │ new       │ diff      
──────────┼───────────┼───────────┼───────────┼───────────┼───────────┼───────────
      dex │  11.6 MiB │  11.6 MiB │ +14.4 KiB │  29.3 MiB │  29.4 MiB │ +29.2 KiB 
     arsc │   1.8 MiB │   1.8 MiB │  +2.2 KiB │   1.8 MiB │   1.8 MiB │  +2.2 KiB 
 manifest │   5.6 KiB │   5.6 KiB │      +1 B │  27.6 KiB │  27.6 KiB │       0 B 
      res │   1.1 MiB │   1.1 MiB │ +20.3 KiB │   1.3 MiB │   1.3 MiB │ +19.5 KiB 
    asset │ 404.2 KiB │ 404.2 KiB │       0 B │ 678.6 KiB │ 678.6 KiB │       0 B 
    other │ 173.5 KiB │ 174.3 KiB │    +846 B │ 336.9 KiB │ 339.1 KiB │  +2.3 KiB 
──────────┼───────────┼───────────┼───────────┼───────────┼───────────┼───────────
    total │  15.1 MiB │  15.1 MiB │ +37.7 KiB │  33.4 MiB │  33.5 MiB │ +53.1 KiB 


@github-actions
Copy link

New Dependencies
project :common:util
project :common:util (*)
project :feature:audioshare

@nacer80
Copy link

nacer80 commented Sep 11, 2022

السلام عليكم ورحمة الله وبركاته

We will faced tow issues some timings:

1- if the recitation of Ayah shared doesn't stop in the end of Ayah (Sheikh recited tow Ayahs, so the there is letter between the end of Ayah and the beginning of the second)
2- if the timing doesn't accurate.

@DoozyDoz
Copy link
Contributor

One more question - today, this code supports sharing across suras (i.e. share a file between sura 113 ayah 1 through the end of sura 114) - if we are to continue with this, I am thinking we should restrict this to sharing within one sura only. In addition to making the code simpler, guessing in most cases, it makes sense to share an ayah or set of ayat from one sura rather than a range across suras.

Same thoughts but I think it's okay since it includes basmala/isthaadha where necessary. The resultant audio file is simply like 2 surahs being played one after the other which is normal, right?

@DoozyDoz
Copy link
Contributor

A bigger question though is whether or not we should continue with this PR and merge it in the first place - while the above issues can be worked through and resolved, there remains one major issue that's difficult to work around - and that is the audio timings are inherently inaccurate. This can easily be tested by sharing a single ayah - it feels unnatural unless the timings are precise.

My worry is that introducing this feature will greatly increase support requests due to inaccurate timings causing shared audio to be cut.

curious to what you think @nacer80 / @benomaire

Is there any way I can be of help in this regard? how can one update the timing data if they wish to? what tools were used to time the initial set of gapless timings?

@ahmedre
Copy link
Contributor Author

ahmedre commented Apr 5, 2023

we can at least enable it for one qari - so your sharing is always shared with one qari whose timings are pretty accurate - one of the slower qaris probably. will try to resume this at some point in the future in sha' Allah.

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.

None yet

3 participants