Skip to content

feat: enhance waveform animation, extraction flexibility and state management#424

Open
benco8186 wants to merge 5 commits into
SimformSolutionsPvtLtd:mainfrom
benco8186:main
Open

feat: enhance waveform animation, extraction flexibility and state management#424
benco8186 wants to merge 5 commits into
SimformSolutionsPvtLtd:mainfrom
benco8186:main

Conversation

@benco8186

@benco8186 benco8186 commented Apr 30, 2025

Copy link
Copy Markdown

This pull request introduces key improvements to both waveform animation behavior, waveform extraction configuration, and state management in AudioFileWaveforms:

🎨 Animation Behavior

  • Made grow animation conditional
    The waveform grow animation is now disabled if animationDuration is set to
    Duration.zero. This allows fully static rendering when animations are not desired.

  • Improved documentation
    Updated the comment for animationDuration to clearly state that Duration.zero disables animations.

⚙️ Waveform Extraction Flexibility

  • Introduced WaveformExtractionType enum
    Enables developers to choose from:

    • none: disable waveform extraction completely.
    • async: extract waveform data asynchronously.
    • sync: extract waveform data immediately during preparePlayer.
  • Exposed internal waveformData list
    Removed the .toList() wrapping to allow direct, in-place modification of
    _waveformData, which is useful for dynamic updates or streaming use cases.

  • Updated preparePlayer
    Modified the player setup logic to respect the new extraction mode and ensure
    correct handling of waveform data.

🔄 State Management Improvements

  • Added didUpdateWidget implementation
    Implemented the didUpdateWidget method to properly detect and handle changes in widget properties,
    particularly waveformData. This ensures the widget updates correctly when its properties change
    without requiring a complete rebuild.

  • Enhanced property change detection
    Added checks for other important properties (playerWaveStyle, size, waveformType) that require
    a UI refresh when changed.

  • Improved UI variable updates
    Added update logic for UI-related variables (margin, padding, decoration, etc.) to ensure
    consistent rendering when these properties change.

  • Fixed waveform color restoration
    Added a mechanism to properly initialize audio progress when recreating a widget with a paused controller,
    ensuring that waveform colors (live vs fixed) are correctly applied. This fixes an issue where waveforms
    would display only in the fixed color when a controller was paused before widget destruction and recreation.

These changes improve performance, customization, control, and reactivity for developers working with audio waveform rendering in Flutter.

…directly

- Added `WaveformExtractionType` to choose between different waveform extraction modes (no extraction, asynchronous extraction, and synchronous extraction).
- Fixed waveformData mutation issue by exposing the internal `_waveformData` list directly instead of a copy (`toList()`), enabling in-place modifications.
- Updated `preparePlayer` method to integrate these changes with improved waveform extraction handling.
Implement didUpdateWidget method in _AudioFileWaveformsState to detect changes
in waveformData and other important properties. This modification allows the
widget to update correctly when its properties are modified without having to
completely rebuild it.
…troller

This commit fixes an issue where waveforms would only display in the fixed color
when a controller was paused before the widget was destroyed and then recreated.
The fix adds a new _initializeAudioProgress method that properly initializes
the audio progress value based on the current position of a paused controller,
ensuring that the waveform colors (live vs fixed) are correctly applied when
the widget is recreated.
@benco8186 benco8186 changed the title feat: enhance waveform animation and extraction flexibility feat: enhance waveform animation, extraction flexibility and state management May 2, 2025

@ujas-m-simformsolutions ujas-m-simformsolutions left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I will need to test this locally but from the surface this PR looks good with these changes. BTW thank you so much for your PR. This is a great improvement.

Comment on lines +201 to +203
if (widget.animationDuration != Duration.zero) {
_growingWaveController.dispose();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please explain why you're only disposing when animation duration isn't zero?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

because the controller is not instantiated over the duration is set to zero

}
}

// Vérifier si d'autres propriétés importantes ont changé

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add comment in english

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry, I forgot.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

^^

if (mounted) setState(() {});
}

// Mettre à jour les variables si nécessaire

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here as well, add comment in english

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sorry, I forgot.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

^^

Comment on lines +147 to +158
if (waveformExtractionType != WaveformExtractionType.noExtraction) {
var extraction = waveformExtraction.extractWaveformData(
path: path,
noOfSamples: noOfSamples,
)
.then(
(value) {
)..then((values) {
waveformExtraction.waveformData
..clear()
..addAll(value);
notifyListeners();
},
);
..addAll(values);
});
if (waveformExtractionType == WaveformExtractionType.extractSync) {
await extraction;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you add some comment above this to explain the logic here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've changed the verifiable shouldExtractWaveform to enum, to allow for different cases. Don't extract waveforms, extract but don't wait for extraction to finish and finally extract and wait for extraction to finish.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add the comments in the code file explaining the logic

/// This returns waveform data which can be used by [AudioFileWaveforms]
/// to display waveforms.
List<double> get waveformData => _waveformData.toList();
List<double> get waveformData => _waveformData;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we don't want to allow users to modify the list but if you had any reason to remove it then can please mention it here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've changed it because this part of the code you've written isn't working properly any more.
if (shouldExtractWaveform) { waveformExtraction .extractWaveformData( path: path, noOfSamples: noOfSamples, ) .then( (value) { waveformExtraction.waveformData ..clear() ..addAll(value); notifyListeners(); }, ); }
Since you return a newly created list every time you call waveformData, what you do in the PlayerController cannot work and be made persistent.

@ujas-m-simformsolutions ujas-m-simformsolutions added the waiting-for-response Waiting for someone to respond. label May 7, 2025
@benco8186

Copy link
Copy Markdown
Author

What's the status of the review for the changes I proposed?

},
);
..addAll(values);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the async path, the old code called notifyListeners() inside this .then once extraction finished — that's gone now. For extractAsync, the data fills waveformData but listeners are never told, and AudioFileWaveforms (non-continuous) only repaints from the controller's listener, so the waveform stays blank until an unrelated notify (play/pause). Could we restore notifyListeners() here? Also worth adding an onError to the .then — otherwise a platform extraction failure becomes an unhandled async error.

playerController.removeListener(_addWaveformDataFromController);
_growingWaveController.dispose();

if (widget.animationDuration != Duration.zero) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This guard (and the late _growingWaveController) keys off the live widget.animationDuration, which can drift from how the controller was actually set up. If it starts non-zero (controller created) and the latest widget value is Duration.zero, dispose() skips _growingWaveController.dispose() and leaks it. And didUpdateWidget updates the cached animationDuration without ever creating the controller, so a Duration.zero → non-zero change leaves _growingWaveController uninitialized and the next access throws LateInitializationError. Could we track whether the controller was actually created (a bool, or make it nullable) and gate create/dispose on that instead of the live duration?

if (mounted) setState(() {});
}

// Mettre à jour les variables si nécessaire

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

^^

}
}

// Vérifier si d'autres propriétés importantes ont changé

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

^^

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please add the enum under lib/src/base/utils.dart

Comment on lines +147 to +158
if (waveformExtractionType != WaveformExtractionType.noExtraction) {
var extraction = waveformExtraction.extractWaveformData(
path: path,
noOfSamples: noOfSamples,
)
.then(
(value) {
)..then((values) {
waveformExtraction.waveformData
..clear()
..addAll(value);
notifyListeners();
},
);
..addAll(values);
});
if (waveformExtractionType == WaveformExtractionType.extractSync) {
await extraction;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Add the comments in the code file explaining the logic

@lavigarg-simform

Copy link
Copy Markdown

Hi @benco8186, thanks for the PR and for your patience on the review.

I went through the changes in detail. The direction is useful, but a few things need addressing before this can move forward:

  • Needs a rebase - The branch is currently conflicting with main. Could you rebase on the latest main and resolve the conflicts?

  • Breaking change to preparePlayer - Replacing shouldExtractWaveform with waveformExtractionType is a breaking API change, and the default also flips from extracting to noExtraction — so existing callers relying on the default will silently stop getting waveforms, and anyone passing shouldExtractWaveform: won't compile. If we go this route, it needs a CHANGELOG entry, a README/dartdoc update (the dartdoc still refers to shouldExtractWaveform), and a note in the PR description calling out the break.

  • A couple of correctness issues - I've left inline comments on them.

  • Open review comments - Some earlier comments are still not fixed. Could you look into them?

  • Scope - This bundles three separate enhancements plus an unrelated iOS extraction fix. It would be easier to review and land if the iOS fix (and ideally each concern) were split out. Heads-up too that it overlaps with a few open PRs on player_controller.dart, so whichever lands first, the others will need to reconcile.

Thanks again for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-response Waiting for someone to respond.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants