droidconKE / flutterconKEApp

https://play.google.com/store/apps/details?id=dev.flutterconke.app
14 stars 25 forks source link

Constant loading #89

Closed root458 closed 3 months ago

root458 commented 3 months ago

The app is constantly loading when switching between different screens. The experience is a bit draining from the waiting one has to do every time.

MillerAdulu commented 3 months ago

@root458,

Do you have a proposal on how you intend to move this along that we can discuss for action? The current strategy is to get the app to feature completion, before we can start on optimization around these issues, but since this has come up now, we can have a discussion around this.

root458 commented 3 months ago

@MillerAdulu

Understood. I lean towards caching the data locally and then hiding the refresh from users whenever any data is available. When there is no data, the loading can be shown to the user.

Various approaches are available for caching. We can use Hive and reset the local storage whenever it crashes (this is a chronic issue it has). This will, however, end any session the user had on the app.

We can alternatively use Isar which offers migrations. But it is currently not supported on the Web. This will not be a problem if the app will only be available for Android and iOS.

MillerAdulu commented 3 months ago

The app is currently Android & iOS only. The sessions tab currently uses Hive, however, this was a temporary work around to avoid adding Isar until we were feature complete. The sessions tab is currently offline-first and it should be snappier than the other pages.

We can move forward with Hive, but there are a number of issues that will have to close first before we can move to an offline-first strategy.

root458 commented 3 months ago

I noticed it didn't visually refresh. I propose we do the same for the other screens.

We can discuss the issues and a strategy to work towards offline-first functionality.

MillerAdulu commented 3 months ago

Yes. We can do that. But for now, this is on hold, until we are done with a couple of other things to prevent unpleasant merge conflicts.

MillerAdulu commented 3 months ago

Will go with the approach of using Isar (https://isar.dev/tutorials/quickstart.html) for this. Ideally a great database to help with persisting data so that users can navigate sessions quickly without having to wait for a data load each time.

root458 commented 3 months ago

This is well in order @MillerAdulu.

BTW, there is a way of creating Isar models using the same code-generated models from freezed. This can help reduce the duplication that comes with having duplicate models for persistence.

MillerAdulu commented 3 months ago

@root458,

Do share a sample. I am not aware of this approach.

root458 commented 3 months ago

@MillerAdulu

Add these changes to build.yaml

`global_options: freezed:freezed: runs_before:

root458 commented 3 months ago

Collections will then be defined as shown below:

@unfreezed @isarCollection class CollectionClass with _$CollectionClass { factory CollectionClass({ required String requiredProperty, @Default('') String defaultProperty, EmbeddedClass? embeddedClass, }) = _CollectionClass;

factory CollectionClass.fromJson(Map<String, dynamic> json) => _$CollectionClassFromJson(json);

CollectionClass._();

Id get id => Isar.autoIncrement; }

root458 commented 3 months ago

Then to enable migrations:

The properties on Embedded classes are defined as null or with default arguments like so:

@freezed @isarEmbedded class EmbeddedClass with _$EmbeddedClass { factory EmbeddedClass({ @Default('') String propertyOne, @Default('') String propertyTwo, }) = _EmbeddedClass;

factory EmbeddedClass.fromJson(Map<String, dynamic> json) => _$EmbeddedClassFromJson(json);

MillerAdulu commented 3 months ago

@root458,

Managed to check this out working on #96. However, my concern with the embedded objects is data integrity. freezed does allow proper data definitions. However, because Isar embedded objects have to be nullable and every property nullable as well, it means we lose a great benefit of freezed that is proper data definitions. As such, my dilemma is whether it's worth losing proper data decoding mechanisms, for the sake of not repeating models.

Another additional question I have is, assuming an app where data comes in via different formats (eg Websocket & an HTTP API), we can define different freezed models to cater for both then when it comes to persisting, we persist a uniform model. In this case, having separate definitions for local storage is beneficial. Do let me know what you think. I am keen to get your input on #96 as well.

root458 commented 3 months ago

@MillerAdulu,

I see. However from experience, having the code generated with nullable embedded classes didn't change much. The difference was only at the part where the class objects had to be referenced. On this sections, null fallbacks were used. But again this could be avoided by using Default arguments.

If by data decoding mechanisms, you're referring to the toJson and fromJson methods generated by freezed, those are not lost. The Isar additions are generated after the models are done generating, as specified by this rule on the build.yaml file.

However, the approach makes sense when the data-from-different-sources factor is considered. The challenge may be on ensuring that both the Collection and Embedded model classes are consistent when changes are made to either of them. It takes extra effort to do this and when unaware, this change may be forgotten altogether.

For this, I propose that we add documentation for the model classes like so

Say this is Class Room

/// [RoomCollection] class persists this model. Update this class in case of modifications class Room ( // Properties here )

The RoomCollection class is then made referenceable by importing the file into which it is defined.

This can be done for the Embedded classes too.

root458 commented 3 months ago

Having a look at #96

MillerAdulu commented 3 months ago

@root458

At the moment, I am having a bit of an issue accepting that we should have many annotations across different packages on one model.

The other enquiry: When a model has id from the API, the Isar model still requires an id property. How does this get gone about without causing collisions.

root458 commented 3 months ago

@MillerAdulu,

I suggest using custom property names in combination with JsonKey('') from freezed

MillerAdulu commented 3 months ago

@MillerAdulu,

I suggest using custom property names in combination with JsonKey('') from freezed

I tried this and there was a collision. Which I wasn't able to test by renaming the Isar ID to something else cause of the embedding issue I am having trouble being comfortable with.