search

ICT4Dat / ict4dat-news-android

ICT4D.at's App to combine all ICT4D news into one Android application
Apache License 2.0
5 stars 1 forks source link

Refs #122 - Feat new news badge #141

Closed rajasone closed 5 years ago

rajasone commented 5 years ago

47052439_426099017922958_7235347317530820608_n

spipau commented 5 years ago

Hi, could you please resolve the merge issues - you know best about your changes 😉 Thanks!

spipau commented 5 years ago

I change the whole logic, since with the previous logic there was a flaw. The last date was taken from the latest news in the database. First the value was always updates, since the Fragment was subscribed via a LiveData to it. So: latestNewsDate -> update -> latestNewsDate gets also updated and loses its old state.

What we actually need to know is, when the last update from the server was done AND also not by the Work Manager Task. We have a value called lastAutomaticNewsUpdateLocalDate in the Share Preferences which holds this information. So I know get it at the beginning and use it in the Adapter.

Please review my changes, I didn't really test it with real data yet...

rajasone commented 5 years ago

@spipau I think there is something missing because it is not showing badge at all (please don't get me wrong I felt I am really pushing you on this issue sorry :smile:) I think there are few use cases for this feature,

  1. User open app for the first time and there is no news exist(all news should show badge)
  2. User have old news list and user want to update new list using refresh button, if new news found (Only new news show badge)
  3. User have updated news list (No news show badge)

I think passing the Date of most recent news is not enough for our need because passing Date to constructor will only help if News are getting download on startup by default, let's say user have the old list of news and user press the refresh button to update list, we are not updating recyclerview mostRecentNewsPublishDateTime field which cause a problem while identifying new news.

spipau commented 5 years ago

I see, I think we have a complete different vision of the feature 😄 - that's why we always talk in two different directions... (and it's very good that you push this here, thanks for that!)

So here is my understanding on how it should work: The user has a list of news in the database. Two scenarios:

  1. He/she opens the application and sees the list. The update starts to spin, since the automatic update was trigged OR the user starts the update by him/herself. All the news which are then added to this list should have the badge. So we need to know the time of the last update (excluding the Worker).
  2. He/she sees the notification on his/her phone and opens the app. All news which got downloaded in the background by the work should have the NEW badge.

Regarding:

User open app for the first time and there is no news exist(all news should show badge)

I thought that too at the beginning and I almost implemented it. But then I decided not to, because we would mark "old" news as new. Check some of the dates more down the list, they are not new at all. And that's why I would not show the NEW badge on the first download.

rajasone commented 5 years ago

@spipau thanks for the explanation , I think we both are agree with the first 2 use cases of this feature, I have a bit different definition/understanding of the New and may be I am completely wrong just want to share my thought :smile: My understanding regarding New is let's say I was in Vienna and my parents renovate my room and placed new furniture, LED, Playstation etc. and when I came back home I saw those things for the first time so, all these things which I saw for the first time they were new to me even though they were placed there an year ago. In that perspective I think we should show badge to all news when user download them for the very first time. For user all of the news are new because user is going to view them for the first time even though few news are year old. Please excuse my lame example I showed off many things :rofl: and please let me know what you think.

spipau commented 5 years ago

@rajasone haha, I like your example and it's not lame at all 😄 I understand what you mean, but I don't think that this edge case is so important that we extra put in some logic to show at the first app start a the NEW badge. I scanned quickly through the code and couldn't find a quick and nice solution. If this is important to you and you find a good and simple solution, then please feel free to implement this extra feature. If not, then I can live with the fact, that the user sees this one time not the NEW badge 😉

rajasone commented 5 years ago

@spipau sounds good to me, we may ignore this edge case for now, thanks for your explanation :smile: