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.

Sign up for GitHub

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

Jump to bottom

Flutter error management #28

Merged
merged 30 commits into from
Mar 18, 2022
Merged

Flutter error management #28

merged 30 commits into from
Mar 18, 2022

Conversation

evdokimovs
Copy link
Contributor

@evdokimovs evdokimovs commented Feb 22, 2022 •
edited by tyranron
Loading

Synopsis

Этот PR реализует механизм уведомления конечного приложения о панике, произошедшей в Расте, а также детектирование ошибок, возвращенных из Флаттера Расту.

Checklist

  • Created PR:
    • In draft mode
    • Name contains Draft: prefix
    • Name contains issue reference
    • Has k:: labels applied
    • Has assignee
  • 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
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • Draft: prefix is removed
    • All temporary labels are removed
    • flutter-webrtc dependency switched back to master branch

@evdokimovs evdokimovs added feature New feature or request k::api Related to API (application interface) labels Feb 22, 2022
@evdokimovs evdokimovs self-assigned this Feb 22, 2022
@evdokimovs
Copy link
Contributor Author

FCM

Flutter error management (#28)

- add `on_panic` callback to the Dart side
- detect returned from Flutter errors in Rust

@evdokimovs evdokimovs marked this pull request as ready for review February 22, 2022 10:55
alexlapa
alexlapa requested changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@alexlapa alexlapa left a comment

Choose a reason for hiding this comment

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

@evdokimovs ,

Sync with mainline.

@evdokimovs
Merge branch 'master' into flutter-error-management
1ce0665 
# Conflicts:
#	flutter/android/src/main/jniLibs/arm64-v8a/libmedea_jason.so
#	flutter/android/src/main/jniLibs/armeabi-v7a/libmedea_jason.so
#	flutter/android/src/main/jniLibs/x86/libmedea_jason.so
#	flutter/android/src/main/jniLibs/x86_64/libmedea_jason.so
#	flutter/example/lib/call.dart
#	flutter/example/pubspec.lock
#	flutter/lib/src/native/platform/constraints.dart
#	flutter/lib/src/native/platform/ice_candidate.dart
#	flutter/lib/src/native/platform/ice_servers.dart
#	flutter/lib/src/native/platform/input_device_info.dart
#	flutter/lib/src/native/platform/media_track.dart
#	flutter/lib/src/native/platform/peer_connection.dart
#	flutter/lib/src/native/platform/transceiver.dart
#	flutter/pubspec.yaml
#	src/platform/dart/constraints.rs
#	src/platform/dart/media_track.rs
@evdokimovs
Update .so
d84306b
alexlapa
alexlapa requested changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@alexlapa alexlapa left a comment

Choose a reason for hiding this comment

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

@evdokimovs , discussed, added todos

@evdokimovs
Use catch_unwind instead of panic hook
8791f7a
@evdokimovs
Get rid of RustHandles ordering
14fd1f2
@evdokimovs
Refactor, fixes
f044e9b
@evdokimovs
Merge branch 'master' into flutter-error-management
7ec62e0 
# Conflicts:
#	flutter/android/src/main/jniLibs/arm64-v8a/libmedea_jason.so
#	flutter/android/src/main/jniLibs/armeabi-v7a/libmedea_jason.so
#	flutter/android/src/main/jniLibs/x86/libmedea_jason.so
#	flutter/android/src/main/jniLibs/x86_64/libmedea_jason.so
#	flutter/example/pubspec.lock
#	flutter/lib/src/interface/media_device_info.dart
#	flutter/lib/src/interface/media_manager.dart
#	flutter/lib/src/native/ffi/exception.dart
#	flutter/lib/src/native/ffi/exception.g.dart
#	flutter/lib/src/native/media_device_info.dart
#	src/api/dart/media_device_info.rs
#	src/api/dart/mod.rs
#	src/api/dart/utils/err.rs
@evdokimovs
Fix merge
84f3586
@evdokimovs
Fix flutter analyze
9f6d9d5
alexlapa and others added 7 commits March 16, 2022 13:48
@alexlapa
Merge branch 'master' into flutter-error-management
4180d6c 
# Conflicts:
#	flutter/android/src/main/jniLibs/arm64-v8a/libmedea_jason.so
#	flutter/android/src/main/jniLibs/armeabi-v7a/libmedea_jason.so
#	flutter/android/src/main/jniLibs/x86/libmedea_jason.so
#	flutter/android/src/main/jniLibs/x86_64/libmedea_jason.so
#	flutter/lib/src/interface/media_manager.dart
@evdokimovs
Minor fix
562f7f3
@alexlapa
corrections [skip ci]
ee0dc08
@evdokimovs
Add Flutter test for panic_catcher
7c9061f
@alexlapa
corrections [skip ci]
2525602
@evdokimovs
Remove Handles from storage of free
76f8ecb
@alexlapa
corrections
f15bd44
@alexlapa alexlapa requested a review from tyranron March 17, 2022 07:56
@tyranron tyranron added k::refactor Refactoring, technical debt elimination and other improvements of existing code base enhancement Improvement of existing features or bugfix labels Mar 17, 2022
@tyranron tyranron changed the title Draft: Flutter error management Flutter error management Mar 17, 2022
tyranron
tyranron approved these changes Mar 17, 2022
View reviewed changes
src/platform/dart/mod.rs Outdated
log::error!("Rust code panicked {bt:?}");
panic::set_hook(Box::new(|bt| {
if let Some(f) = unsafe { PANIC_FN.as_ref() } {
f.call1(format!("{bt:?}"));
Copy link
Member

Choose a reason for hiding this comment

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

Why fmt::Debug? Seems fmt::Display suits better here.

src/platform/dart/utils/handle.rs Outdated
let err_msg = unsafe {
c_str_into_string(Dart_GetError_DL_Trampolined(handle))
};
panic!("Unexpected Dart error: {}", err_msg)
Copy link
Member

Choose a reason for hiding this comment

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

This should have been documented in # Panic section of the docs.

@tyranron tyranron marked this pull request as draft March 17, 2022 14:27
@tyranron
Copy link
Member

Don't merge.

@evdokimovs
Copy link
Contributor Author

@tyranron ,

Мне запушивать запрошенные изменения, или были обнаружены еще какие-то неучтенные моменты?

@tyranron tyranron marked this pull request as ready for review March 17, 2022 15:35
@tyranron
Copy link
Member

@evdokimovs there was GitHub outage, preventing me from pushing changes. Since the PR is aproved, you don't need to push anything.

@tyranron
Copy link
Member

But GitHub Actions still seem to be outaged. We need to wait.

@tyranron
Copy link
Member

@evdokimovs Flutter tests stuck all the time. Please, fix them in a separate PR.

@tyranron tyranron merged commit 76a077c into master Mar 18, 2022
@tyranron tyranron deleted the flutter-error-management branch March 18, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexlapa alexlapa alexlapa approved these changes

@tyranron tyranron tyranron approved these changes

Assignees

@evdokimovs evdokimovs

Labels
enhancement Improvement of existing features or bugfix feature New feature or request k::api Related to API (application interface) k::refactor Refactoring, technical debt elimination and other improvements of existing code base
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evdokimovs @tyranron @alexlapa