Fix/129 fix eliminate parkings flickering #152

mikolaj-jalocha · 2024-08-10T07:18:23Z

I've tested it on Android device (the bug was there before as well). Everything looks fine. Hopefully new state is being updated.

…date

simon-the-shark

I'll try to do this too when I have a moment. I have few ideas but we'll see how they'll do

simon-the-shark · 2024-08-10T08:22:10Z

lib/features/parkings_view/repository/parkings_repository.dart

@@ -13,7 +13,7 @@ extension RefIntervalRefreshX on Ref {
  void setRefresh(Duration interval) {
    final timer = Timer.periodic(
      interval,
-      (t) => invalidateSelf(),
+      (t) => invalidate(parkingsRepositoryProvider),


Unfortunetly this throws :(

[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: 'package:riverpod/src/framework/element.dart': Failed assertion: line 614 pos 11: 'listenable._origin != origin': A provider cannot depend on itself

basically invalidateSelf() and invalidate(someProvider) works the same with the difference the second one seems to throw when use inside the same provider (I didn't knew that actually, but make sense)

simon-the-shark · 2024-08-10T14:26:51Z

@mikolaj-jalocha I've been playing around with it for some time 😭 But I have a fix. Suprisingly it wasn't so much about the riverpod but more with the UI itself and the switch pattern matching logic. Tell me what think. Let's hope everything works as expected now

Nagranie.z.ekranu.2024-08-10.o.16.14.49.mov

simon-the-shark · 2024-08-10T18:47:22Z

@mikolaj-jalocha when you have a moment you can make a review here, cause i cannot request review from its author

mikolaj-jalocha · 2024-08-10T21:08:16Z

@simon-the-shark thanks for feedback. I didn't see exception before (I should look on logs..).

I will do review ASAP (tomorrow)

mikolaj-jalocha

The flickering issue seemed simple to fix but ended up requiring quite a refactor. Good job! I wouldn’t have come up with a solution so quickly—my Flutter experience is too limited and my state management skills aren’t strong enough for that. 😂

Below are a few suggestions and some questions I have about how certain parts of the code work, so I can better understand the issue.

I really like the introduction of IList. It's an order of magnitude faster than Flutter's built-in version, and I think it could have a noticeable impact on our app’s performance.
By the way, every time I review your code, I end up reading documentation on something new—which is cool!

mikolaj-jalocha · 2024-08-12T08:12:57Z

lib/features/parkings_view/api_client/iparking_client.dart

@@ -6,7 +6,7 @@ import "../../../config/api_base_config.dart";
 part "iparking_client.g.dart";

 abstract class ParkingsConfig {
-  static const parkingsRefreshInterval = Duration(minutes: 1);
+  static const parkingsRefreshInterval = Duration(seconds: 30);


Is the period here ok?

Yeah I did this on purpose. It's not our server anyway and previously if we entered app in unfortunate moment we could have almost 1 minut delay from the server. Now it's tops 30 sec. In the future we can think of some mechanism that will "align" our refreshes with those on server

mikolaj-jalocha · 2024-08-12T08:15:31Z

lib/features/map_view/widgets/map_widget.dart

+                .watch(context.mapSourceRepository<T>())
+                .value
+                .whereNonNull
+                .toList();


I wonder if we can use here immutable list? (IList)

btw the way Consumer here's introduced improved visibility a lot imo.

Btw, is this a change that reduced flickering? Moving state refreshment inside AsyncValue?

Yeah we can use IList here

I moved the state listening down the tree in few places, meaning less widgets are refreshed needlesly. But it still doesn't help :P with the flickering

mikolaj-jalocha · 2024-08-12T08:18:16Z

lib/features/map_view/widgets/bottom_scroll_sheet/data_list.dart

      AsyncError(:final error) =>
        SliverToBoxAdapter(child: MyErrorWidget(error)),
-      AsyncValue(:final value) =>
-        _DataSliverList<T>(value.whereNonNull.toList())
+      _ => const DataListLoading(),


What happened to AsyncLoading?

That's the change that helped. This alone solves the problem. I got the idea from docs as well, from the pull to refresh guide. https://riverpod.dev/docs/case_studies/pull_to_refresh

What was the problem is the fact that after refresh the state changes to AsyncLoading and we showed the loading screen for a second.

It turns out through, even though the state is loading. State.value still holds previous value for us to use.

So I changed the pattern matching switch logic a bit.

Now if the async value have any value we show it. If it has error we show error and in all other cases (_ means all unmatched previously things) we show loading screen. So the loading screen shows if we have async loading or async data with value=null

mikolaj-jalocha · 2024-08-12T08:18:56Z

lib/features/map_view/widgets/bottom_scroll_sheet/data_list.dart

+      // misz-masz here fixed flickering
+      // we probably should consider similar refactoring to all switches, but won't be visible in our other use-cases
+      AsyncValue(:final IList<T?> value) =>
+        _DataSliverList<T>(value.whereNonNull.toIList()),


Does the introduction of immutable list reduced flickering?

It was one of many things I've tried cause IList implements list comparability, meaning ILists are equal if have equal elements in same order. Hovewer I forgot the riverpod doesn't check equality == operator anyway, but uses identical comparator. More you can read here in one of the questions https://riverpod.dev/docs/essentials/faq

It allowed me to reduce flickering frequency with manual check, but didn't eliminate it fully:

if (state != newstate) { state = newstate; // or ref.invalidate or smth, basically reduces flickering to only situation when the number of places changed }

So its not the solution but I left the IList anyway cause it's recommended to have immutable and combarable state objects

mikolaj-jalocha · 2024-08-12T08:21:25Z

lib/features/map_view/controllers/map_data_controller.dart

    final itemSelected = ref.watch(mapControllers.activeMarker);
    if (itemSelected != null) {
-      return [itemSelected]; // shows only selected building
+      return [itemSelected].lock; // shows only selected building


Tell me if i properly understand that; if the item is selected, the list cannot be modified?

I believe .lock is just a shortcut for .toIList() but I didn't dive deeply

simon-the-shark · 2024-08-12T17:13:33Z

Thanks for the review! Hopefully i answered your questions here. If not, then hit me up or we can hop on a call too.

Basically for state management i reccomend the official riverpod.dev docs. They are crazy good. I like to read all the pages there from time to time when I have a problem.

And also if youre not that familiar with pattern matching concept you can check it out too. Its very powerfull tool, pretty new to dart and we're using it in a very limited range here

mikolaj-jalocha added 2 commits August 10, 2024 09:16

#129 - eliminate parkings flickering - replace typo

8be2346

#129 - eliminate parkings flickering - change ivalidateSelf to invali…

f095f58

…date

mikolaj-jalocha added the bug Something isn't working label Aug 10, 2024

mikolaj-jalocha requested a review from simon-the-shark August 10, 2024 07:18

mikolaj-jalocha self-assigned this Aug 10, 2024

mikolaj-jalocha linked an issue Aug 10, 2024 that may be closed by this pull request

fix/ eliminate parkings flickering #129

Closed

simon-the-shark reviewed Aug 10, 2024

View reviewed changes

fix(parkings): eliminate flickering

5c8e55a

simon-the-shark assigned simon-the-shark and mikolaj-jalocha and unassigned mikolaj-jalocha Aug 10, 2024

simon-the-shark marked this pull request as draft August 10, 2024 14:28

simon-the-shark marked this pull request as ready for review August 10, 2024 14:28

chore: update api schema

c0d5083

simon-the-shark force-pushed the fix/129-fix-eliminate-parkings-flickering branch from 1a93f8c to c0d5083 Compare August 10, 2024 17:42

Merge branch 'main' into fix/129-fix-eliminate-parkings-flickering

f32b1aa

mikolaj-jalocha commented Aug 12, 2024

View reviewed changes

simon-the-shark added 9 commits August 12, 2024 19:40

fix(home-view): increased tiles' visibility a bit

3e48d97

fix(home-screen): temp exchange sections order

0018843

refactor(guide-details): extract my expansion tile

c2bf781

feat(department-details): add exapanded tiles for field of studies

27c7a91

feat(department-details): add tiles icons

909cbc0

feat(deparment-details): add FoS url launching

81876ef

fix(home-view): eliminate jumping scroll (again, after sections reorder)

752ea52

feat(departments): more distinct tiles

2d42295

refactor(field-of-studies): extract extension

dbd9e0e

Merge branch 'main' into fix/129-fix-eliminate-parkings-flickering

7c1eba8

simon-the-shark merged commit 0fa9d5c into main Aug 12, 2024
2 checks passed

simon-the-shark deleted the fix/129-fix-eliminate-parkings-flickering branch August 12, 2024 17:41

This was referenced Aug 30, 2024

fix/ home screen offline refresh flickering #179

Closed

Refactor/IList and nullstafety tweaks #182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/129 fix eliminate parkings flickering #152

Fix/129 fix eliminate parkings flickering #152

mikolaj-jalocha commented Aug 10, 2024

simon-the-shark left a comment

simon-the-shark Aug 10, 2024

simon-the-shark commented Aug 10, 2024

simon-the-shark commented Aug 10, 2024 •

edited

Loading

mikolaj-jalocha commented Aug 10, 2024

mikolaj-jalocha left a comment

mikolaj-jalocha Aug 12, 2024

simon-the-shark Aug 12, 2024

mikolaj-jalocha Aug 12, 2024

mikolaj-jalocha Aug 12, 2024

simon-the-shark Aug 12, 2024

mikolaj-jalocha Aug 12, 2024

simon-the-shark Aug 12, 2024

mikolaj-jalocha Aug 12, 2024

simon-the-shark Aug 12, 2024

mikolaj-jalocha Aug 12, 2024

simon-the-shark Aug 12, 2024

simon-the-shark commented Aug 12, 2024 •

edited

Loading

Fix/129 fix eliminate parkings flickering #152

Fix/129 fix eliminate parkings flickering #152

Conversation

mikolaj-jalocha commented Aug 10, 2024

simon-the-shark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon-the-shark commented Aug 10, 2024

simon-the-shark commented Aug 10, 2024 • edited Loading

mikolaj-jalocha commented Aug 10, 2024

mikolaj-jalocha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon-the-shark commented Aug 12, 2024 • edited Loading

simon-the-shark commented Aug 10, 2024 •

edited

Loading

simon-the-shark commented Aug 12, 2024 •

edited

Loading