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

Code updated to run on SDK 33 and Gradle 7.5 #12

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

graffiti75
Copy link
Contributor

Unfortunately this repo is not working anymore because the last time it was updated was 3 years ago.

  • This repo is used on course "Android Kotlin Developer" of Udacity, more specifically on chapter "Advanced Android Apps with Kotlin - Part 2", class "5.7. Project: Build a Location Reminder App".
  • As a Code Reviewer for the Android course in Udacity, in order to help students of this course, I decided to create this PR to update the repo.
  • Between the requested changes, we have:
    • SDK version updated to version 33.
    • Gradle version updated to 7.5.
    • Dependencies updated on app/build.gradle.
    • AndroidManifest.xml updated.
    • settings.gradle updated.
    • Indentations fixed.

@graffiti75 graffiti75 requested a review from a team as a code owner April 14, 2023 21:26
@graffiti75 graffiti75 requested review from SudKul and removed request for a team April 14, 2023 21:26
- Back Pressed callback added to Fragments
- Old Menu implementations updated to menuHost.addMenuProvider
- Updating Permission's checking.
- Location tracking fixed.
- registerForActivityResult added.
- onActivityResult and onRequestPermissionsResult removed.
- Permission's Extension methods created.
* When the user confirms on the selected location, send back the selected location details
* to the view model and navigate back to the previous fragment to save the reminder and add
* the geofence.
*/
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 I can see you've shifted the TODO comments a little bit, so could you replicate the same syntax again? As I can see "TODO" word is missing and please use // to represent single lines of comments.

Copy link
Contributor Author

@graffiti75 graffiti75 May 3, 2023

Choose a reason for hiding this comment

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

Hello @islamalsawy .

Let me explain to you what happened here.

This PR was created 3 weeks ago with the intention to fix the starter code of project Build a Location Reminder App.

3 weeks ago the only commit to be merged with udacity:master was this commit, that is the starter commit for students to work on:

Code updated to run on SDK 33 and Gradle 7.5

Since then I kept implementating this project because I needed to check how deprecated Udacity's suggestions were on Advanced Android with Kotlin course. There are several deprecations here. All the following concepts are deprecated, and Udacity haven't updated this in the course:

  • Notifications, that now need Runtime Permissions and need newer Flags to be able to run.
  • Geofences, that were incorrectly using JobIntentService (that doesn't work anymore since 2021), and now need to be implemented using WorkManager.
  • onRequestPermission method, that was deprecated by Google, and now need to be replaced by registerForActivityResult.
  • onCreateOptionsMenu and onOptionsItemSelected methods, that are now deprecated by Google, and need to be replaced by MenuHost.addMenuProvider.
  • onActivityResult method, that is now deprecated by Google, and need to be replaced by registerForActivityResult.
  • Also, the Location Tracking feature using GPS in Android that was never taught in this Udacity course. This feature is requested on project Build a Location Reminder App, and in the last years the implementation of this feature in Android changed dramatically.

Any student enrolled on this course, to have this final project finished, will need to fix all those deprecations! I think this is a very heavy load for them to carry, right?

We have a lot to fix on this course!!

Anyway, to begin with, what I suggest here is that I close this PR and create a newer one with only the starter commit (as explained earlier). This way future students will be able, at least, to start this project with an updated starter code, containing all the correct Gradle dependencies and SDK versions up to date.

If you need further explanations, please contact me in private on Udacity Circle website.

Copy link
Contributor Author

@graffiti75 graffiti75 May 3, 2023

Choose a reason for hiding this comment

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

New PR created: #14

Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 Thank you for putting things in context. Let's start with approving this new PR. I am going to share more details with you Circle.

binding = DataBindingUtil.setContentView(this, layoutId)
binding.authButton.text = getString(R.string.login)

// TODO: Line below was commented to avoid losing time Login In on Firebase
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 I can see you've added new lines, but I can not see any TODOs listed here in this file for the learners!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR created: #14


//TODO: implement the onReceive method to receive the geofencing events at the background
override fun onReceive(context: Context, intent: Intent) {
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 Kindly, where I can find the TODO: implement onRecieve method... in your updated code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR created: #14

override fun onOptionsItemSelected(item: MenuItem): Boolean {
when (item.itemId) {
R.id.logout -> {
// TODO: add the logout implementation
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 Where I can find tis TODO in your updated code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR created: #14

}

// Use the user entered reminder details to:
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 I can see this a TODO, so could you add the word TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR created: #14

android:layout_width="match_parent"
android:layout_height="match_parent">
<!--TODO: Add the map fragment for the user to select the location-->
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 Kindly, why did you decide to delete this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR created: #14

@@ -1,5 +1,5 @@
<resources>
<string name="app_name">Location Reminders</string>
<string name="app_name">AAA Location Reminders</string>
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 Could explain why did you add AAA, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR created: #14

// Architecture Components
//Navigation dependencies
implementation 'androidx.appcompat:appcompat:1.2.0'
implementation 'androidx.constraintlayout:constraintlayout:2.0.0-rc1'
Copy link
Member

Choose a reason for hiding this comment

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

@graffiti75 Do you see the first change of this PR is not required here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR created: #14

@SudKul
Copy link
Contributor

SudKul commented May 4, 2023

@islamalsawy
Let me know when the PR is ready to merge. Also, can you set up some time with the Mentor to update all projects in the ND with the same Gradle, SDK, and dependencies? This will be a paid assignment. Check with Varun/Xiaodi for more details.

@islamalsawy
Copy link
Member

@SudKul Sure. I am going to review the PR and let you know once finished by EOD tomorrow. Thank you for letting me know. Also, I am going to check with the team for the next collaborations of the ND.

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.

3 participants