Skip to content

Conversation

@TaeBbong
Copy link
Contributor

Summary

  • Small refactors to improve clarity and flexibility, addressing items previously marked with NOTES/FIXME.
  • This PR has no dependency with existing PR Rename app namespace in examples #27.

Changes

  1. TVGCanvas

    • Expose fit, filterQuality, alignment as constructor params (were hardcoded defaults).
  2. Thorvg play state

    • Replace multiple boolean flags with PlayState enum for readability and state safety.

Before → After

Play state

// before
// FIXME: should be an enumeration for each status
bool isPlaying = false;
bool deleted = false;
// after
enum PlayState { stopped, playing, deleted }
PlayState state = PlayState.stopped;

Update flow

// before
if (deleted) throw Exception('Thorvg is already deleted');
// ...
isPlaying = false;
// after
if (state == PlayState.deleted) throw Exception('Thorvg is already deleted');
// ...
state = PlayState.stopped;

@hermet hermet requested a review from Copilot August 25, 2025 02:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the ThorVG library to improve code clarity and flexibility by replacing boolean state flags with an enum and exposing previously hardcoded canvas parameters.

  • Introduced PlayState enum to replace multiple boolean flags for better state management
  • Exposed fit, filterQuality, and alignment as constructor parameters in TVGCanvas
  • Updated all state checks to use the new enum values instead of boolean flags

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/src/thorvg.dart Replaced boolean state flags with PlayState enum and updated all state validation logic
lib/src/lottie.dart Added constructor parameters for canvas rendering options and updated repaint logic

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@tinyjin tinyjin self-requested a review August 25, 2025 06:24
@tinyjin tinyjin added the enhancement Improve features label Aug 25, 2025
Copy link
Member

@tinyjin tinyjin left a comment

Choose a reason for hiding this comment

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

Thank you. Please check comments

final ThorVGFlutterBindings tvg = ThorVGFlutterBindings(_dylib);

/* ThorVG Play State */
enum PlayState { stopped, playing, deleted }
Copy link
Member

Choose a reason for hiding this comment

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

enum PlayState { stopped, playing, destroyed }

Purpose is to align with existing web player states
Image

Copy link
Member

Choose a reason for hiding this comment

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

Also wondering if we need paused state

Copy link
Member

@hermet hermet Sep 4, 2025

Choose a reason for hiding this comment

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

Pause/Resume functions are quite general, essential functions as a player.

If we select either Paused vs Frozen -> Paused win.

Eventually, we might need these enough status definition.

    Uninitialized             //any file is not loaded yet. 
    Loading (optional)    //this needs only player supports async loading
    Ready
    Playing
    Paused
    Frozen      
    Stopped
    Error
    Destroyed       //Hmm... Is it possible to query this?

Copy link
Member

@hermet hermet Sep 4, 2025

Choose a reason for hiding this comment

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

  • The player needs to support user event listening for activation and deactivation events (such as for Frozen).

@TaeBbong TaeBbong force-pushed the taebbong/refactor branch 2 times, most recently from f53f9b7 to d1a85e8 Compare September 4, 2025 13:37
@tinyjin tinyjin self-requested a review September 4, 2025 14:27
@tinyjin
Copy link
Member

tinyjin commented Oct 19, 2025

@TaeBbong
Could you please rebase to fix conflict?
CleanShot 2025-10-19 at 17 16 01@2x

@TaeBbong
Copy link
Contributor Author

TaeBbong commented Oct 19, 2025

@TaeBbong Could you please rebase to fix conflict?

@tinyjin It's done now!

- simply aligned with existing web player
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improve features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants