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

feature/2331/saveFiles #3416

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Conversation

FaribaKhandani
Copy link

@FaribaKhandani FaribaKhandani commented Oct 30, 2023

resolves #2331

The feature of saving media in personal storage space

Access to feature
Access to this feature is possible through the menu in full screen media mode or long click on media.

process of saving
In this feature, if the media is already stored in the cache, it is copied to the download folder, and if the media is not available in the cache, it is first downloaded to the cache and then copied to the download folder.

Permitions
Due to the limitations of memory access, if there is no prior permission from the user's side, the user's permission will be asked first, and like other features in this app, the media will be saved in the next attempt.
According to the descriptions mentioned in the related issue, every time an attempt is made to save the media, a warning is sent to the user that other programs have access to the saved media.

TODO

  • [Physical device Test ] After fetch this project and running it through Android Studio on two types of physical device , the "Handshake" error occurred when logging into the account.
    For this reason, this feature has only been tested on virtual Android devices and needs to be tested by the QA team on a physical device.

@AndyScherzinger AndyScherzinger added enhancement New feature or request 3. to review Waiting for reviews feature: 🗨️ chat labels Oct 30, 2023
@mahibi
Copy link
Collaborator

mahibi commented Nov 2, 2023

Hi @FaribaKhandani,

thanks for your PR! With my test device it works as expected 👍

Regarding the handshake failed error: can you comment on #1578 by adding the devices and versions you used?

Regarding the code i have some minor remarks, i will just add some comments.
If you would like and have some time, feel free to solve my remarks. Otherwise just let me know if i should do it, that's totally fine.

@mahibi mahibi added this to the 18.0.0 milestone Nov 2, 2023
@SuppressLint("LongLogTag")
private fun saveImageToStorage(
fileName: String,
funToCallWhenDownloadSuccessful: (() -> Unit)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the second parameter (funToCallWhenDownloadSuccessful) is not used so it could also just be deleted.
So just call the method with saveImageToStorage(fileName) and delete the observer.

try {
for (workInfo in workers.get()) {
if (workInfo.state == WorkInfo.State.RUNNING || workInfo.state == WorkInfo.State.ENQUEUED) {
Log.d(TAG, "Download worker for $fileId is already running or scheduled")
Copy link
Collaborator

Choose a reason for hiding this comment

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

just replace "Download Worker" with SaveFileToStorageWorker, so the log message makes sense again.

}
}

fun showWarningDialog(message: ChatMessage) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe better find a name that tells a bit more about what this dialog does. For example "showSaveToStorageWarning"


val cacheFile = File(sourceFilePath)

val downloadDirectory = "storage/self/primary/Download"
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it would be better to use MediaStore to store the files.
https://developer.android.com/training/data-storage/shared

Log.d("amdd" , destFile.path)

if (destFile.exists()){
destFile.delete()
Copy link
Collaborator

Choose a reason for hiding this comment

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

deleting is not the best choice here i guess. Instead there could be number appended. But that's maybe out of scope for this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Hello

All of your remarks are fixed and pushed .
Media Store approach for saving file is added (previous way for saving files is commented within the code )

Please let me know what is the next step?

@mahibi mahibi force-pushed the feature/savefiles branch from d92a238 to fec0b0d Compare November 7, 2023 12:31
@mahibi mahibi force-pushed the feature/savefiles branch from fec0b0d to dd9eb35 Compare November 7, 2023 12:32
@mahibi mahibi merged commit 946eb84 into nextcloud:master Nov 7, 2023
11 of 16 checks passed
@mahibi
Copy link
Collaborator

mahibi commented Nov 7, 2023

Hi Fariba,

thanks again for the PR and also thanks to solve the remarks.
I did some more modifications, basically:

  • remove some unnecessary params for saveImageToStorage
  • make some methods private
  • do some code formatting

Please feel free to ask if any of my changes are unclear.

@mahibi mahibi mentioned this pull request Nov 8, 2023
5 tasks
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

save shared files
3 participants