Skip to content

Comments

feat(controllers): enhance documentation, add lifecycle callbacks, an…#3361

Merged
jonataslaw merged 2 commits intojonataslaw:masterfrom
Aniketkhote:master
Jun 16, 2025
Merged

feat(controllers): enhance documentation, add lifecycle callbacks, an…#3361
jonataslaw merged 2 commits intojonataslaw:masterfrom
Aniketkhote:master

Conversation

@Aniketkhote
Copy link
Contributor

  • Improve documentation for all controller classes with usage examples
  • Add error handling in FullLifeCycleMixin callbacks
  • Implement onMemoryPressure and onAccessibilityChanged callbacks
  • Add proper ScrollController disposal in ScrollMixin
  • Enhance type safety and error reporting
  • Update documentation with clear method descriptions and examples
  • Fix potential memory leaks in lifecycle management

Copy link
Owner

@jonataslaw jonataslaw 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 very much for this PR. The documentation is generally well written, but I have some concerns regarding the try/catch blocks.

The first one is meant to handle a case where update() is called before the controller has been initialized. However, this is impossible, since the constructor itself initializes the controller. In other words, by the time we call controller.update(), the controller is already initialized. This makes the try/catch both unnecessary and a source of extra overhead.

Second, the lifecycle methods are meant to be overridden by the user. If they override without calling super, the try/catch will never execute; if they override and call super, they’ll simply run an empty block (or Flutter’s default code). In my opinion, this try/catch is also unnecessary and can actually confuse debugging: if an error is thrown, the stack trace will point to the library code rather than the function that actually caused the error.

} else {
for (final id in ids) {
refreshGroup(id);
try {
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, could you explain in which situation the update block could face an exception to make this try/catch necessary?

@Aniketkhote
Copy link
Contributor Author

I've addressed the feedback with the following changes:

Removed try-catch blocks from lifecycle methods to align with Flutter's idiomatic error handling, allowing better debugging and letting global handlers catch issues.

Simplified the update() method by removing redundant error handling—it's safe as the controller is already initialized.

Kept documentation improvements for better developer clarity.

These changes follow Flutter best practices, giving developers more control while improving maintainability.

@jonataslaw
Copy link
Owner

Thank you!
LGTM!

@jonataslaw jonataslaw merged commit daa3b93 into jonataslaw:master Jun 16, 2025
1 check passed
@fisforfaheem
Copy link

any new updates??

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.

3 participants