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

second sample #6

Open
OyaCanli opened this issue Nov 27, 2018 · 23 comments
Open

second sample #6

OyaCanli opened this issue Nov 27, 2018 · 23 comments

Comments

@OyaCanli
Copy link
Member

So up to now, we have been saying that we want another databinding sample with network connection. Let's discuss the details of it.
-Are we using NewsApi ?
-How do we deal with network request? Retrofit? Raw http request?
-How will be the structure? Will we again show a list of items in recyclerview and a detail page?
-Normally, it is not good practice to make network request from main activity. It would be better to put it in ViewModel or a singleton Repository. But shall we avoid those for simplicity?

@GretaGri
Copy link
Member

My ideas to those questions:

  • yes, we are going to use NewsAPI;
  • maybe Retrofit could be simple enough?
  • Could be the same. Thinking if there is any reason to change? Do we want to show something more?
  • we could use ViewModel or Repository, I am not sure if we are going for simplicity or more for good practice:)

@rajtheinnovator
Copy link
Member

@OyaCanli @GretaGri yes, we'd be using NewsApi for that purpose. Retrofit is good but as the focus here is to help people on Databinding mainly, so even simple http call is also fine. We've things implemented in simple form already in the first sample and so now we can have one example with best practice now. That's what I think here.

@OyaCanli
Copy link
Member Author

I don't think retrofit is more complicated than making raw http request. But both are fine for me.
For a news app, I guess it makes sense to show articles in a list and then show individual articles in a details screen. Though, from the databinding perspective, its gonna have a similar structure to previous sample. The only interesting new think I can think of is to pass a boolean to the binding for differentiating loading states and using ternary operator to play with visibilities. And we'll use a binding adapter for loading images with Glide.
May be we can use a layout with include tag, in order to demonstrate how to reach a 'included' layout.
If we use a viewmodel, may be we can pass the viewmodel to databinding, instead of the item..

@rajtheinnovator
Copy link
Member

@OyaCanli the use of included tag would be a nice addition here. For the ViewModel part, I'm not sure how you plan to do that because I've not done deep diving on this topic but I'm sure that you two have done enough research and so it must be something nice. So please go with what you find the best way there. For http request, I understand that, it's not easy itself but I've seen people working with it from beginning and so even if that's not easy, it look intuitive to them. Retrofit is really nice but people try it generally after already working with simple http request so maybe they'll find http request more natural to them 👍

@OyaCanli
Copy link
Member Author

OyaCanli commented Nov 30, 2018

Ok @rajtheinnovator. Let's go with http request. Frankly, I had found myself the underlying mechanisms of http request complicated as well. But it was a chunk of code that we could copy paste and reuse, so it was easy in that sense.

I have been playing with viewmodels for a while, I'm comfortable with that. For passing the viewmodel to databinding vs passing directly the item, I'm not sure whichis better in fact. I guess it depends on case. I have seen both in google samples. I'll check them out again and compare. And we'll discuss it more as the project takes shape and we know more details.

@OyaCanli
Copy link
Member Author

OyaCanli commented Dec 1, 2018

@OyaCanli
Copy link
Member Author

OyaCanli commented Dec 1, 2018

One interesting thing is the implementation of BaseObservable to make the binding update itself. But I think we can do it with livedata as well. Once we decide the details of project structure, I'm voluntary to experiment with it.

@OyaCanli
Copy link
Member Author

OyaCanli commented Dec 1, 2018

Looking at NewsApi, I see all articles have these attributes: source(name of journal), author, title, description, url(for the article), urlToImage, publishedAt, content.
For the list items, I guess we'll use titles, authors, images(thus image url), publishedAt, and may be also source. Description and content are long for list items. If we'll have a detail view for reading the article in app, we won't need article url. But we'll need description and content.
And if we'll have a detail view for in app reading, one question I'd ask is to whether it is better to parse all these data (including description and content) at once and then simply pass selected item to details screen, OR to exclude the description and content when parsing for the list and then parse whole contents only for the selected item in the details screen. (I mean, in order not to make a large unnecessary operation, since article contents can be very long)
But since this is only a sample, may be we simply limit the article numbers and don't worry about these details?

@OyaCanli
Copy link
Member Author

OyaCanli commented Dec 8, 2018

@GretaGri @rajtheinnovator Shall we finalize the structure of the app and share tasks to begin working?
-Two fragments: one with list and one with details seems ok? (Have you read my concerns above)
-Shall we use a fix simple query, like 8 or 10 articles in a given topic(may be tech?)

@GretaGri Do you want to again design layouts and reuse them on kotlin version? Or do you want to do something else this time?

@OyaCanli
Copy link
Member Author

OyaCanli commented Dec 9, 2018

I recognized that we need to use special escape characters for logical operators, like in here: https://github.com/OyaCanli/TrainInventory-Firebase/blob/master/app/src/main/res/layout/fragment_train_list.xml#L34
May be we can use it in our sample as well. We won't have empty state I guess, but we can add something for no network case.

@GretaGri
Copy link
Member

I can work again with layouts and with Kotin codes.

@OyaCanli
Copy link
Member Author

Ok, that sounds good @GretaGri.
I assume we'll have again a single activity with two fragments, one fragment with a list, one fragment with details?
Something different may be a loading indicator and no network image/message besides recyclerview.

@OyaCanli
Copy link
Member Author

Shall we let you first make the layouts then @GretaGri ? Will you have time for that in the following days, for example? Otherwise I can make some parts without UI..

@GretaGri
Copy link
Member

GretaGri commented Dec 13, 2018

@OyaCanli I will try to work with it this weekend :) but you can start too on the parts that do not need layouts 💯

@OyaCanli
Copy link
Member Author

Ok, great @GretaGri. Yeah, I was thinking the same. I can work on different parts. Since we'll be working on different files, it won't be difficult to merge. Is it ok @rajtheinnovator ?

@rajtheinnovator
Copy link
Member

rajtheinnovator commented Dec 14, 2018

@OyaCanli Yeah, I do think it's all fine. There should not be any issue when merging it. And obviously the approach looks good as this way we can avoid any delay and keep working on it continuously.

@OyaCanli
Copy link
Member Author

@rajtheinnovator @GretaGri I worked on this today and added the parts not related to UI. I made a PR. I'll continue with the rest after Greta adds the layouts.

I wonder something: I get this "Asynctask should be static.." warning. But the asynctask is inside the singleton repository, not inside the activity. Is there still a risk of memory leak?

@rajtheinnovator
Copy link
Member

@OyaCanli if we're having something singleton, that's one of the best way to avoid memory leaks but if you'd like to really see in action if there is any memory leak, there is an awesome library which I like to use and you can check that yourself. Here's that: https://github.com/square/leakcanary. So give it a go and let me know what you find there.

@OyaCanli
Copy link
Member Author

Thanks @rajtheinnovator ! I had heard LeakCanary, but haven't tried yet. I'll discover that when I have time.

I have a pending PR by the way.

@rajtheinnovator
Copy link
Member

@OyaCanli you can ask for any help if needed for LeakCanary, but I guess you can do that easily as it's not that tricky to use.

@OyaCanli
Copy link
Member Author

@rajtheinnovator I tried out LeakCanary. No leaks are seen in our simple sample.
Though? I can see lots of leaks in another app of mine 😅 Thanks for this tip! I didn't know it was that simple to use.

@rajtheinnovator
Copy link
Member

That's great you found it helpful, @OyaCanli I too found that this helped me realize how and where the problems are in other parts of my various projects. Definitely a thing to have in our development toolbox :)

@OyaCanli
Copy link
Member Author

@GretaGri I'm working on the second sample and once the data began to come from the api, the apperance of the layouts seem different than your intended designs. Titles are long and seem so huge all together. Sometimes authors are long.. And I felt the need to change some fields: descriptions are long for list items, so I want to remove them from there and add to details screen. And I want to seperate date and time either with an empty space or with a new line, or as two textboxes. So I'll make some adjustments in the layouts if you don't mind. You'll look at later and make further adjustments again if you wish, ok?

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

No branches or pull requests

3 participants