-
Notifications
You must be signed in to change notification settings - Fork 73
feat: Made Directory Picker instead of edittext #251
base: master
Are you sure you want to change the base?
Conversation
43b1921
to
5e36e79
Compare
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.
Great work @Chromicle
LGTM
switch (requestCode) { | ||
case DIRECTORY_REQUEST_CODE: | ||
try { | ||
String filePath = (data.getData()).getPath(); |
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.
(data.getData()).getPath()
can be written as data.getData().getPath()
case DIRECTORY_REQUEST_CODE: | ||
try { | ||
String filePath = (data.getData()).getPath(); | ||
filePath = filePath + getString(R.string.directory_odk); |
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.
filePath = filePath + getString(R.string.directory_odk);
can be written as filePath = filePath + getString(R.string.directory_odk);
@@ -159,4 +155,25 @@ private void showPasswordDialog() { | |||
AlertDialog alertDialog = builder.create(); | |||
alertDialog.show(); | |||
} | |||
|
|||
public void chooseDirectory() { |
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.
@Chromicle Have you tried handling when there is no intent to handle directory picker?
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.
Yeah, I tried but it does not give better results alike as putting intent
But I checked in Android 6,7,8,9 phones whether the intent is working or not it is perfectly working
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 tried this in Android 4.4 version and I was not able to select any directory. Can you please verify or can you think of any fallback mechanism?
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.
Since there is no "default file manager" in KitKat it is not working if the user is installed a particular 3rd party file manager then it may works
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.
@Chromicle If we are adding some feature then we should be confident enough that it won't break anything which was working previously. So adding this feature will not work as well as won't allow user to manually write the directory path. So I would suggest you rather than printing log or toast it would be beneficial to have some fallback mechanism for some devices in which directory picker is not available.
One more suggestion I would like to give that whenever you're doing some kind of testing then always try to test on less than 5.
Also resolve conflicts. |
@Chromicle Are you still working on this, because I see that few of the above issues are still not resolved related to lower Android devices |
@lakshyagupta21 I resolved in the way that for android version less than KitKat they will get the edit text and android versions greater than that will get the directory picker |
@Chromicle I would recommend you to run
|
@Chromicle Please update your PR template. |
Yeah I updated can you please check it |
CheckBoxPreference passwordRequirePreference; | ||
EditTextPreference odkDestinationDirPreference; | ||
SwitchPreference passwordRequirePreference; | ||
Preference odkDestinationDirPreferenceDirectoryPicker; |
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.
Creating two different preferences is fine, but the values that we get from these two different preferences on different versions stored separately. Ideally, whatever value that we receive from these UI Objects should be saved to the same preference.
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 tried that but I am getting preference cast exception
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'll give it a shot then will suggest the solution
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.
Yeah than you
@Chromicle due to some recent commits, now this PR again have some merge conflicts can you resolve them, and do a dev testing like transfer a form from one device to another and check if you're able to see the entries in the selected directory or not. |
Directory Picker Fixed typos fixed Null pointer exception reformated code in xml added /ODK to filePath Changed method names Added /odk Deleted some unused changes Made code Checkstyle removed unused strings
3775bd5
to
e5cebb3
Compare
Closes #250
What has been done to verify that this works as intended?
Checked whether selected Directory is taking as a summary and file path value
Checked all other preferences whether they are working properly or not
Checked in some phones(Android 7,8,9) whether the intent is working properly or not
Checked whether it is taking sdCard or not along with internal storage
Checked whether it is working perfectly in lower android devices
Why is this the best possible solution? Were any other approaches considered?
As due to there is no default file manager in lesser android devices so the devices lesser than API 19
will get the editText for a directory path and greater android devices will get the directory picker so they directly can pick the whatever the folder they want
Implementation of Directory picker
I made Intent
Intent.ACTION_OPEN_DOCUMENT_TREE
so by clicking on that we can see all files in a deviceBy that, I added another method
onActivityResult
so we can get the file path through(data.getData()).getPath()
without any typosHow does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Mainly, It will avoid the typos regarding Directory names and user can directly select the Directory easily
GIF
Greater Android Devices
Lesser android Devices
Before submitting this PR, please make sure you have:
./gradlew checkCode
and confirmed all checks still pass OR confirm CircleCI build passes