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

Migrated KotlinSynthetics to ViewBinding #5

Merged
merged 17 commits into from
Oct 13, 2022

Conversation

thedroiddiv
Copy link
Collaborator

@thedroiddiv thedroiddiv commented Oct 1, 2022

Purpose / Description

Migration to ViewBindings

Fixes

Fixes #4

How Has This Been Tested?

Tested on Pixel 4a API 30 and Realme 6i API 30

Learning (optional, can help others)

https://developer.android.com/topic/libraries/view-binding/migration

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@anuragshukla06
Copy link
Contributor

anuragshukla06 commented Oct 2, 2022

I think we have KotlineSynthetics in other Fragments as well :( Are you planning to raise seperate PRs for them?

@thedroiddiv
Copy link
Collaborator Author

Yeah I was thinking of doing it a bit by bit. It would be easy to review rather than ten-twelve classes all at once. More chances of getting something wrong unnoticed.

@thedroiddiv
Copy link
Collaborator Author

And this is why I wanted the #3 to be merged first 🥹
Screenshot 2022-10-03 at 1 03 04 AM

@anuragshukla06
Copy link
Contributor

Yeah I was thinking of doing it a bit by bit. It would be easy to review rather than ten-twelve classes all at once. More chances of getting something wrong unnoticed.

Sure, if you think there are not many changes, you can do it at once as well. As a sanity check after we have fully migrated, remove the dependency for KotlinSynthetics

@thedroiddiv thedroiddiv marked this pull request as draft October 3, 2022 15:34
@thedroiddiv
Copy link
Collaborator Author

making commit fragment by fragment, can be merged all at once.

@thedroiddiv thedroiddiv changed the title Migrated usage of KaryaToolbar from KotlinSynthetics to ViewBinding Migrated KotlinSynthetics to ViewBinding Oct 3, 2022
@thedroiddiv
Copy link
Collaborator Author

@anuragshukla06 since I din't had tasks for all scenario, haven been able to test all of the fragments. Once tested, will mark it ready. Meanwhile if you want to review each commit could parallely do as well.

@thedroiddiv
Copy link
Collaborator Author

All the commits are independent, so if one commit it changed it won't cause any conflict to other ones. So that's a good part.

@thedroiddiv thedroiddiv marked this pull request as ready for review October 6, 2022 10:08
@thedroiddiv
Copy link
Collaborator Author

@anuragshukla06 these five commits are checked, can be merged.

@thedroiddiv
Copy link
Collaborator Author

thedroiddiv commented Oct 6, 2022

These three also done. Seven fragments migrated and checked. Six more remaining.

Comment on lines +87 to +90
// Create label detail array // TODO: It is throwing ArrayIndexOutOfBounds (colors is always of size 4 but labels not)
labelDetailArray = Array(labels.size) { idx ->
Pair(labels[idx], colors[idx])
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@anuragshukla06 this line is throwing ArrayIndexOutOfBounds because colors is static array with size 4 but label may have different lengths.

Anyhow, I don't understand label here??

Copy link
Contributor

Choose a reason for hiding this comment

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

labels come from the server. At the time, we dont receive colors corresponding to labels from the server and the labels are not supposed to be more than 4.
Label in this image annotation task denotes one type of classification that the user can mark. We differentiate labels from each other visually by the colors for the user. There exists a one-to-one mapping from label to color

Copy link
Contributor

@anuragshukla06 anuragshukla06 left a comment

Choose a reason for hiding this comment

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

Did you remove the gradle dependency on Kotlin synthetic? Rest looks good to me

@thedroiddiv
Copy link
Collaborator Author

@anuragshukla06 finally done with the migration, removed the dependency as well. Next on list is gradle upgrade.

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

Successfully merging this pull request may close these issues.

None yet

2 participants