Implement audio player for audio file attachments in Chat #618 by DaemonWoo · Pull Request #1619 · team113/messenger

DaemonWoo · 2026-02-24T06:12:56Z

Resolves #618

Synopsis

Display audio files as a player with audio playback option, cover with integration test.

Solution

Implement a global AudioPlayer class to handle audio files playback, including seek, pause, and duration tracking.
Cover the functionality with tests.

Checklist

SleepySquash

@DaemonWoo, I kindly ask you to look at the whole code of the task, take a look at every single file, and try to fix the issues beforehand, try to reproduces the bugs I've described and do something with the mock, please. I did some renaming, because ActiveAudioSession sounds a little bit wrong to me, I'm used to think of an "audio session" as a whole audio hardware configuration the app uses currently.

And I would like to quote one of the comments I've left, since it seems to be important, if you don't mind!

It seems like you may in a hurry in resolving the comments. Please, don't do that. It leads to sometimes minor, sometimes major difficulties that may be hard to catch.

And such things could also lead to the long reviews 🥲

SleepySquash · 2026-03-26T10:12:17Z

    await Get.deleteAll();
+    Get.put<AudioWorker>(MockAudioWorker());



Why would you put the MockAudioWorker in both onAfterRun and onBeforeRun? This hook is invoked two times per test:

onBeforeRun /* test 1 executes */ onAfterRun // <- First time onBeforeRun // <- Second time /* test 2 executes */ ... etc

Shouldn't it be put only once per run? Like it does at appInitializationFn() function. Actually, I don't see a reason to use this hook at all, since appInitializationFn() already should be invoked for every app, which happens once per each test, isn't it?

At first, it was used once in appInitializationFn(). However, i started getting an error after each scenario saying that AudioWorker is not found. I agree that there's no need to call it everywhere, i think it should only be called in OnAfterRun.

@DaemonWoo, "not found" is a problem. It shouldn't be fixed with adding the worker every time manually. What is the reason, why does every single other mock work perfectly without such an error and only the audio worker throws that error? Such behaviour shouldn't be ignored, perhaps there's an issue somewhere deeper and more dangerous than it seems?

This happens because of Get.deleteAll() in restart_app. Even if we call Get.put<AudioWorker>() again (similar to how we do it for GraphQL and GeoLocation providers), the error still occurs. The reason is that AudioPlayerView is not destroyed yet and continues trying to find AudioWorker.
If we make AudioWorker permanent in appInitializationFn, or reregister it in onAfterScenario, the error no longer occurs.

@DaemonWoo,

This happens because of Get.deleteAll() in restart_app

That's exactly how it should be. When an app is restarted, the whole dependency tree is destroyed, isn't it?

The reason is that AudioPlayerView is not destroyed yet and continues trying to find AudioWorker.

That's a problem. Why? The view isn't displayed to anyone. It should be destroyed when an app is restarted. When you destroy the process OS runs the app in, and then launch a new process, this view is created again. And that's how this restart step should work ideally.

@SleepySquash I think GetX dependencies are destroyed first, while the view widget is still offstage and able to rebuild. And this error occurs during that brief moment before this view is created again

If making AudioWorker dependency permanent is bad, we can add And I return to previous page step at the end of each scenario. This way view will dispose cleanly, and only after that Get.deleteAll() would be triggered

@DaemonWoo,

I think GetX dependencies are destroyed first, while the view widget is still offstage and able to rebuild

Then a view widget should get destroyed first. How can this be done? Perhaps the restart step doesn't do what it does? And it bothers me that it's the only mock that fails this way, other ones are working as they should.

I don't like an idea to fix the results of the problem and to ignore the problem itself. At least we should have an idea of what's going on in order to determine what to do.

@SleepySquash I was wrong about restart_app step, we don't use it here. I think this error occurs when Flutter might still be in the process of unmounting or rebuilding AudioPlayerView during cleanup/navigation transitions. And when it tries to rebuild this view, GetX dependencies have already been cleaned up. If we navigate away from chat before onAfterScenario the error is gone, because AudioPlayerView has time to be fully destroyed.

SleepySquash · 2026-03-26T11:28:16Z

+  'I see {string} audio is paused',
+  (name, context) async {
+    await context.world.appDriver.waitForAppToSettle();
+
+    final AudioWorker worker = Get.find<AudioWorker>();
+    final AudioId id = _findAudioId(name);
+
+    final bool isPaused =
+        (worker.activeSession.value?.item.id == id &&
+            !worker.activeSession.value!.isPlaying) ||
+        (worker.activeSession.value?.item.id != id);
+
+    expect(isPaused, true);
+  },


When doing such steps, you may use a single step with different parameters. It reduces the code and makes it more strict.

Also you should ideally do a await context.world.appDriver.waitUntil(() async {}), because E2E tests run in real-time, unlike unit or widget tests, and a lot of delaying stuff can happen in a real-time environment. Sometimes a network connection may drop, and in order for something to happen a ~1-2 seconds delay might happen, etc. So we ideally should "wait until the status is what we expect for the status to be".

SleepySquash · 2026-03-27T10:28:20Z

Also, in Web version I get the following behaviour:

Screen.Recording.2026-03-27.at.13.27.25.mov

After a second drag for some reason the audio pauses.

In HomeView, I access Playback directly via public getter and expose it to UI, while in AudioPlayerView I interact with it only through the controller, since each method needs to verify that exactly this view is active. Is it okay to have two different approaches for interacting with playback?

@DaemonWoo, what's your opinion? Do you think it's ok or not? And why?

@SleepySquash I think it does introduce some inconsistency, so it would be better to encapsulate playback in HomeController and expose what the view actually needs. This way, if we change the playback implementation, we won't need to modify HomeView

@DaemonWoo, we'll still need to modify the HomeController in this case, I guess?

@SleepySquash Yes, we will

SleepySquash · 2026-04-03T08:26:02Z

@DaemonWoo, you shouldn't push a commit after requesting a review without a notice. You can't be sure that no one is working with this branch currently after you've sent it for a review, and checking the comments and rebasing can be cumbersome things to do. Please, either notify about a commit being made beforehand, or request a review only when you are absolutely sure about the code.

SleepySquash · 2026-04-02T12:22:29Z

+      if (isAudio) {
+        AudioItem? item;
+        Widget? progress;
+
+        if (e is LocalAttachment && e.file.path != null) {
+          item = AudioItem(
+            id: widget.audioId ?? AudioId('${e.id}'),
+            source: AudioSource.file(e.file.path!),
+            title: e.filename,
+          );
+          progress = WidgetButton(onPressed: e.cancelUpload, child: leading);
+        } else if (e is FileAttachment) {
+          item = AudioItem(
+            id: widget.audioId ?? AudioId('${e.id}'),
+            source: AudioSource.url(e.original.url),
+            title: e.filename,
+          );
+        }
+
+        if (item != null) {
+          return AudioPlayer(
+            item: item,
+            progress: progress,
+            onForbidden: e is FileAttachment
+                ? () async {
+                    await widget.onForbidden?.call();
+                    return AudioSource.url(e.original.url);
+                  }
+                : null,
+          );
+        }


I don't really like the fact that all of that happens every single time the build() method is executed. This means that all of those AudioItems are being recreated every build().

SleepySquash · 2026-04-02T12:27:03Z

+        if (item != null) {
+          return AudioPlayer(
+            item: item,
+            progress: progress,


This progress is leading, not a progress, because it doesn't has to display any progress. I may do something like progress: const Icon(...) or even progress: Text(...), which breaks the naming.

SleepySquash · 2026-04-02T12:27:50Z

+  /// Indicates uploading progress.
+  final Widget? progress;


A property should have a property documentation.

SleepySquash · 2026-04-02T12:29:31Z

+  /// Callback, called when [source] fetch fails with `403` status code.
+  final Future<AudioSource?> Function()? onForbidden;


What is [source]?

SleepySquash · 2026-04-02T12:30:57Z

+    return GetBuilder(
+      init: AudioPlayerController(
+        Get.find(),
+        item: item,
+        onForbidden: onForbidden,
+      ),
+      tag: item.id.val,
+      builder: (AudioPlayerController c) {


It'd be a more appropriate for that construction to be a stateful widget rather than a view + controller, since this ain't a page. However, let it be this way.

SleepySquash · 2026-04-02T13:27:01Z

+      if (_playRequestId == playId) await stop();
+    } finally {
+      if (_playRequestId == playId) _delegate.isLoading.value = false;


You sometimes use the _isStale() method, and sometimes don't. Since such misuse is already happening, then is there a reason to have this method at all?

SleepySquash · 2026-04-06T06:02:20Z

+/// Mocked [AudioWorker] to use in the tests.
+class MockAudioWorker extends AudioWorker {
+  MockAudioWorker() : super(delegate: DummyDelegate());
+}


Do we need this class at all if we could just do AudioWorker(DummyDelegate())?

SleepySquash · 2026-04-06T06:04:29Z

+        break;
+      }
+
+      position.value = position.value + const Duration(seconds: 1);


Can it exceed the total Duration this way?

SleepySquash · 2026-04-06T06:11:41Z

+
+    When I pause "test.mp3" audio
+    Then I see "test.mp3" audio is paused
+    And I return to previous page


Adding this I return to previous page still seems like a workaround of the issue instead of a solution. I can imagine this error happening currently in tests to happen in a real world scenario. For example, when application's business logic is being deconstructed due to an account switch, or due to a logout. Or even when the app is being closed. Since you first assumed that the problem is within restart_app step, which happened to be a different thing, can you please deconstruct the exception? This "not found" error should be happening due to some method being executed in the widget, I guess? Or why? Let's try to pinpoint the problem, and only afterwards decide as to how it should be fixed, please

SleepySquash · 2026-04-06T06:41:18Z

+      final cachedDuration = _durationCache[item.id];
+      if (cachedDuration != null && cachedDuration != Duration.zero) {
+        _delegate.duration.value = cachedDuration;
+      }


You should add comments describing what you are doing in such cases, because it's only obvious to you currently. And after some time even you can forget what this code does.

SleepySquash · 2026-04-06T13:44:15Z

@DaemonWoo, our headquarters took a look at this PR and have expressed a slight disturbance due to this freelance task having so much review cycles, with amount of review comments not decrementing. This means that this task isn't what we are willing to keep this way, it grew a lot and took too much time already, with its completion date still being unknown. Since this project isn't a charity or some kind of personal project, I've been asked to make a sad decision to stop this cycle, break it at this point. I've invested my personal resources as well, so it's unfortunate for me as well. However, company standards don't allow me to do anything about it.

At this point I'll close the PR and unassign the issue. However, if you would like to, you can always try again with a different issue.

Thank you very much for all this time, all this effort you've put into the code!

create audio_player widget

7e8cb78

DaemonWoo changed the title ~~Implement audio player~~ Implement audio player for audio file attachments in Chat #618 Feb 24, 2026

DaemonWoo added 8 commits February 25, 2026 16:09

add audio recognition

11e1695

fix audio recognition

3e53af2

add documentation && refactor

885a302

revert redundant

54a9494

revert fix

d61d2ad

fix id and onCompleted

357afba

add copyright && improve code&& add tests

47bc230

add audio controller

ab9bd10

DaemonWoo force-pushed the feature/audio-player branch from e5aa03c to ab9bd10 Compare March 1, 2026 10:34

DaemonWoo added 4 commits March 1, 2026 18:59

tests

d85502f

fmt

d5968d0

add audio progress uploading

27a79e8

fixes

67da4f1

SleepySquash added this to the 0.10.0 milestone Mar 2, 2026

DaemonWoo and others added 14 commits March 2, 2026 10:49

refactor view to stateless

5961e78

fix controller && refactor test steps

0c3bd18

add test step

a8229cc

fix playback on ios & macos

6aaa1b3

enhancements

afa8c5c

enhancements && add tests && fix bugs

ad86dae

fix narrow jump onPlay

691a40e

fix redundant

b124f57

delete redundant

626ca33

revert progress changes

4b1aadd

change comment

0113713

sort fields

b071a6a

Merge remote-tracking branch 'origin/main' into feature/audio-player

4d0588f

Corrections

5370186

SleepySquash suggested changes Mar 27, 2026

View reviewed changes

DaemonWoo added 9 commits March 28, 2026 18:30

corrections && fix play nulling duration

6715e67

fix seek

5b4b060

enhancements

ec5f71e

make MockAudioWorker permanent

408e09f

delete redundant in tests

8dc23ed

uniform doc comment

30a33ca

refactor AudioPlayback

1b25887

fix tests

88e454e

update docs

e3b504b

DaemonWoo requested a review from SleepySquash April 1, 2026 09:02

DaemonWoo added 3 commits April 1, 2026 13:19

enhance controller

2e407fc

move _slider into AudioPlayer class

40bc509

fix async

9e9721d

SleepySquash modified the milestones: 0.10.1, 0.11.0 Apr 6, 2026

SleepySquash added 2 commits April 6, 2026 09:00

Corrections

6e68fdb

Merge remote-tracking branch 'origin/main' into feature/audio-player

390ddd1

SleepySquash suggested changes Apr 6, 2026

View reviewed changes

Corrections

72545e4

SleepySquash assigned DaemonWoo Apr 6, 2026

SleepySquash closed this Apr 6, 2026

		await Get.deleteAll();
		Get.put<AudioWorker>(MockAudioWorker());

		/// Callback, called when [source] fetch fails with `403` status code.
		final Future<AudioSource?> Function()? onForbidden;

Conversation

DaemonWoo commented Feb 24, 2026

Synopsis

Solution

Checklist

Uh oh!

SleepySquash left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaemonWoo Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SleepySquash commented Apr 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SleepySquash commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

DaemonWoo Mar 30, 2026 •

edited

Loading