-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Removed all hardcoded string in DeleteHelper #3052
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ A review job has been created and sent to the PullRequest network.
@PavelAplevich you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add tests to make sure this change works as expected?
Can you please rebase your branch to fix conflicts. It is a good practice to fetch and merge upstream/master before creating a new branch. |
I removed the conflict in the branches.If I do something wrong, please tell me. |
Thanks @PavelAplevich , git part seems fine now. But after your changes some tests fails:
Can you please review them? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests should pass
@neslihanturan I will try, but I still can not understand what the problem is. I need some time |
Take your time @PavelAplevich , I see some possible problems on the code now, I will submit a detailed review tomorrow, it should help. |
@neslihanturan I think I solved the problem. |
Codecov Report
@@ Coverage Diff @@
## master #3052 +/- ##
======================================
Coverage 4.41% 4.41%
======================================
Files 259 259
Lines 12309 12309
Branches 1056 1056
======================================
Hits 544 544
Misses 11726 11726
Partials 39 39
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my inline comment
|
||
authCookie = sessionManager.getAuthCookie(); | ||
mwApi.setAuthCookie(authCookie); | ||
|
||
Calendar calendar = Calendar.getInstance(); | ||
String fileDeleteString = "{{delete|reason=" + reason + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @PavelAplevich , I don't think this should go to strings.xml. In string xml there shouldn't be any code (or markup language) like content. Strings are what translators will see and they should include just natural language expressions. So please revert any changes which does not fit this description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on neslihanturan's suggestion. Additionally, you might want to use string formatter to combine code like template and replaceable content for easier to read code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these strings will ever be translated please follow the pattern outlined here:
https://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling
Reviewed with ❤️ by PullRequest
String editToken; | ||
String authCookie; | ||
String summary = "Nominating " + media.getFilename() + " for deletion."; | ||
String summary = context.getString(R.string.delete_helper_delete_summary_1) | ||
+ media.getFilename() + context.getString(R.string.delete_helper_delete_summary_2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For strings with placeholders, it would be cleaner (and safer) to use the built-in functionality described here: https://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling
When translating to other languages, concatenating strings this way won't always result in the desired output. This is where the filename would go in english, but grammar in other languages may put it somewhere else in the sentence. This applies to all of the below strings which you separated and concatenated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the minor suggestion improvement, the change looks good. Good job on pulling the string constants out!
Reviewed with ❤️ by PullRequest
I hope this time everything is done well) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minors:)
@@ -71,7 +72,7 @@ public DeleteHelper(MediaWikiApi mwApi, | |||
* @param reason | |||
* @return | |||
*/ | |||
private boolean delete(Media media, String reason) { | |||
private boolean delete(Context context, Media media, String reason) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think context is not used in this method anymore, so please keep the method as it was previously
@@ -91,12 +92,12 @@ private boolean delete(Media media, String reason) { | |||
reason + | |||
" ~~~~"; | |||
|
|||
String logPageString = "\n{{Commons:Deletion requests/" + media.getFilename() + | |||
String logPageString = "\n" + "{{Commons:Deletion requests" + media.getFilename() + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you forgot removing "+" from your previous implementation. This method shouldn't be edited at all if I don't miss any point.
} else { | ||
title += ": Failed"; | ||
message = "Could not request deletion."; | ||
title += context.getString(R.string.delete_helper_show_deletion_notification_title_else); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this string should be just "Failed" and operation shuld be ": "+[string]
app/src/main/res/values/strings.xml
Outdated
<string name="delete_helper_ask_reason_and_execute_reason_copyright_internet_photo">Random photo from internet</string> | ||
<string name="delete_helper_ask_reason_and_execute_reason_copyright_logo">Logo</string> | ||
<string name="delete_helper_ask_reason_and_execute_reason_copyright_other">Other</string> | ||
<string name="delete_helper_ask_reason_and_execute_alert_set_positive_button">Ok</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure we have Ok and Cancel strings in our file already. Please do not add them again, it is duplicate.
app/src/main/res/values/strings.xml
Outdated
<string name="delete_helper_ask_reason_and_execute_reason_spam_nonsense">Nonsense</string> | ||
<string name="delete_helper_ask_reason_and_execute_reason_spam_other">Other</string> | ||
<string name="delete_helper_ask_reason_and_execute_reason_copyright_press_photo">Press photo</string> | ||
<string name="delete_helper_ask_reason_and_execute_reason_copyright_internet_photo">Random photo from internet</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous string naming seemed better to me. This ones are too long
"decision problem with color of info icon #2940" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I create copy icon with right color, because drawabletint dont work with API lower 23
I dont undestand, how i can delete my comments and how i can comment what i want. In "Correction DeleteHelper and string.xml" I correcting hardcoded string. In "decision problem with color of info icon #2940" I tried to fix issue #2940 . But i do it in this pullrequest( I found #2660 issue. Think, i solved this issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ A review job has been created and sent to the PullRequest network.
@PavelAplevich you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minors
@@ -0,0 +1,9 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this file is edited?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant
app/src/main/res/values/strings.xml
Outdated
<string name="delete_helper_delete_summary_2">for deletion.</string> | ||
<string name="delete_helper_show_deletion_title">Nominating for Deletion</string> | ||
<string name="delete_helper_show_deletion_title_if">Success</string> | ||
<string name="delete_helper_show_deletion_message_if_1">Successfully nominated</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string naming is not clear. Additionally, you can insert variables into one string instead of using 2 strings. See the example at line 561:
%1$s is uploaded by: %2$s
app/src/main/res/values/strings.xml
Outdated
<string name="delete_helper_delete_summary_1">Nominating</string> | ||
<string name="delete_helper_delete_summary_2">for deletion.</string> | ||
<string name="delete_helper_show_deletion_title">Nominating for Deletion</string> | ||
<string name="delete_helper_show_deletion_title_if">Success</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable naming is nothing about success
@@ -25,7 +25,7 @@ | |||
android:layout_gravity="center_horizontal" | |||
android:paddingLeft="12dp" | |||
android:paddingRight="12dp" | |||
android:drawableEnd="@drawable/ic_info_outline_24dp" | |||
android:drawableEnd="@drawable/ic_info_outline_blue_24dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be edited
title += ": Failed"; | ||
message = "Could not request deletion."; | ||
title += ": " + context.getString(R.string.delete_helper_show_deletion_title_else); | ||
message = context.getString(R.string.delete_helper_show_deletion_message_else) + "."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dot is part of natural language, so can be added string itself.
title += ": " + context.getString(R.string.delete_helper_show_deletion_title_if); | ||
message = context.getString(R.string.delete_helper_show_deletion_message_if_1)+ " " | ||
+ media.getDisplayTitle() + " " | ||
+ context.getString(R.string.delete_helper_show_deletion_message_if_2) + "."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dots can be added to string.
"}}\n"; | ||
SimpleDateFormat sdf = new SimpleDateFormat("yyyy/MM/dd", Locale.getDefault()); | ||
String date = sdf.format(calendar.getTime()); | ||
|
||
String userPageString = "\n{{subst:idw|" + media.getFilename() + | ||
String userPageString = "\n" + "{{subst:idw|" + media.getFilename() + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This liine shouldn't be edited.
@@ -91,12 +92,12 @@ private boolean delete(Media media, String reason) { | |||
reason + | |||
" ~~~~"; | |||
|
|||
String logPageString = "\n{{Commons:Deletion requests/" + media.getFilename() + | |||
String logPageString = "\n{{Commons:Deletion requests" + media.getFilename() + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line shouldn't be edited.
@@ -59,7 +59,8 @@ public DeleteHelper(MediaWikiApi mwApi, | |||
* @return | |||
*/ | |||
public Single<Boolean> makeDeletion(Context context, Media media, String reason) { | |||
viewUtil.showShortToast(context, "Trying to nominate " + media.getDisplayTitle() + " for deletion"); | |||
viewUtil.showShortToast(context, context.getString(R.string.delete_helper_make_deletion_toast_1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use only one string here, and get media.getDisplayTitle() as a variable into it. For more info see my comment at strings.xml file.
@neslihanturan I dont undestand why i dont see problems in this files in my commit. Only added 1 file and change 1 line. Or #2940 already solved? |
@neslihanturan Accept my apologies for my carelessness. I hope this time my changes in DeleteHelper and string.xml right |
This reverts commit fe4ea52.
@@ -0,0 +1,9 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Irrelevant
Sorry @PavelAplevich but some of the irrelevant changes are still there. I noted it to the file. You can always click on files changed tab and see differences your PR made. Other parts seems correct to me, thanks for your fast responses and cooperation:) |
This reverts commit 5895cc8.
I revert my commit. I think we can do #2940 issue in another pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PavelAplevich !
Hello, i did small changes for you.
Removed all hardcoded strings in DeleteHelper and refractor they ID fo better navigation in Strings.xml
Tested on my Redmi4x and Nexus One(emul) API 25 and 22. Everything works correctly.