Desktop cleanups #1143
Desktop cleanups #1143
Conversation
|
@jigar-f I discovered there were a few different issues that were causing the desktop app to crash on macOS: certain settings not being initialized, missing error handling, etc). The most prominent issue is when you navigate to a new screen: after calling I think I've gotten it to a point where the app doesn't crash at all for me, but it would be great if you could take this for a spin to see how it works for you |
atavism
force-pushed
the
atavism/desktop-cleanups
branch
2 times, most recently
from
f7640ce to
49fb3c5
Compare
|
@atavism thanks for making those changes, I will do testing today with this branch. |
|
@atavism I am trying to run this branch I am getting this error at most places, I think this coz we are using class method inside compute. |
|
@jigar-f Thanks! I think that is happening because of the following: Making some changes to fix this |
|
@jigar-f So another reason the app has been crashing is totally unrelated to lantern-client 🤦 The old desktop app crashed for me using the latest flashlight.. Pretty sure we've been seeing the same error: |
|
Ahh, Yeah, that might be right. |
|
I am still getting issues, with plans, report issues, and others. Also, I added a few more changes. |
atavism
force-pushed
the
atavism/desktop-cleanups
branch
from
1ebb46e to
b20b808
Compare
lib/app.dart
Outdated
| @@ -122,9 +122,7 @@ class _LanternAppState extends State<LanternApp> | |||
| ChangeNotifierProvider(create: (context) => VPNChangeNotifier()), | |||
| ChangeNotifierProvider(create: (context) => InternetStatusProvider()) | |||
| ], | |||
| child: ChangeNotifierProvider( | |||
There was a problem hiding this comment.
@jigar-f I think this may be another reason why the app has been crashing.. it looks like we are creating multiple ChangeNotifierProviders in the App class for the bottom bar notifier? There's an instance already passed to MultiProvider.
There was a problem hiding this comment.
I might have forgotten to remove that, but I don't think that should crash app.
There was a problem hiding this comment.
Ok cool, I assume we only need to use a single ChangeNotifierProvider though
|
@atavism I ended up changing the LanternFFi class, To fix the issue and isolate the issue, When we used to compute we had to use the static method. |
Sweet, thanks for making those changes! Working well for me. |
|
@jigar-f Another potential issue I found is in initCallbacks in the VPN notifier: (bool, bool, bool) startUpInitCallBacks() {
return LanternFFI.startUpInitCallBacks();
}
void initCallbacks() {
if (timer != null) {
return;
}
timer = Timer.periodic(const Duration(seconds: 1), (_) {
final result = startUpInitCallBacks();
if (!result.$1 || !result.$2) {
flashlightState = 'fetching_configuration'.i18n;
}
if (result.$1 && result.$2 && !result.$3) {
flashlightState = 'establish_connection_to_server'.i18n;
}
notifyListeners();
if (result.$1 && result.$2 && result.$3) {
// everything is initialized
isFlashlightInitialized = true;
isFlashlightInitializedFailed = false;
print("flashlight initialized");
notifyListeners();
timer?.cancel();
} else if (timer!.tick >= 6) {
// Timer has reached 6 seconds
// Stop the timer and set isFlashlightInitialized to true
print("flashlight fail initialized");
isFlashlightInitialized = true;
isFlashlightInitializedFailed = true;
notifyListeners();
}
});
}It looks like there's occasions where we invoke the Timer.periodic callback, which calls I think the callback function that we provide to Timer.periodic runs asynchronously, and that's causing issues since we expect to call the FFI functions on the main thread and would need to use isolates to manage them otherwise. I disabled the initCallbacks call for now |
|
The @atavism issue you are facing, Is nothing to do with, This issue is same issue, I am facing due to some signal handler. |
I meant initCallbacks in the VPN notifier. I thought it may be related because I'm able to get the app to crash repeatedly with the following test code: class TestWidget extends StatelessWidget {
const TestWidget({Key? key}) : super(key: key);
@override
Widget build(BuildContext context) {
final vpnNotifier = context.watch<VPNChangeNotifier>();
final res = vpnNotifier.isFlashlightInitialized;
return SizedBox();
}
}
Future<void> main() async {
WidgetsFlutterBinding.ensureInitialized();
await LanternFFI.startDesktopService();
await Localization.ensureInitialized();
runApp(MultiProvider(
providers: [
ChangeNotifierProvider(create: (context) => VPNChangeNotifier())
],
child: TestWidget()));
}That's with |
|
Thats a great find, I will take a look at this sprint. |
|
I am going to pull in these changes and if we need to fix additional issues, we can do that on the websocket PR |
For https://github.com/getlantern/engineering/issues/1504