Upgrade Flutter and packages by rajveermalviya · Pull Request #1278 · zulip/zulip-flutter

rajveermalviya · 2025-01-13T20:49:24Z

Fixes: #942

PIG208

Thanks for the PR! I checked the changelogs from the upgrades. None of the breaking changes packages from the tools/upgrade pub-major upgrade affects us. The app also works fine on my Android device. It would be great to have it tested on someone's iOS device too.

Left some comments, but no action is required for moving this PR forward.

PIG208 · 2025-01-16T17:53:30Z

test/model/binding.dart

    bool criticalAlert = false,
    bool provisional = false,
    bool sound = true,
+    bool providesAppNotificationSettings = false,


Not directly related, but I noticed that the documentation https://firebase.flutter.dev/docs/messaging/permissions/#permission-settings we linked at NotificationService._requestPermission does not seem up-to-date with the actual requestPermission method.

PIG208 · 2025-01-16T18:47:33Z

test/model/binding.dart

+  //   so return the default value.
+  @override
+  // ignore: non_constant_identifier_names
+  final String pigeonVar_messageChannelSuffix = '';


I wonder if dartHostTestHandler helps, but it is also subject to removal (flutter/flutter#159886). Mocking HostApi this way seems fine to me. Maybe these pigeonVar prefixed variables should be private in the generated code.

gnprice

Thanks @rajveermalviya for taking care of this, and @PIG208 for the previous review and the manual testing!

Generally this all looks good — one request below to split up a commit. The other comments are all just me reading the changelogs, the results of which are all that there's nothing more we need to do. I can add that information to the commit messages before merge.

The app also works fine on my Android device. It would be great to have it tested on someone's iOS device too.

For this set of upgrades I'm content with that testing you did, and not doing additional manual testing on iOS.

The main area where I'd be concerned about platform-specific issues that call for manual testing on both platforms is notifications. In this upgrade, the changelogs for the notification-related (Firebase-related) upgrades look quite benign.

gnprice · 2025-01-16T19:46:51Z

lib/notifications/display.dart

    final soundsToAdd = NotificationSound.values.toList();

-    final List<StoredNotificationSound?> storedSounds;
+    final List<StoredNotificationSound> storedSounds;


deps: Upgrade pigeon to 22.7.2 Fixes: #942 And migrate to using non-nullable generic types.

Can the upgrade and the switch to non-nullable types be separated into two commits? I'd prefer to track which changes were required by the upgrade and which were our own choice.

gnprice · 2025-01-16T19:57:22Z

ios/Podfile.lock

-  - Firebase/CoreOnly (11.4.0):
-    - FirebaseCore (= 11.4.0)
-  - Firebase/Messaging (11.4.0):
+  - Firebase/CoreOnly (11.6.0):
+    - FirebaseCore (~> 11.6.0)
+  - Firebase/Messaging (11.6.0):


Here's the corresponding release notes:
https://firebase.google.com/support/release-notes/ios

from 11.4.0 up to 11.6.0. Most are in specific libraries we don't use, though, like Analytics or Vertex AI.

It looks like for the components we actually pull in, there's just a couple of build-time fixes in 11.4.1 and 11.4.2, then some error logging in FCM in 11.5.0. All quite innocuous, then.

gnprice · 2025-01-16T19:58:35Z

pubspec.lock

-    version: "3.9.0"
+    version: "3.10.0"


Looks like one change at https://pub.dev/packages/firebase_core/changelog : bumps the version of the Firebase iOS SDK.

Which is probably fine but opens the question of what changed there. (OK, found that changelog and posted another comment just now with the results.)

gnprice · 2025-01-16T19:59:26Z

pubspec.lock

-    version: "15.1.6"
+    version: "15.2.0"


Cool, changelog all looks innocuous.

gnprice · 2025-01-16T23:31:46Z

pubspec.yaml

  convert: ^3.1.1
  crypto: ^3.0.3
-  device_info_plus: ^10.0.1
+  device_info_plus: ^11.2.0


Looks like the one "breaking" change in this changelog is:

BREAKING FIX(device_info_plus): fixed webasm compliance (#3254). (e35e2123)

so I'll assume that's web-only and has no effect on us.

gnprice · 2025-01-16T23:33:02Z

pubspec.yaml

  image_picker: ^1.0.0
  json_annotation: ^4.9.0
-  mime: ^1.0.5
+  mime: ^2.0.0


The breaking change here is in extensionFromMime.

Looks like we don't use that method:

$ git grep extensionFromMime $

so doesn't affect us.

gnprice · 2025-01-16T23:40:14Z

pubspec.yaml

  fake_async: ^1.3.1
  flutter_checks: ^0.1.1
-  flutter_lints: ^4.0.0
+  flutter_lints: ^5.0.0


By nature if this doesn't cause CI to fail, it can't have broken anything.

There is one interesting change:

Removes the following lints (see https://github.com/dart-lang/lints/issues 205):

prefer_const_constructors

prefer_const_declarations

prefer_const_literals_to_create_immutables

So that ends the regular nagging from the analyzer to add const everywhere. As discussed in the thread, it's not clear that the performance benefit of it is/was material.

chrisbobbe · 2025-01-17T22:36:30Z

pubspec.yaml

-  sdk: '>=3.7.0-267.0.dev <4.0.0'
-  flutter: '>=3.28.0-2.0.pre.38575'  # 65ff060283e19423e9538c18c24e44495b70aeff
+  sdk: '>=3.7.0-312.0.dev <4.0.0'
+  flutter: '>=3.28.0-2.0.pre.38699'  # d14140f85439c517c98b0c40f8eaf942fcb46c74


Huh, I guess we still can't do #1204 then. 🤷‍♂️

rajveermalviya · 2025-01-19T20:56:59Z

Thanks for the review @gnprice. Pushed an update, PTAL.

gnprice

Thanks for the revision!

Looks good; one nit below. I'll fix that, add to those commit messages what I found above, and merge.

gnprice · 2025-01-22T06:11:45Z

android/app/src/main/kotlin/com/zulip/flutter/ZulipPlugin.kt

                    .setConversationTitle(messagingStyle.conversationTitle)
                    .setGroupConversation(messagingStyle.isGroupConversation)
-                messagingStyle.messages.forEach { it?.let {
+                messagingStyle.messages.forEach { it.let {


After this change, the it.let { isn't doing anything, so that layer of wrapping can be cut out.

I'll do that in a separate commit, since I'm revising this to add text to the commit messages anyway.

And update Flutter's supporting libraries to match.

Regenerate the output Dart and Kotlin files, and remove the statements which were made redundant after upstream added support for non-nullable types in collections, in the following commit: flutter/packages@218fd4a Changelog: https://pub.dev/packages/pigeon/changelog#2272

Now that we use the version of Pigeon package that supports non-nullable types in collections, update our Pigeon files to incorporate them. Fixes: zulip#942

When the argument here was nullable, "it?.let" served a useful purpose; but now that it's not, this can be cut out for a small simplification.

Upgrade carried using by following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ And adapted the test bindings to use the new `providesAppNotificationSettings` flag. Changelogs: https://pub.dev/packages/firebase_core/changelog#3100 https://pub.dev/packages/firebase_messaging/changelog#1520 The one notable change there is a bump in the version of the Firebase iOS SDK from 11.4.0 to 11.6.0 (seen also in the Podfile.lock changes). That changelog in turn is here: https://firebase.google.com/support/release-notes/ios though it mixes in all the different Firebase components into one changelog. For the components we actually pull in, the changes all look quite innocuous. [greg: added readings of changelogs]

…ter_lints This is the result of `tools/upgrade pub-major`. Changelogs: https://pub.dev/packages/device_info_plus/changelog#1120 https://pub.dev/packages/mime/changelog#200 https://pub.dev/packages/flutter_lints/changelog#500 In device_info_plus, the one "breaking" change is: BREAKING FIX(device_info_plus): fixed webasm compliance (#3254). (e35e2123) so I'll assume that's web-only and has no effect on us. In mime, the breaking change is in a method "extensionFromMime", which we don't use. And flutter_lints by nature can't have broken anything if it doesn't cause CI to fail. There is one interesting change in flutter_lints: it removes three lints that called for adding `const` modifiers. Apparently (as discussed in the dart-lang/lints thread linked from that changelog) it's not clear that the performance benefits of doing that everywhere was material. [greg: added readings of changelogs]

rajveermalviya force-pushed the flutter-upgrade branch from 5b54b63 to 829a166 Compare January 13, 2025 21:17

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Jan 13, 2025

PIG208 self-assigned this Jan 16, 2025

rajveermalviya force-pushed the flutter-upgrade branch from 829a166 to 107c6c7 Compare January 16, 2025 17:04

PIG208 reviewed Jan 16, 2025

View reviewed changes

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jan 16, 2025

PIG208 requested a review from gnprice January 16, 2025 18:50

PIG208 assigned gnprice and unassigned PIG208 Jan 16, 2025

gnprice reviewed Jan 16, 2025

View reviewed changes

chrisbobbe reviewed Jan 17, 2025

View reviewed changes

rajveermalviya force-pushed the flutter-upgrade branch from 107c6c7 to f7f5d12 Compare January 19, 2025 20:53

rajveermalviya requested a review from gnprice January 19, 2025 20:57

rajveermalviya force-pushed the flutter-upgrade branch from f7f5d12 to 18fdc64 Compare January 21, 2025 20:48

gnprice reviewed Jan 22, 2025

View reviewed changes

rajveermalviya and others added 7 commits January 21, 2025 22:12

deps: Upgrade Flutter to 3.28.0-2.0.pre.38699

084f94d

And update Flutter's supporting libraries to match.

notif: Use null-safe generics in Pigeon

16fe7ce

Now that we use the version of Pigeon package that supports non-nullable types in collections, update our Pigeon files to incorporate them. Fixes: zulip#942

notif [nfc]: Cut a redundant "it.let"

4c3657c

When the argument here was nullable, "it?.let" served a useful purpose; but now that it's not, this can be cut out for a small simplification.

deps: Upgrade packages within constraints (tools/upgrade pub)

f48dc5a

gnprice force-pushed the flutter-upgrade branch from 18fdc64 to cac0171 Compare January 22, 2025 06:30

gnprice merged commit cac0171 into zulip:main Jan 22, 2025

gnprice mentioned this pull request Feb 6, 2025

made List and Map items non-nullable #960

Closed

rajveermalviya deleted the flutter-upgrade branch February 25, 2025 11:54

Conversation

rajveermalviya commented Jan 13, 2025

Uh oh!

PIG208 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Jan 19, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PIG208 left a comment •

edited

Loading

chrisbobbe Jan 17, 2025 •

edited

Loading