Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions flutter_local_notifications_windows/lib/src/ffi/mock.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Just a mock, doesn't need real types or safety.
// ignore_for_file: type_annotate_public_apis, always_specify_types

import 'dart:ffi';

import 'package:ffi/ffi.dart';

import 'bindings.dart';

/// Mocked FFI bindings.
class MockBindings implements NotificationsPluginBindings {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you place this in the test folder? It's purely test related and is the approach taken when creating mocks via mocktail or mockito or hand crafted ones. Note this is also currently a stub not a mock. A mock provides the ability to record the interactions that can then be used to verify what took place. Can you also rename the class and adjust the docs accordingly to reflect this?

@override
void cancelAll(_) {}

@override
void cancelNotification(_, int id) {}

@override
Pointer<NativePlugin> createPlugin() => malloc<Int>().cast();

@override
void disposePlugin(_) {}

@override
void freeLaunchDetails(_) {}

@override
void freeDetailsArray(ptr) => malloc.free(ptr);

@override
Pointer<NativeNotificationDetails> getActiveNotifications(_, __) =>
malloc<NativeNotificationDetails>();

@override
Pointer<NativeNotificationDetails> getPendingNotifications(_, __) =>
malloc<NativeNotificationDetails>();

@override
bool hasPackageIdentity() => false;

@override
bool init(_, __, ___, ____, _____, ______) => true;

@override
bool isValidXml(ptr) => true;

@override
bool scheduleNotification(a, b, c, d) => true;

@override
bool showNotification(a, b, c, d) => true;

@override
NativeUpdateResult updateNotification(a, b, c) => NativeUpdateResult.success;
}
31 changes: 25 additions & 6 deletions flutter_local_notifications_windows/lib/src/plugin/ffi.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import 'dart:ffi';

import 'package:ffi/ffi.dart';
import 'package:meta/meta.dart';
import 'package:xml/xml.dart';

import '../details.dart';
import '../details/notification_to_xml.dart';
Expand All @@ -22,10 +24,28 @@ extension on String {
this[23] == '-';
}

/// Does a basic syntax check on XML.
bool _checkXml(String xml) {
Copy link
Owner

Choose a reason for hiding this comment

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

It's file private but look this could have been private within the FlutterLocalNotificationsWindows class. Would you be able to adjust this?

Note: on a related note and not that it has to be done in this PR but I believe I missed picking this up when it comes to _globalLaunchCallback as well

try {
XmlDocument.parse(xml);
return true;
} on XmlFormatException {
return false;
}
}

/// The Windows implementation of `package:flutter_local_notifications`.
class FlutterLocalNotificationsWindows extends WindowsNotificationsBase {
/// Creates an instance of the native plugin.
FlutterLocalNotificationsWindows();
FlutterLocalNotificationsWindows()
: _bindings = NotificationsPluginBindings(
DynamicLibrary.open('flutter_local_notifications_windows.dll'));

/// Creates an instance of this plugin with the given (mocked) bindings.
@visibleForTesting
FlutterLocalNotificationsWindows.withBindings(
Copy link
Owner

Choose a reason for hiding this comment

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

I was about to mention perhaps to rename it to FlutterLocalNotificationsWindows.private as it would be consistent with what you had on Linux. There was also an element of how the with<...> convention can lead to issues (repetition from readability perspective and could break if more arguments got introduced).

Rather than taking this approach, I noticed first-party plugins are taking an optional, nullable argument with the @visibleForTesting annotation applied to it. I think this is actually a good approach as implementation and test code would point to the same constructor. Would you be able to switch it so that the only constructor follows that pattern? An example of what I mean can be found here

this._bindings,
);

/// Registers the Windows implementation with Flutter.
static void registerWith() {
Expand All @@ -37,11 +57,7 @@ class FlutterLocalNotificationsWindows extends WindowsNotificationsBase {
static FlutterLocalNotificationsWindows? instance;

/// The FFI generated bindings to the native code.
late final NotificationsPluginBindings _bindings =
NotificationsPluginBindings(_library);

final DynamicLibrary _library =
DynamicLibrary.open('flutter_local_notifications_windows.dll');
final NotificationsPluginBindings _bindings;

/// A pointer to the C++ handler class.
late final Pointer<NativePlugin> _plugin;
Expand Down Expand Up @@ -281,6 +297,9 @@ class FlutterLocalNotificationsWindows extends WindowsNotificationsBase {

@override
bool isValidXml(String xml) => using((Arena arena) {
if (!_checkXml(xml)) {
return false;
}
final Pointer<Utf8> nativeXml = xml.toNativeUtf8(allocator: arena);
return _bindings.isValidXml(nativeXml);
});
Expand Down
14 changes: 14 additions & 0 deletions flutter_local_notifications_windows/lib/src/plugin/stub.dart
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
import 'package:meta/meta.dart';

import '../details.dart';
import '../ffi/bindings.dart';
import 'base.dart';

/// The Windows implementation of `package:flutter_local_notifications`.
class FlutterLocalNotificationsWindows extends WindowsNotificationsBase {
Copy link
Owner

Choose a reason for hiding this comment

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

With the comment I left that allows the FFI implementation to take an argument for the bindings used behind the scenes, does this perhaps render the stub redundant now? Based on my own knowledge and testing, it would be redundant but let me know if I've missed anything

/// Creates an instance of this plugin.
FlutterLocalNotificationsWindows();

/// Creates an instance of this plugin with the given (mocked) bindings.
@visibleForTesting
FlutterLocalNotificationsWindows.withBindings(
// Needed in this file due to conditional imports
// ignore: avoid_unused_constructor_parameters
NotificationsPluginBindings bindings,
);

@override
Future<bool> initialize(
WindowsInitializationSettings settings, {
Expand Down
58 changes: 0 additions & 58 deletions flutter_local_notifications_windows/test/bindings_test.dart

This file was deleted.

Loading
Loading