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

feat: add hashtags to event cards #1595

Merged
merged 2 commits into from
Apr 20, 2019

Conversation

aggarwalpulkit596
Copy link
Contributor

@aggarwalpulkit596 aggarwalpulkit596 commented Apr 12, 2019

Fixes #1592

Changes: Added Event Types, SubTopics to the database to show hashtags in event cards

Screenshots for the change:
screenshot-1555442322949

Copy link
Contributor Author

@aggarwalpulkit596 aggarwalpulkit596 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikit19 any suggestions ?

@aggarwalpulkit596
Copy link
Contributor Author

I tried using resolve = true but it isn't Saving the object

@aggarwalpulkit596 aggarwalpulkit596 force-pushed the 1592 branch 3 times, most recently from a9914f2 to 3999c03 Compare April 15, 2019 05:33
@aggarwalpulkit596 aggarwalpulkit596 changed the title [WIP]feat: add hashtags to event cards feat: add hashtags to event cards Apr 15, 2019
@auto-label auto-label bot added the feature label Apr 15, 2019
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal @nikit19 updated please review

@iamareebjamal
Copy link
Member

Use the outline style in chip

@aggarwalpulkit596 aggarwalpulkit596 force-pushed the 1592 branch 2 times, most recently from 90a2afb to bdaa97f Compare April 15, 2019 19:57
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated

@iamareebjamal
Copy link
Member

Peer Review

@aggarwalpulkit596
Copy link
Contributor Author

@liveHarshit please review

@liveHarshit
Copy link
Member

UI is not looking good. You can decrease the size to make UI effective.

@aggarwalpulkit596
Copy link
Contributor Author

okay anything related to code @liveHarshit ?

Copy link
Member

@liveHarshit liveHarshit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert unnecessary changes

@@ -27,6 +27,7 @@ import kotlinx.android.synthetic.main.content_event.similarEventsContainer
import kotlinx.android.synthetic.main.content_event.view.eventDateDetailsFirst
import kotlinx.android.synthetic.main.content_event.view.eventDateDetailsSecond
import kotlinx.android.synthetic.main.content_event.view.eventDescription
import kotlinx.android.synthetic.main.content_event.view.eventImage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated changes

import kotlinx.android.synthetic.main.content_event.view.nestedContentEventScroll
import kotlinx.android.synthetic.main.content_event.view.organizerContainer
import kotlinx.android.synthetic.main.content_event.view.organizerLogoIcon
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

import kotlinx.android.synthetic.main.fragment_event.view.container
import kotlinx.android.synthetic.main.content_fetching_event_error.view.retry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@aggarwalpulkit596
Copy link
Contributor Author

UI is not looking good. You can decrease the size to make UI effective.

@liveHarshit which one you're referring to outline or purple colored ones?

@liveHarshit
Copy link
Member

UI is not looking good. You can decrease the size to make UI effective.

@liveHarshit which one you're referring to outline or purple colored ones?

The overall size of the chips.

@aggarwalpulkit596
Copy link
Contributor Author

@liveHarshit updated

@aggarwalpulkit596 aggarwalpulkit596 force-pushed the 1592 branch 2 times, most recently from 8b51d36 to 4d84231 Compare April 16, 2019 19:44
@iamareebjamal
Copy link
Member

The error is unrelated to GSON, and we are not even using GSON

@iamareebjamal
Copy link
Member

iamareebjamal commented Apr 17, 2019

If we are using it, then please remove it as we already have Jackson

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal here gson is used to
A Java serialization/deserialization library to convert Java Objects into JSON and back

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal i'm extremely sorry it was my mistake i forgot to push the build.gradle file in which i added a library

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal @liveHarshit updated it should work now

app/build.gradle Outdated
@@ -118,6 +118,7 @@ dependencies {
testImplementation "androidx.room:room-testing:${roomVersion}"
implementation 'androidx.preference:preference:1.1.0-alpha01'
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
implementation "com.google.code.gson:gson:2.8.5"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type converter can't work without them

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is GSON?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why do you need it when the project is using Jackson

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GSON
A Java serialization/deserialization library to convert Java Objects into JSON and back
Refrences
https://medium.com/@amit.bhandari/storing-java-objects-other-than-primitive-types-in-room-database-11e45f4f6d22

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class EventTypeConverter {

 @TypeConverter
fun toEventSubTopic(json: String): EventType? {
    val type = object : TypeToken<EventType>() {}.type
    return Gson().fromJson(json, type)
}

 @TypeConverter
fun toJson(eventSubTopic: EventType?) = Gson().toJson(eventSubTopic) }

here the implementation can't be achieved using jackson

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't it be done using Jackson?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I have a job of pointing to the documentation of a JSON serializing library

http://lmgtfy.com/?q=convert+object+jackson

Copy link
Contributor Author

@aggarwalpulkit596 aggarwalpulkit596 Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamareebjamal i'm sorry and intiallly thought you weren't getting the point of gson/jackson for the converter

app/build.gradle Outdated
@@ -118,6 +118,7 @@ dependencies {
testImplementation "androidx.room:room-testing:${roomVersion}"
implementation 'androidx.preference:preference:1.1.0-alpha01'
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
implementation "com.google.code.gson:gson:2.8.5"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i need it to convert event topic,subtopic and type model into event model by converting into gson and then converting it back to respective models

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated.Thank you

@@ -42,5 +50,9 @@ abstract class OpenEventDatabase : RoomDatabase() {

abstract fun eventTopicsDao(): EventTopicsDao

abstract fun eventTypesDao(): EventTypesDao

abstract fun eventSubTopicDao(): EventSubTopicDao
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are now stored with the event, this is unneeded

Copy link
Contributor Author

@aggarwalpulkit596 aggarwalpulkit596 Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we can use these dao in future for chips or filtering i suggest we should keep them

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use them unless there is a table for them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm saving them as well in a seperate table i could remove that part though but we may need them in future

@@ -76,6 +94,8 @@ class EventService(
return eventApi.searchEvents("name", locationName).flatMap { apiList ->
val eventIds = apiList.map { it.id }.toList()
eventTopicsDao.insertEventTopics(getEventTopicList(apiList))
eventTypesDao.insertEventTypes(getEventTypeList(apiList))
Copy link
Contributor Author

@aggarwalpulkit596 aggarwalpulkit596 Apr 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamareebjamal i'm saving them individually as well so there is a table for them as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you converting in JSON then? Just save them as JSON. No need to add tables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay i'll remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@iamareebjamal
Copy link
Member

Since new columns have been added to the table, there must be changes in schema JSON

@aggarwalpulkit596 aggarwalpulkit596 force-pushed the 1592 branch 2 times, most recently from 474e435 to 6acc55c Compare April 18, 2019 19:13
@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated JSON schema

@aggarwalpulkit596
Copy link
Contributor Author

Resolved Conflicts

class EventSubTopicConverter {
@TypeConverter
fun toEventSubTopic(json: String): EventSubTopic? {
return jacksonObjectMapper().readerFor(EventSubTopic::class.java).readValue<EventSubTopic>(json)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already an object mapper in dependency graph, use that

class EventSubTopicConverter {
@TypeConverter
fun toEventSubTopic(json: String): EventSubTopic? {
val objectMapper = ObjectMapper()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamareebjamal updated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is still instantiating ObjectMapper yourself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamareebjamal
val mapType = object : TypeReference() {}
return ObjectMapper().readValue(json, mapType)
like this ?
instead of
val objectMapper = ObjectMapper()
val mapType = object : TypeReference() {}
return ObjectMapper().readValue(json, mapType)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you need to use the ObjectMapper from DI. Who said anything about TypeReference or that your implementation was wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamareebjamal how i can pass that in type converter because type converter can only have empty constructors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and also ListAttendeeIdConverter uses the same pattern because we can't pass anything to type converter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamareebjamal anything else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can manually inject the dependency. See field injection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but for field injection, we need a context or something type converter don't even have that
i tired using StandAloneContext.getKoin().koinContext and val objectMapper: ObjectMapper by inject()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Create an issue to convert all object mappers from the one in DI

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal please review

@TypeConverter
fun toEventType(json: String): EventType? {
val mapType = object : TypeReference<EventType>() {}
return ObjectMapper().readValue(json, mapType) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert to how it was before

@aggarwalpulkit596
Copy link
Contributor Author

@iamareebjamal updated

@iamareebjamal iamareebjamal merged commit 0a2f916 into fossasia:development Apr 20, 2019
addchips(it.name)
}
event.eventSubTopic.let {
if (it != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason for explicit null checks?
Using ?.let would work?

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

Successfully merging this pull request may close these issues.

Add hashtags to event cards
4 participants