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

Implement audio player for audio file attachments in Chat (#618) #872

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

Conversation

oogxdd
Copy link

@oogxdd oogxdd commented Feb 26, 2024

Resolves #618

Synopsis

Сделать: отображение аудио файлов в виде плеера с возможностью аудио воспроизвести, покрыть интеграционным тестом.

Solution

  • PlayableAsset виджет расширить и произвести его редизайн под описанные выше необходимости.
  • Реализовать фичу: Файл ремоутный (FileAttachment) и файл локальный (LocalAttachment) должны иметь возможность идентифицироваться как аудио, чтобы в списке прикреплений отображаться в виде плеера.
  • Для воспроизведения аудио использовать AudioUtils класс. Завести в данном классе какое-нибудь поле или метод, которое бы гарантировало воспроизведение только одного аудио каким-нибудь образом.
  • Для покрытия тестом написать мок AudioUtils. который будет подтверждать тот факт, что вызов к аудио был совершён, через соответствующие степы.
  • Написать тесты
  • Написать документацию

Checklist

  • Created PR:
    • In draft mode
    • Name contains issue reference
    • Has type and k:: labels applied
  • Before review:
    • Documentation is updated (if required)
    • Tests are updated (if required)
    • Changes conform code style
    • CHANGELOG entry is added (if required)
    • FCM (final commit message) is posted or updated
    • Draft mode is removed
  • Review is completed and changes are approved
    • FCM (final commit message) is approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • All temporary labels are removed

@SleepySquash SleepySquash added enhancement Improvement of existing features or bugfix feature New feature or request k::UI/UX UI (user interface) and UX (user experience) changes k::refactor Refactor changes of existing code labels Feb 26, 2024
@SleepySquash SleepySquash added this to the 0.1.0-alpha.13 milestone Feb 26, 2024
@oogxdd
Copy link
Author

oogxdd commented Mar 4, 2024

FCM

Introduce AudioPlayer to the app (allow to play attached audio files)

- Implemented common `AudioPlayerInterface` to provide unified API to work with both `just_audio.AudioPlayer` and `media_kit.Player`
- Created `AudioPlayerController` which acts as a state and methods wrapper for AudioPlayer functionality
- Added `AudioAttachment` widget which displays audio files and allows to play them

@oogxdd oogxdd marked this pull request as ready for review March 4, 2024 06:28
@oogxdd
Copy link
Author

oogxdd commented Mar 4, 2024

@SleepySquash В целом готово к ревью. Единственное, пока без тестов.

Несколько комментов.

  1. Создал единый интерфейс AudioPlayerInterface для media_kit и just_audio плееров. Потом можно будет дополнять функционалом по типу addToPlaylist, setShuffleMode, etc.

  2. Уже имеющийся метод play в AudioUtils не стал использовать, потому что он создает новый инстанс плеера для каждого звука, а нам в аудио плеере это не нужно (потому что нужно будет проигрывать плейлисты етц). По-хорошему было бы отрефакторить AudioUtils полностью, но пока до этого не дошел.

  3. Пока что реализовал через GetxController (AudioPlayerController). Мне как программисту с бэкграундом в React/Redux это решение кажется логичном, но если вы считаете что это bad practice - переделаю на воркер (или просто создам дополнительный класс в audio_utils).

  4. Сейчас инициализирую контроллер при заходе на страницу чата, и убираю при выходе. В будущем, когда будет делаться глобальный плеер, надо будет перенести это в другое место.

  5. Думаю на бэкенде нужно добавить новый филд duration для аудио файлов (и компьютить его при получении файла). Потому что насколько я понимаю нет простого способа высчитать дюрейшен не прогружая файл библиотеками по типу just_audio.

lib/store/audio_player.dart Outdated Show resolved Hide resolved
lib/store/audio_player.dart Outdated Show resolved Hide resolved
lib/ui/page/home/page/chat/widget/audio_attachment.dart Outdated Show resolved Hide resolved
lib/ui/page/home/page/chat/widget/audio_attachment.dart Outdated Show resolved Hide resolved
lib/ui/page/home/page/chat/widget/audio_attachment.dart Outdated Show resolved Hide resolved
lib/store/audio_player.dart Outdated Show resolved Hide resolved
lib/store/audio_player.dart Outdated Show resolved Hide resolved
lib/domain/model/native_file.dart Outdated Show resolved Hide resolved
Comment on lines 86 to 90
if (_isMobile) {
player = JustAudioPlayerAdapter();
} else {
player = MediaKitPlayerAdapter();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Идея с интерфейсом классная. Я бы предложил, правда, перенести определение, какой плеер должен быть создан, в AudioUtils каким-нибудь отдельным методом. Примеры, как такой getter/метод бы выглядеть:

class AudioUtilsImpl {
  ...
  
  AudioPlayerInterface get player { ... }
  AudioPlayerInterface create() { ... }
  AudioPlayerInterface construct() { ... }
  
  ...
}

Аргумент такого переноса - инкапсуляция. Получится так, что за прямую работу с аудио, даже определение того, какой плеер должен быть создан, отвечает AudioUtils.

lib/ui/page/home/page/chat/controller.dart Outdated Show resolved Hide resolved
@SleepySquash
Copy link
Contributor

@oogxdd, просьба сориентировать по дедлайну, Вы переносили срок на вчера.

@oogxdd
Copy link
Author

oogxdd commented Mar 5, 2024

@SleepySquash

Добрый день. По поводу ПР:

мне потребуется пара часов чтобы заресолвить оставшиеся комменты и еще какое-то время чтобы покрыть тестом. Получится ли дедлайн на завтра перенести, чтобы не сгорел таск?

@SleepySquash
Copy link
Contributor

@oogxdd, не возражаю, хорошо.

@oogxdd
Copy link
Author

oogxdd commented Mar 8, 2024

@SleepySquash а есть может возможность разделить этот таск на имплементацию и тесты?

По сути из имплементации мне осталось:

  1. Переделать архитектуру чтобы использовать воркер (или я думаю можно даже просто использовать AudioUtils класс. Хранить там стейт и методы. not sure yet)

  2. Пройтись заного полностью по Effective Dart и Contribution Guide и убедиться что весь код чистый.

Потом будут тесты. Уже определил какие желательно заимлементить

  1. can play
  2. can pause
  3. can resume
  4. can seek
  5. see duration
  6. see current position
  7. can change tracks (previous one stops playing, new one is playing)

и для более полного покрытия:

  1. track finishes and status changes to completed (ui updated)
  2. симулировать буферизацию аудио?

Сделать железно дедлайн для первого понедельник, а тесты оставить как открытый таск (пример уже будет задан).

@SleepySquash
Copy link
Contributor

@oogxdd, идея issue сводилась как раз к 50% задача, 50% покрытие тестами, поэтому не уверен. Но отдельно посмотреть имплементацию от тестов смогу, конечно.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix feature New feature or request k::refactor Refactor changes of existing code k::UI/UX UI (user interface) and UX (user experience) changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[$250] Implement audio player for audio file attachments in Chat
2 participants