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

AC-821 Fix Java and Kotlin deprecation warnings #938

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

Conversation

Anshul-9923
Copy link
Contributor

@Anshul-9923 Anshul-9923 commented Jan 31, 2022

Description of what I changed

In Travis-ci while building this project many Java and Kotlin deprecation warnings were arising so to fix all those warnings, I have made changes in 4 files.

Issue I worked on

JIRA Issue: https://issues.openmrs.org/browse/AC-821

Checklist: I completed these to help reviewers :)

  • My pull request only contains ONE single commit
    (the number above, next to the 'Commits' tab is 1).
  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)
  • All new and existing tests passed.
  • My pull request is based on the latest changes of the master branch.

@Anshul-9923
Copy link
Contributor Author

@LuGO0 Please review my code and merge PR, if any changes are required let me know.

@Anshul-9923 Anshul-9923 changed the title AC-821 Fix deprecation warnings AC-821 Fix Java and Kotlin deprecation warnings Jan 31, 2022
@LuGO0
Copy link
Collaborator

LuGO0 commented Jan 31, 2022

@Anshul-9923 few warnings are still left just supress them if you think they are not critical

@Anshul-9923
Copy link
Contributor Author

Anshul-9923 commented Jan 31, 2022

@LuGO0 Should I also have to fix deprecated build features and can you please tell me which other files are needed to be changed to fix the remaining warnings?
I am not able to get which other files needed to be changed seeing the Travis-ci build.

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 1, 2022

Oh okay @Anshul-9923 I will look into it if I can find something.

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 2, 2022

public abstract class AppDatabase extends RoomDatabase {
                ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
1 warning

I think this is the one which is not related to build warnings is it? ^

w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\dashboard\DashboardFragment.kt: (116, 13): Condition 'root != null' is always 'true'
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\formadmission\FormAdmissionPresenter.kt: (142, 21): Unreachable code
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\formlist\FormListFragment.kt: (57, 51): Parameter 'parent' is never used, could be renamed to _
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\formlist\FormListFragment.kt: (57, 103): Parameter 'id' is never used, could be renamed to _
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\formlist\FormListFragment.kt: (72, 48): Parameter 'v' is never used, could be renamed to _
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\formlist\FormListFragment.kt: (168, 29): Variable 'json' initializer is redundant
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\formlist\FormListFragment.kt: (180, 32): Variable 'obj' initializer is redundant
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\activities\logs\LogsFragment.kt: (51, 30): Parameter 'context' is never used
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\listeners\watcher\PatientBirthdateValidatorWatcher.kt: (39, 13): Unreachable code
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\listeners\watcher\PatientBirthdateValidatorWatcher.kt: (40, 13): Unreachable code
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\listeners\watcher\PatientBirthdateValidatorWatcher.kt: (45, 17): Unreachable code
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\listeners\watcher\PatientBirthdateValidatorWatcher.kt: (46, 17): Unreachable code
w: C:\projects\openmrs-contrib-android-client\openmrs-client\src\main\java\org\openmrs\mobile\utilities\NotificationUtil.kt: (30, 43): 'constructor Builder(Context!)' is deprecated. Deprecated in Java

I came across these warnings as well in the CI build logs can you fix them with this PR only?

@Anshul-9923
Copy link
Contributor Author

@LuGO0 I have fixed the remaining deprecated warnings. Please review my code.

@Anshul-9923
Copy link
Contributor Author

Anshul-9923 commented Feb 5, 2022

@LuGO0 I did changes as told by you. PTAL

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 5, 2022

Please check the build is failing

@Anshul-9923
Copy link
Contributor Author

Anshul-9923 commented Feb 5, 2022

Please check the build is failing

@LuGO0 It is because of changing compileSdkVersion and targetSdkVersion to 30.

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 6, 2022

Please check the build is failing

@LuGO0 It is because of changing compileSdkVersion and targetSdkVersion to 30.

So

Please check the build is failing

@LuGO0 It is because of changing compileSdkVersion and targetSdkVersion to 30.

I think we dont need this change . We can simply have a build version check and have 2 different implementations for network Utils.
Suppress the older implementation using a method level supress annotation, does it help?

@Anshul-9923
Copy link
Contributor Author

Please check the build is failing

@LuGO0 It is because of changing compileSdkVersion and targetSdkVersion to 30.

So

Please check the build is failing

@LuGO0 It is because of changing compileSdkVersion and targetSdkVersion to 30.

I think we dont need this change . We can simply have a build version check and have 2 different implementations for network Utils. Suppress the older implementation using a method level supress annotation, does it help?

@LuGO0 So, Should I change library from android.preference.PreferenceManager to androidx.preference.PreferenceManager as it is deprecated warning.

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 8, 2022

I think we dont need this change . We can simply have a build version check and have 2 different implementations for network Utils. Suppress the older implementation using a method level supress annotation, does it help?

@LuGO0 So, Should I change library from android.preference.PreferenceManager to androidx.preference.PreferenceManager as it is deprecated warning.

Yes, just add a supress annotation locally if the build is failing and you are unable to fix it.

@Anshul-9923
Copy link
Contributor Author

Anshul-9923 commented Feb 8, 2022

I think we dont need this change . We can simply have a build version check and have 2 different implementations for network Utils. Suppress the older implementation using a method level supress annotation, does it help?

@LuGO0 So, Should I change library from android.preference.PreferenceManager to androidx.preference.PreferenceManager as it is deprecated warning.

Yes, just add a supress annotation locally if the build is failing and you are unable to fix it.

@LuGO0 After importing it build fails and asks for changing minCompileSdk to 31.
Screenshot (465)

@Anshul-9923
Copy link
Contributor Author

@LuGO0 Done with the changes as told by you.

val prefs = PreferenceManager.getDefaultSharedPreferences(OpenmrsAndroid.getInstance())
val toggle = prefs.getBoolean("sync", true)
return if (toggle) {
val prefs = OpenmrsAndroid.getInstance()?.let { android.preference.PreferenceManager.getDefaultSharedPreferences(it) }
Copy link
Collaborator

@LuGO0 LuGO0 Feb 10, 2022

Choose a reason for hiding this comment

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

there should be no change here right? after you reverted your changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, reverted the changes

@Anshul-9923
Copy link
Contributor Author

@LuGO0 I have reverted the changes wherever told by you. PTAL

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 11, 2022

Few warnings are still there but we can merge this will try running the application locally then merge this PR Thanks!

@Anshul-9923
Copy link
Contributor Author

Few warnings are still there but we can merge this will try running the application locally then merge this PR Thanks!

Ok @LuGO0

@Anshul-9923
Copy link
Contributor Author

@LuGO0 You have not merged my PR, do I need to do any changes?

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 16, 2022

Will run it locally then merge it.

@LuGO0
Copy link
Collaborator

LuGO0 commented Feb 19, 2022

@shubhamsgit need some help verifying this PR locally, and let me know if it works fine? Thanks!

@Anshul-9923
Copy link
Contributor Author

Anshul-9923 commented Mar 1, 2022

@LuGO0 When will the idealist release for GSoC projects by openmrs?

@LuGO0
Copy link
Collaborator

LuGO0 commented Mar 1, 2022

@LuGO0 When will the idealist release for GSoC projects by openmrs?

Not sure @Anshul-9923, you can find it on OpenMRS Talk or ask the GSOC co-ordinators like Jennifer and Moses !
Also sorry for the delay in this PR's review haven't got a chance to run it locally once !

@Anshul-9923
Copy link
Contributor Author

@LuGO0 When will the idealist release for GSoC projects by openmrs?

Not sure @Anshul-9923, you can find it on OpenMRS Talk or ask the GSOC co-ordinators like Jennifer and Moses ! Also sorry for the delay in this PR's review haven't got a chance to run it locally once !

Thanks @LuGO0

/**
* Starts new Activity depending on which ImageView triggered it
*/
private fun startNewActivity(clazz: Class<out ACBaseActivity?>) {
Copy link
Member

Choose a reason for hiding this comment

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

@Anshul-9923 As I can see, this PR is about fixing Java & Kotlin deprecation warnings but I see some deletions for the redundant code (which is not wrong). @LuGO0 Shouldn't a separate PR be opened for removing redundant code?

Copy link
Member

Choose a reason for hiding this comment

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

or better remove all the redundant code under this PR only? @LuGO0

@LuGO0
Copy link
Collaborator

LuGO0 commented Mar 1, 2022

Please dont delete any unreachable code since these are probably part of some unfinished module. I would be happy to keep them as they are for now with a warning supression.

Copy link
Member

@shubhamsgit shubhamsgit left a comment

Choose a reason for hiding this comment

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

PR title is misleading. PR fixes only two deprecations i.e., Preference Manager & Recycler View. Remaining part of the PR just removes redundant code. @Anshul-9923 Either you should remove all of the project's redundant code in this PR and rename it or if @LuGO0 suggests open a new PR.

@@ -31,6 +31,7 @@ import com.openmrs.android_sdk.library.api.repository.ProviderRepository
import org.openmrs.mobile.application.OpenMRS
import com.openmrs.android_sdk.library.listeners.retrofitcallbacks.DefaultResponseCallback

@Suppress("UNREACHABLE_CODE")
Copy link
Member

Choose a reason for hiding this comment

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

No need to suppress this warning. It can be resolved.

@@ -19,6 +19,7 @@ import android.widget.EditText
import com.openmrs.android_sdk.utilities.ApplicationConstants
import com.openmrs.android_sdk.utilities.StringUtils.notEmpty

@Suppress("UNREACHABLE_CODE")
Copy link
Member

Choose a reason for hiding this comment

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

Remove this too. Can be resolved.

@shubhamsgit
Copy link
Member

Please dont delete any unreachable code since these are probably part of some unfinished module. I would be happy to keep them as they are for now with a warning supression.

No need to delete @LuGO0 The unreachable code's position just has to be changed

@shubhamsgit
Copy link
Member

Please dont delete any unreachable code since these are probably part of some unfinished module. I would be happy to keep them as they are for now with a warning supression.

You mean 'redundant code' or 'unreachable code'?

@LuGO0
Copy link
Collaborator

LuGO0 commented Mar 1, 2022

Unreachable code, like the formAdmissionPresenter portion.

@shubhamsgit
Copy link
Member

shubhamsgit commented Mar 1, 2022

Unreachable code, like the formAdmissionPresenter portion.

I think that code can be removed as program control never reaches view.enableSubmitButton(true) because error(errorMessage) throws IllegalStateException and program terminates.

@shubhamsgit
Copy link
Member

and in PatientBirthdateValidatorWatcher, edmonth.text.clear() and edyr.text.clear() these two lines should be moved before the error() function. This can be counted as a bug though. Can open a new PR if you want.

@LuGO0
Copy link
Collaborator

LuGO0 commented Mar 2, 2022

Yupp create new PRs if needed with each one for a specific use case, don't do it in this one!

@shubhamsgit shubhamsgit added Changes Required Stale Inactivity for 60 days labels Jun 1, 2023
@FaheemorFAB
Copy link

I request the maintainer to clarify the exact work that you are looking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Required Stale Inactivity for 60 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants