Skip to content

feat(#51): Implement Messages feature #55

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.

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

nivisi
Copy link
Contributor

@nivisi nivisi commented Dec 9, 2022

Resolves #51

  • Built using streams, so if in the future we’ll decide to pull messages periodically / to use sockets to listen to updates, the app will react.
  • Allows you to listen to Messages using NStack.messages.onMessage()
  • On the UI layer, you can use the NStackMessageListener widget
  • For displaying a simple dialog, you can use the NStackMessageDialog(message: message) widget. On Android it shows a material dialog, on iOS — a cupertino dialog
  • Updated & provided docs

/// );
/// }
/// ```
class NStackMessageListener extends StatefulWidget {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do factory methods on the main NStack class?

Kind of like NStack.Message()

We could potentially extend that to have NStack.MessageCustom() etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johsoe why?

Copy link
Contributor

Choose a reason for hiding this comment

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

No other reason than i like that pattern more than exposing multiple NStack* classes upfront which can be a bit messy to figure out

Copy link
Collaborator

Choose a reason for hiding this comment

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

@johsoe I guess you mean a named constructor, cause a factory on a class T can only return an instance of T, so in this case if we use a factory on NStack we can't return a widget. Named constructors also can't return widgets:

class NStack {
  Widget NStack.Message() {
    return Container();
  }
}

You will get the following errors:

  • Constructors can't have a return type. Try removing the return type.dart(constructor_with_return_type)
  • Constructors can't return values. Try removing the return statement or using a factory

To return a widget you can use a static method:

class NStack {
  static Widget message({Widget? child}) {
    return child ?? Container();
  }
}

// And then call `NStack.message(child: HomeScreen())` 

However, one interesting implementation I came up with is using a mixin:

mixin NStackMessageListener<T extends StatefulWidget> on State<T> {
  StreamSubscription? _subscription;

  void onMessage(Message message) {}

  @override
  void didChangeDependencies() {
    if (_subscription == null) {
      final scope = NStackScope.of(context);
      _subscription = scope.messages.onMessage.listen(onMessage);
    }
    super.didChangeDependencies();
  }

  @override
  void dispose() {
    _subscription?.cancel();
    super.dispose();
  }
}

And in your widget:

class MainScreen extends StatefulWidget {
  const MainScreen({super.key});

  @override
  State<MainScreen> createState() => _MainScreenState();
}

class _MainScreenState extends State<MainScreen> with NStackMessageListener {
  @override
  void onMessage(Message message) {
    NStackMessageDialog.show(context, message: message);
    super.onMessage(message);
  }

  @override
  Widget build(BuildContext context) {
    // ...
  }
}

Comment on lines +12 to +14
// There's no dispose / close method
// ignore: close_sinks
final _onMessage = StreamController<Message>.broadcast();
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this stream controller will remain open? I also noticed another StreamController not being closed in NStackLocalization called _onLocaleChanged

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the stream controller is a bit overkill here, I guess we can leave it as a normal future request that we call from the main widget and just call onMessage.

In the future if a web-socket connection is required we can refactor and make it a stream in a way that it can be disposed.

Comment on lines +12 to +14
// There's no dispose / close method
// ignore: close_sinks
final _onMessage = StreamController<Message>.broadcast();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the stream controller is a bit overkill here, I guess we can leave it as a normal future request that we call from the main widget and just call onMessage.

In the future if a web-socket connection is required we can refactor and make it a stream in a way that it can be disposed.

@@ -142,6 +158,8 @@ class _Test extends SectionKeyDelegate {
* NStack Flutter Widgets
*
*/

/// Allows to access NStack features via a `BuildContext`.
class NStackScope extends InheritedWidget {
final NStackState state;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not being used I think it's safe to remove, plus storing a widget's state is not a best practice and might produce unpredictable results.

Comment on lines +33 to +35
onMessage: (message) {
NStackMessageDialog.show(context, message: message);
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the onMessage can be added directly to NStackWidget, hence no need for an NStackMessageListener.

class ExampleApp extends StatelessWidget {
  const ExampleApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      builder: (context, child) {
        return NStackWidget(
          child: child!,
          onMessage: (message) {
           // ...
          }
        );
      },
      home: const MainScreen(),
    );
  }
}

@@ -42,20 +51,27 @@ export 'package:nstack/models/app_open_platform.dart';
*/

final NStack = NStackSdk<Localization>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it have to be a global variable?

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.

Implement Messages feature
3 participants