codernaut / citizenconnect

Apache License 2.0
1 stars 9 forks source link

Splash screen becomes non responsive when loading data for first time #18

Closed codernaut closed 6 years ago

codernaut commented 6 years ago

When we install app for first time all the data sets are loaded, Though its done in a separate asynch tasks the spalsh screen becomes unresponsive. Investigate and fix

wahibhaq commented 6 years ago

@codernaut I am investigating this so consider that it is assigned to me. I believe I requested earlier to allow me the rights to assign myself to an issue.

codernaut commented 6 years ago

accept invite and then i will assign you

wahibhaq commented 6 years ago

thanks for adding me as a collaborator 👍

wahibhaq commented 6 years ago

@codernaut In order to better understand the problem, I need to see how does the dataset or Medical Stores schema looks like on firebase database. Can you share a screenshot or mention the fields and the structure of children.

codernaut commented 6 years ago

i will share first thing tomorrow whole thing on json

wahibhaq commented 6 years ago

ok please do so. I will be waiting.

codernaut commented 6 years ago

checked in latest db snapshot

codernaut commented 6 years ago

thanks for taking this up

wahibhaq commented 6 years ago

yeah got it. Thanks for sharing.

wahibhaq commented 6 years ago

@codernaut so I have thoroughly investigated the issue on app startup and there are several things going wrong from fetching approach, running on main thread instead of Async and also the firebase data is creating problems.

First thing first, I would like you to change the data on database as I can expect you might not allow me the access to firebase. I would like to get these properties fixed and some questions to reconsider.

These all points are creating internal exceptions which contributes to slowing down of fetching cycle. Please also upload the latest db-snapshot after the changes on firebase backend and tag me here.

codernaut commented 6 years ago

regarding fixing "running on main thread instead of Async" does not really depends upon the data correction. Though i noticed in review that we have asynch in splash screen and it is supposed to parallize the data loading am i wrong here ?

Secondly data sanitation is done by another resource let me contact him

wahibhaq commented 6 years ago

It is currently using a sync operation and not async for doing the realm transaction. I replaced it with async but the effect is incomplete unless the data sanitation happens.

codernaut commented 6 years ago

cool (Y) does the busy indicator run ?

codernaut commented 6 years ago

submit pr and i will ask ali raza the sanitization resource to fix it

wahibhaq commented 6 years ago

Sorry didn't understand the point about busy indicator.

Secondly, I don't think it makes sense to submit PR without confirming that my changes are not further breaking anything. Sanitization of data is like a prerequisite for my PR. I will wait for the changes.

codernaut commented 6 years ago

the progress indicator -- circle i need to understand where we missed doing it asynch please do submit a pr thanks

codernaut commented 6 years ago

regarding any unintended domino effect you can verify on the current data set

wahibhaq commented 6 years ago

Oh ok, now I got it. The progress bar is running but I can see that it still gets stuck and logs show that there is a massive occurrence of realm error because of fields mismatch in Dataset.java. Also I can fairly see that using one model class for the whole db source is not an optimal way and it should be divided into smaller models like for one for NGOs, Medical Stores etc.

I will definitely submit PR but it is still work in progress. Its my fair assumption that those fields mismatch due to mistakes in firebase data is contributing to extra work but probably it requires further work in terms of making the fetching part more lighter and multi-threaded. I have a solution in mind but want to move step by step.

shahzaib414 commented 6 years ago

i created one model class , because as per current structure,if you add more dataset other then NGO , medical store etc , you don't need to update your App it will populate as per firebase data source , without adding new model , the way you are suggesting i.e each model for each data set , in that case every time we add new category in data sets we have to update App and release new version of App. Gulzaib , your suggestions ?

wahibhaq commented 6 years ago

I can see your point, I would need to investigate it to be sure about this limitation.

But otherwise, is it a business requirement to regularly add new categories to the app? I mean when you will add a new category in firebase datastore, it will still require changes in "Services" screen to show the data right? Or it will also display newly added category without need of releasing newer app version?

shahzaib414 commented 6 years ago

yes you can say it's business requirement, data sets can be add any time , secondly when you add data to firebase you will also add icons, title and background details in firebase so you don't need to update App , actually i tried to structure App in a way so that App should less dependent on firebase data etc.

On 5 Feb 2018 11:28 pm, "Wahib Ul Haq" notifications@github.com wrote:

I can see your point, I would need to investigate it to be sure about this limitation.

But otherwise, is it a business requirement to regularly add new categories to the app? I mean when you will add a new category in firebase datastore, it will still require changes in "Services" screen to show the data right? Or it will also display newly added category without need of releasing newer app version?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/codernaut/citizenconnect/issues/18#issuecomment-363175791, or mute the thread https://github.com/notifications/unsubscribe-auth/AWQBASvb3i3qT7L7LtIXqMu4Pl-MNi-Lks5tR0hNgaJpZM4Ryu-i .

codernaut commented 6 years ago

Shazaib is right. we need to cater for requirement that when ever new data set is added there should be no change on app side it should automatically be used.

wahibhaq commented 6 years ago

Ok, I can see the point here.

@shahzaib414 Help me understand one thing. The reason you are fetching only name and address in the Dataset.java is because the listview in Services tab only needs to display name and address? At least that's what I see in the app for NGOs, Medical Stores and Establishments.

 object.setName(dataSet.getName());
                                object.setAddress(dataSet.getAddress());
                                object.setDataSetType(_child.child("type").getValue().toString());

Why I am asking this is because there are other properties as well like male, female, employees, registration etc. but their values are not stored in app start. Also the listview is not clickable to show other details but is this something to be implemented in future?

This is imp information because Realm is complaining about missing getter/setters functions.

shahzaib414 commented 6 years ago

Yes according to current requirement, we have to show only name and address , that's why for now we are not dealing with other values . Regarding list view clickable , that will be implemented in future. If it's issue of get/set of values which we are not storing i.e other then name and address, you can comment those values for now. I think that will be require when we will implement clickable listview

On Tue, Feb 6, 2018 at 12:42 PM, Wahib Ul Haq notifications@github.com wrote:

Ok, I can see the point here.

@shahzaib414 https://github.com/shahzaib414 Help me understand one thing. The reason you are fetching only name and address in the Dataset.java is because the listview in Services tab only needs to display name and address?

object.setName(dataSet.getName()); object.setAddress(dataSet.getAddress()); object.setDataSetType(_child.child("type").getValue().toString());

Why I am asking this is because there are other properties as well like male, female, employees, registration etc. but their values are not stored in app start. Also the listview is not clickable to show other details but is this something to be implemented in future?

This is imp information because Realm is complaining about missing getter/setters functions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/codernaut/citizenconnect/issues/18#issuecomment-363336932, or mute the thread https://github.com/notifications/unsubscribe-auth/AWQBAbcN39O8Wba2DYVkA2imP2IhOTdzks5tSAIigaJpZM4Ryu-i .

wahibhaq commented 6 years ago

so do you think it makes sense to fetch all data in app start and store it in one big model with properties which are unrelated to each other? I don't see it as a reasonable solution. For e.g imagine dataset object is created for Medical Store which will have a property of male, female, total employees as blank maybe because this piece of information doesn't belong to Medical store. You see my point?

I can understand that you want to keep things dynamic but we need to come up with a better solution on app side when it comes to retrieving and storing it in model classes.

shahzaib414 commented 6 years ago

it all depends on your current requirement , i feel that if we want to make App dynamics then this one is fine , if you skip this thing then obviously many alternatives. by keeping this solution i think you can still optimise this by doing all stuff in seperate background service i.e downloading all data .

On 6 Feb 2018 1:31 pm, "Wahib Ul Haq" notifications@github.com wrote:

so do you think it makes sense to fetch all data in app start and store it in one big model with properties which are unrelated to each other? I don't see it as a reasonable solution. For e.g imagine dataset object is created for Medical Store which will have a property of male, female, total employees as blank maybe because this piece of information doesn't belong to Medical store. You see my point?

I can understand that you want to keep things dynamic but we need to come up with a better solution on app side when it comes to retrieving and storing it in model classes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/codernaut/citizenconnect/issues/18#issuecomment-363347613, or mute the thread https://github.com/notifications/unsubscribe-auth/AWQBAVhfXAGdtmc-UqXEmU6S15kCggEEks5tSA3FgaJpZM4Ryu-i .

wahibhaq commented 6 years ago

for now, i am focusing on optimising the fetching part but this is definitely something which can be improved later. It will be equally important to make things scalable for future additions to database.

@codernaut any update from Ali Raza about data sanitisation?

codernaut commented 6 years ago

boss data sanitization ko nahi cheda imp demo coming up on Friday if we can have fetching optimized till then would be great. Our end goal regardless who or what has been implemented is to dynamically populate what ever attribute we have in data but thats not our objective till Friday.

wahibhaq commented 6 years ago

hmm ok let me see. Nevertheless, I added in my todo list to make it work even without the data cleanup but just didn't get time in past 2 days. I will try to find some time today to fix it. I haven't used Realm before so that brings a bit of learning curve for me.

wahibhaq commented 6 years ago

I am done with fixing the task. I think its good enough to go forward. I am seeing some problem with commits history and need to cherry-pick them. So as soon as I have fixed that, I will create PR for review and merge.

wahibhaq commented 6 years ago

@codernaut here is the PR for review https://github.com/codernaut/citizenconnect/pull/25

wahibhaq commented 6 years ago

@codernaut any updates on data sanitisation task? Any hopes to make it happen in near future? Also who adds such record on firebase end? maybe define some naming convention for key

codernaut commented 6 years ago

actually we have gone through a couple of iterations for design.

There is a corresponding data portal.

Previously we were meaning to use firebase as a data repo for app and porta

but now we have diverged from that

now the data port is using postgres and flask based json service

one of the task for shahzaib this week is to move to that

wahibhaq commented 6 years ago

ahan ok. Interesting!

There is a web interface as well for this project?

So safe to assume that someone will enter data on flask based admin and it will be pushed to firebase or both are maintained separately?

wahibhaq commented 6 years ago

PR is merged