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

Adds dual samples for many sounds #286

Merged
merged 9 commits into from Aug 20, 2020
Merged

Adds dual samples for many sounds #286

merged 9 commits into from Aug 20, 2020

Conversation

ashutoshgngwr
Copy link
Member

@ashutoshgngwr ashutoshgngwr commented Aug 19, 2020

Changes

  • Sound declarations can now have multiple source samples to play simultaneously
  • Make necessary changes to the Chromecast receiver applications as well
  • Optimize graphics for Chromecast UI (should help with OOM errors on speaker devices [just a guess though])
  • Split existing relevant sounds into two samples (key result: reduction in size on disk by 42%)

Testing

  • Tested on a physical device
  • Added or modified unit test cases

Others

TODO

  • Add a different icon for casting status on cast-receiver
  • Fix app icon in cast receiver (icon in fastlane metadata is not transparent anymore)
  • Rename PlayerManagerStatusEvent to PlayerManagerStatus

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #286 into master will increase coverage by 0.17%.
The diff coverage is 93.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   85.37%   85.55%   +0.17%     
==========================================
  Files          26       29       +3     
  Lines        1272     1440     +168     
  Branches      147      172      +25     
==========================================
+ Hits         1086     1232     +146     
- Misses         89      113      +24     
+ Partials       97       95       -2     
Flag Coverage Δ
#android 84.61% <92.85%> (-0.77%) ⬇️
#cast_receiver 95.27% <94.11%> (?)
#ui_tests 63.91% <65.17%> (+0.77%) ⬆️
#unit_tests 45.60% <87.81%> (+5.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cast-receiver/src/library.mock.ts 100.00% <ø> (ø)
.../ashutoshgngwr/noice/sound/player/PlayerManager.kt 82.20% <50.00%> (ø)
...noice/cast/player/strategy/CastPlaybackStrategy.kt 60.00% <66.66%> (ø)
...ice/sound/player/strategy/LocalPlaybackStrategy.kt 64.81% <86.66%> (-27.35%) ⬇️
cast-receiver/src/player_manager.ts 94.18% <90.74%> (ø)
...hutoshgngwr/noice/fragment/SoundLibraryFragment.kt 87.50% <91.66%> (+0.37%) ⬆️
...java/com/github/ashutoshgngwr/noice/sound/Sound.kt 98.14% <98.41%> (-0.49%) ⬇️
.../github/ashutoshgngwr/noice/sound/player/Player.kt 92.10% <100.00%> (ø)
cast-receiver/src/status_ui_handler.ts 97.43% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d24117...f530f98. Read the comment docs.

Copy link
Member Author

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 13
- Added 4
           

See the complete overview on Codacy

return;
}
handlePlayerEvent(event: PlayerControlEvent): void {
event.src.forEach((soundKey: string) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

*/
showIdleStatus(): void {
if (this.isShowingIdleStatus() === true) {
enableStatus(statusID: string, enabled: boolean): void {
Copy link
Member Author

Choose a reason for hiding this comment

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

}

private createIcon(id: string): HTMLElement {
const template = document.createElement("template");
template.innerHTML = `<svg id="${id}" class="sound"><use xlink:href="${Icons[id]}" /><svg>`;
template.innerHTML = `<svg id="${id}" class="icon"><use xlink:href="${Icons[id]}" /><svg>`;
Copy link
Member Author

Choose a reason for hiding this comment

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

Codacy found an issue: Unsafe assignment to innerHTML

}

private createIcon(id: string): HTMLElement {
const template = document.createElement("template");
template.innerHTML = `<svg id="${id}" class="sound"><use xlink:href="${Icons[id]}" /><svg>`;
template.innerHTML = `<svg id="${id}" class="icon"><use xlink:href="${Icons[id]}" /><svg>`;
Copy link
Member Author

Choose a reason for hiding this comment

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

Codacy found an issue: Generic Object Injection Sink

@ashutoshgngwr ashutoshgngwr merged commit 809f131 into master Aug 20, 2020
@ashutoshgngwr ashutoshgngwr deleted the feat/dual-samples branch August 20, 2020 08:57
@ashutoshgngwr ashutoshgngwr added this to the 0.10.x milestone Aug 21, 2020
@Lesik
Copy link

Lesik commented Nov 24, 2020

Hi @ashutoshgngwr,

first off, thanks a lot for this great app!

I noticed that this MR modified app/src/main/assets/airplane_seatbelt_beeps.mp3, I assume related to the "Split existing relevant sounds into two samples". Before the change, the sound consisted of two beeps, high-middle. After the change, it is three beeps low-high-middle.

I was wondering if this change was deliberate, as according to my understanding this MR should only split effects into separate files to playback intertwinedly, not change the content of the sounds. In case it was an accident, I would be very happy if you could revert the change to this particular effect, as it was one of my favorite effects in the library and in my personal opinion the third tone quite ruins it.

Before: https://raw.githubusercontent.com/ashutoshgngwr/noice/7f4d5fa97e460411924bc2b4b5422a776fbde06d/app/src/main/assets/airplane_seatbelt_beeps.mp3

After: https://raw.githubusercontent.com/ashutoshgngwr/noice/d6660000c2bac06928ef6730442ed45466fddbdf/app/src/main/assets/airplane_seatbelt_beeps.mp3

Thanks!

@ashutoshgngwr
Copy link
Member Author

@Lesik it was a deliberate change. Long story short, original sample had 3 beeps (current version) but I trimmed one beep in earlier versions (personal preference). Don't remember why I changed it though. Feel free to submit a PR reverting to old sample. :)

@Lesik
Copy link

Lesik commented Nov 25, 2020

@ashutoshgngwr I was going to write "if you could accept a MR" but then I realized that many samples (including the seatbelt beeps) have been equalized since (#313). Just reverting the original file would result in an unequalized sample, so it's not that easy for me to submit a MR. Could you document the process you used for equalization?

@ashutoshgngwr
Copy link
Member Author

ashutoshgngwr commented Nov 25, 2020

@Lesik #227 (comment)

Sorry I am caught up somewhere else. Maybe you could add the instructions from that comment in the adding sounds section of CONTRIBUTING.md doc? It's fine if you can't. Will add myself later. Thanks.

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.

Dual Samples
2 participants