Skip to content
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

Constant loading #89

Closed
root458 opened this issue Aug 16, 2024 · 17 comments · Fixed by #96
Closed

Constant loading #89

root458 opened this issue Aug 16, 2024 · 17 comments · Fixed by #96
Assignees
Labels
enhancement New feature or request

Comments

@root458
Copy link
Contributor

root458 commented Aug 16, 2024

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
Copy link
Contributor

@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.

@MillerAdulu MillerAdulu added the planning This is being planned for. Don't start yet. label Aug 16, 2024
@MillerAdulu MillerAdulu changed the title Constant Loading Constant loading Aug 16, 2024
@MillerAdulu MillerAdulu added the enhancement New feature or request label Aug 16, 2024
@root458
Copy link
Contributor Author

root458 commented Aug 16, 2024

@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
Copy link
Contributor

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
Copy link
Contributor Author

root458 commented Aug 16, 2024

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
Copy link
Contributor

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 MillerAdulu self-assigned this Aug 18, 2024
@MillerAdulu
Copy link
Contributor

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
Copy link
Contributor Author

root458 commented Aug 18, 2024

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
Copy link
Contributor

@root458,

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

@MillerAdulu MillerAdulu removed the planning This is being planned for. Don't start yet. label Aug 18, 2024
@root458
Copy link
Contributor Author

root458 commented Aug 18, 2024

@MillerAdulu

Add these changes to build.yaml

global_options: freezed:freezed: runs_before: - isar_generator:isar_generator

Define these constants:
const isarCollection = Collection(ignore: {'copyWith'}); This is used for collections.
const isarEmbedded = Embedded(ignore: {'copyWith', 'toJson', 'fromJson'}); This is used for models which are objects of Collections

@root458
Copy link
Contributor Author

root458 commented Aug 18, 2024

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
Copy link
Contributor Author

root458 commented Aug 18, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

root458 commented Aug 18, 2024

@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
Copy link
Contributor Author

root458 commented Aug 18, 2024

Having a look at #96

@MillerAdulu
Copy link
Contributor

@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
Copy link
Contributor Author

root458 commented Aug 18, 2024

@MillerAdulu,

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

@MillerAdulu
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants