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

fix: Local folders to be secured can be chosen again. #2559

Merged
merged 4 commits into from
Mar 29, 2019
Merged

fix: Local folders to be secured can be chosen again. #2559

merged 4 commits into from
Mar 29, 2019

Conversation

yashk2000
Copy link
Member

Fixed #2368

Changes: Now local folders can be secured again without switching off security. Also now if security on local folders is turned off, the folders which were previously secured are remembered. When security is switched on later, those folders are already checked.

Screenshots of the change:

screencap
screencap1
screencap2

@theabhishekavi
Copy link
Contributor

@yashk2000 According to my opinion, instead of getting the secured folder list everytime in recyclerview(which is a bad practice) ,we can get the secured folder list just before creating the recyclerview and we can make the albums secured even before inflating the view. So that when recyclerview is created ,the checkbox corresponding to the secured album gets marked automatically. In that way we don't need to call secured folder list everytime in recyclerview.

@yashk2000
Copy link
Member Author

@theabhishekavi I agree, it is a better way. I will update my pull request with the changes.

@theabhishekavi
Copy link
Contributor

@yashk2000 one more thing that i want to say, we can get the history of previously locked folders in just 3-4 lines of code. All the codes are so well written that we can easily implement new features... So we don't need much changes in code. And instead of creating an option to choose folders we can create an onclicklisteners on secured folders itself which will create the same dialog box. So we dont need an extra folder to be created.

@theabhishekavi
Copy link
Contributor

And there is one more fault in that section is that when we directly toggle off the button. Secured folder is disable but in shared preferences the details of previously locked folder is still there which can creates an issue in other activities. So remember to clear the data from sharedpreferneces also if we toggle off the button. Heartily welcome for any more suggestion if you want. :-)

@yashk2000
Copy link
Member Author

And there is one more fault in that section is that when we directly toggle off the button. Secured folder is disable but in shared preferences the details of previously locked folder is still there which can creates an issue in other activities. So remember to clear the data from sharedpreferneces also if we toggle off the button. Heartily welcome for any more suggestion if you want. :-)

I did not clear the data from special preferences deliberately. I think it will be better to remember which folders the user secured the last time. If the user will be setting security again, most probably the user will set security on those folders again in addition to any more folder if needed.

@yashk2000
Copy link
Member Author

@yashk2000 one more thing that i want to say, we can get the history of previously locked folders in just 3-4 lines of code. All the codes are so well written that we can easily implement new features... So we don't need much changes in code. And instead of creating an option to choose folders we can create an onclicklisteners on secured folders itself which will create the same dialog box. So we dont need an extra folder to be created.

I did not do this as the user might just want to turn off security on local folders. It is better to have a separate choose folder textview to allow the user to choose folders again.

@theabhishekavi
Copy link
Contributor

yes,you are right don't clear the data from sharedpreferences. I think i was talking about some other activity which is not needed there. You should just get secured folder list from shared preferences and set album.setsecured(true) in case of secured albums. And do this implementation before recyclerview as said earlier.

@yashk2000
Copy link
Member Author

Yes. I did it. I'll push it soon.

@theabhishekavi
Copy link
Contributor

And can we make securedfolder itself to be clickable if password on folder is on?

@yashk2000
Copy link
Member Author

yashk2000 commented Feb 15, 2019

That is a separate issue since even now when the dialog pops up, the securedfolder is not clickable. I'll open a separate issue for it and work on it.

@theabhishekavi
Copy link
Contributor

So instead of making a new folder can we make securedfolder itself be clickable to open the dialog box list of securedfolder?

@yashk2000
Copy link
Member Author

So instead of making a new folder can we make securedfolder itself be clickable to open the dialog box list of securedfolder?

Can you please elaborate this a bit, I did not understand this?

@yashk2000
Copy link
Member Author

Also I made the changes to this pr. Is it fine now?

@yashk2000
Copy link
Member Author

Do you mean that we should make "Local Folders" itself clickable if security is on? and show the dialog if it is clicked?

@theabhishekavi
Copy link
Contributor

Yes,exactly. And in that way we don't need an extra choose folders.

@theabhishekavi
Copy link
Contributor

Also I made the changes to this pr. Is it fine now?

Now the code looks good to me, but still i think instead of creating a new textview,you should make Local Folder itself be clickable if security is on.

@theabhishekavi
Copy link
Contributor

Now the code looks good to me, but still i think instead of creating a new textview,you should make Local Folder itself be clickable if security is on.

@abishekvashok what do you think?

@yashk2000
Copy link
Member Author

Also I made the changes to this pr. Is it fine now?

Now the code looks good to me, but still i think instead of creating a new textview,you should make Local Folder itself be clickable if security is on.

No that shouldn't be done. We should leave it so that user can easily toggle the button on/off. Making a new textview makes it easier for the user to understand what is to be done to secure more folders.

@theabhishekavi
Copy link
Contributor

Okay.

@fossasia fossasia deleted a comment Feb 16, 2019
@yashk2000
Copy link
Member Author

@abishekvashok @sauravvishal8797 please review.

@sauravvishal8797 sauravvishal8797 merged commit ed994fb into fossasia:development Mar 29, 2019
pull bot pushed a commit to sahilsaha7773/phimpme-android that referenced this pull request Jul 18, 2019
* Folders to be secured can be choosen again.

* removed unused imports
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

4 participants