engine icon indicating copy to clipboard operation
engine copied to clipboard

Win32: Add lifecycle channel support

Open moko256 opened this issue 4 years ago • 12 comments

This PR adds flutter/lifecycle channel support for win32 ~~and winuwp~~(removed), and enable apps to receive window focused/minimized event. This PR adds StringMessageCodec to shell/platform/common.

This PR will support part of flutter/flutter#103637

No changes in flutter/tests.

Pre-launch Checklist

  • [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • [X] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • [X] I listed at least one issue that this PR fixes in the description above.
  • [X] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
  • [ ] I updated/added relevant documentation (doc comments with ///).
  • [X] I signed the [CLA].
  • [x] All existing and new tests are passing.

Example (main.dart, modified from WidgetsBindingObserver example in docs): This app displays last event and print it to stdout.

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const AppLifecycleReactor(),
    );
  }
}

class AppLifecycleReactor extends StatefulWidget {
  const AppLifecycleReactor({Key? key}) : super(key: key);

  @override
  State<AppLifecycleReactor> createState() => _AppLifecycleReactorState();
}

class _AppLifecycleReactorState extends State<AppLifecycleReactor>
    with WidgetsBindingObserver {
  @override
  void initState() {
    super.initState();
    WidgetsBinding.instance!.addObserver(this);
  }

  @override
  void dispose() {
    WidgetsBinding.instance!.removeObserver(this);
    super.dispose();
  }

  AppLifecycleState? _notification;

  @override
  void didChangeAppLifecycleState(AppLifecycleState state) {
    print('Last notification: $state\n');
    setState(() {
      _notification = state;
    });
  }

  @override
  Widget build(BuildContext context) {
    return Text('Last notification: $_notification');
  }
}

moko256 avatar Oct 21 '21 20:10 moko256

I resolved the conflicts and this is ready for review.

moko256 avatar Feb 18 '22 00:02 moko256

We need this desperately. Do we have a commit for MacOS as well?

ollyde avatar Mar 13 '22 19:03 ollyde

@ollydixon Sorry, this PR is for Windows only (and I don't have Mac now), but StringMessageCodec in this PR should help its implementation for MacOS and other platforms too.

moko256 avatar Mar 13 '22 20:03 moko256

@cbracken I ping softly (sorry if this was already triaged and is just low priority)

moko256 avatar Mar 13 '22 21:03 moko256

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

flutter-dashboard[bot] avatar Apr 25 '22 03:04 flutter-dashboard[bot]

Gold has detected about 27 new digest(s) on patchset 5. View them at https://flutter-engine-gold.skia.org/cl/github/29288

skia-gold avatar May 12 '22 17:05 skia-gold

Thanks for reviewing! but I can't take time just now so I mark this as draft.

moko256 avatar Jul 07 '22 12:07 moko256

Sounds good. Feel free to tag me directly when you're ready for another review!

loic-sharma avatar Jul 07 '22 17:07 loic-sharma

Cross-referencing https://github.com/flutter/flutter/issues/65061 since it's related.

cbracken avatar Jul 19 '22 23:07 cbracken

``

Eldeloro1 avatar Aug 21 '22 23:08 Eldeloro1

@cbracken @loic-sharma This is ready for review now. Please review this at your convenience.

moko256 avatar Sep 05 '22 16:09 moko256

Would it be possible to add an integration test to the engine using flutter_windows_unittests.cc and its corresponding main.dart fixture? In theory you should be able to post Windows messages and check you receive the expected lifecycle message in Dart.

loic-sharma avatar Sep 08 '22 20:09 loic-sharma

This is a useful PR, and I'm aware that we very much need lifecycle event support, but I'm concerned that if we do this without a coherent strategy for all of the platforms that we'll end up with a fractured API for it that will require special handling for each platform on the framework side. Can we hold off on this just a little longer until we have a unified design?

gspencergoog avatar Nov 14 '22 20:11 gspencergoog

@gspencergoog I agree with that, but I don't have much time to fix this now. I'm sorry, but I'll close this for now.

moko256 avatar Nov 15 '22 02:11 moko256

No worries! Your work will help inform the unified design, thank you for taking a first stab at this problem

loic-sharma avatar Nov 15 '22 21:11 loic-sharma