[FEATURE REQUEST] Folder with "Local only" remove option

[parneet-guraya]

Fixes: #3936

• Add changelog files for the fixed issues in folder changelog/unreleased. More info here
• Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

Record_2024-01-26-09-45-11.mp4

[parneet-guraya]

Hey @JuancaG05, I did implement the recursive logic to check for local files. However, I had a question before going further. Could you give it a quick high level look to my logic below and let me know If I'm going right?

Here's how the flow looks ->

• I created a UseCase that recursively check for isLocallyAvailable and I put it in the app module's usecases package as I believe it is closely related to UI.
• To launch a remove dialog, we first execute the usecase and use the result to launch the dialog.

I wanted to put the common logic of launching a usecase in a common place and I believe FileActivity is the right place however it's in java so tell me If I should migrate it? (right now I temporarily kept the logic for demo in DrawerActivity as it's a direct parent to FileActivity).

These question involves architectural decisions so needed an advice. Also, If whole approach isn't the correct approach let me know.

[JuancaG05]

Hi @parneet-guraya! I'm happy that you asked so that we can see if it is in the correct path before going further 😃. Some things to consider here:

• I think this is not needed for single files, because the case that a single file is available locally is already managed. Thus, you don't need to add this to the previews activities/fragments and to the details fragment.
• The idea of using use cases is that they are called from ViewModels, as far as we are able to. In this case, you thought about adding this method to a common superclass (DrawerActivity or FileActivity), and it is well thought, but if you realise, we also have a FileOperationsViewModel which is used from all these places as well (although we don't need to use this new feature for single files as I mentioned in the previous point, but if we did, this would be also the right place). This is the suitable place to add a new method and call the use case from there by using observables (we're using Flow in new added stuff).
• FileActivity is in Java yet, yes, it deserves a nice refactoring to move it to Kotlin 😀, but it's such a big class and one of the main ones, so it should be done in an independent issue because it implies lots of changes and a deep testing to check that everything is working as it did before the refactor. But in these cases, if you need to add something to the class, don't be afraid to do it. Java is not as nice as Kotlin, but we still can do most of the things we do with Kotlin (don't forget Kotlin in Java under the hood 🤠).
• By definition, the use cases don't belong to the owncloudApp module (presentation layer), but to the owncloudDomain module (domain layer). It's true that we have some use cases in owncloudApp, but these are exceptional cases, and all of them have one thing in common: they have dependencies to WorkManager, which needs the context of the app and that we can only find on that module. But in this case, the use case can be moved safely to owncloudDomain module.
• About the recursive algorithm, we could improve it a bit to avoid going through all the files hierarchy if possible. We can check first if any of the is available locally, and in such case, return true with no more checks, and in any other case, take just the folders and do the recursive check. Something like this (not tested):

private fun getIfFileFolderLocal(listOfFiles: List<OCFile>): Boolean {
        var thereIsLocal = false
        if (listOfFiles.any { it.isAvailableLocally }) {
            thereIsLocal = true
        } else {
            val justFolders = listOfFiles.filter { it.isFolder }
            thereIsLocal = getIfFileFolderLocal(justFolders)
        }
        return thereIsLocal

In recursivity, there is a basic case and a recursive case. The basic case has to return something, and the recursive case just iterate again over the same function. In our case, the basic case would be the if and the recursive case the else. As I stated, that snippet is not tested but the idea would be something like that 🙂.

I hope these points could help you. Keep the good work! 💪

[parneet-guraya]

• The idea of using use cases is that they are called from ViewModels, as far as we are able to. In this case, you thought about adding this method to a common superclass (DrawerActivity or FileActivity), and it is well thought, but if you realise, we also have a FileOperationsViewModel which is used from all these places as well (although we don't need to use this new feature for single files as I mentioned in the previous point, but if we did, this would be also the right place).

Initially I added the usecase in FileOperationsViewModel itself. One reason to change that I was having doubts since the file name indicates doing operations on the file and other was obviously to make it available for single files which you cleared that does not need.

• FileActivity is in Java yet, yes, it deserves a nice refactoring to move it to Kotlin 😀, but it's such a big class and one of the main ones, so it should be done in an independent issue because it implies lots of changes and a deep testing to check that everything is working as it did before the refactor. But in these cases, if you need to add something to the class, don't be afraid to do it. Java is not as nice as Kotlin, but we still can do most of the things we do with Kotlin (don't forget Kotlin in Java under the hood 🤠).

Didn't need to use the file. Will definitely do the migration if ever touch this file in future and yes I'm also comfortable with java.

• By definition, the use cases don't belong to the owncloudApp module (presentation layer), but to the owncloudDomain module (domain layer). It's true that we have some use cases in owncloudApp, but these are exceptional cases, and all of them have one thing in common: they have dependencies to WorkManager, which needs the context of the app and that we can only find on that module. But in this case, the use case can be moved safely to owncloudDomain module.

Again I originally added the usecase in domain layer and yes you're right business logic should be the part of domain layer. It's just that I saw the usecases in the UI layer as well and thought maybe I can add this here as it is closely related to UI and not being used elsewhere. But, as you said those usecases were exceptions :-)

In recursivity, there is a basic case and a recursive case. The basic case has to return something, and the recursive case just iterate again over the same function. In our case, the basic case would be the if and the recursive case the else. As I stated, that snippet is not tested but the idea would be something like that 🙂.

Thankyou for clearing my doubts 👍 @JuancaG05

[parneet-guraya]

call the use case from there by using observables (we're using Flow in new added stuff).

Let me see If I can explain it. For one shot operations like this let's see which one is more suitable SharedFlow vs StateFlow
-->

• SharedFlow can emit repeated values. Meaning It will emit every state (loading,success,error) even if the value repeats (remains same) which is important in this case because there can be a case where the selected files aren't changed and user can dismiss the dialog and launch it again.

  On the other hand StateFlow ignores the repeated value meaning it won't emit anything and we wouldn't see any dialog
  which is not idle here.

• SharedFlow does not have any state so there might be a chance that we loose the emission if lifecycle state changes while usecase is still processing since repeatOnLifecycle cancels the coroutines that is collecting and we loose the value.

  Here StateFlow have edge because it retains state but when UI goes through lifecyle change and collecting coroutines
  launches again. It emits the last value it had which means we might show a unintended dialog and if user hasn't dismissed the
  previous one we might see multiple dialogs.

However we can set SharedFlow's parameter replay=1. So that when collector reattach it will emit the last value once and we can avoid handling event that is already handled by having a solution like Event.kt which is we have for LiveData to filter out events that are handled already.

Another solution I have seen being used is Coroutines Channels. I have not used this myself but I have read that it also has the chances of value getting lost in some cases.

There is an interesting article (from the guy who is part of coroutines team) talking about how SharedFlow and Channels is used for one shot operation for events but have some cons and there's a better approach by modeling the result from usecase as a UI state. Something like this -->

data class RemoveDialogUIState( val isLoading: Boolean = false,
    val isError: Boolean = false,
    val errorMessage: String? = null,
    val data: T? = null)

Right now I just pushed the code with LiveData and it works fine. I think we can go forward with LiveData for this one and I can migrate from using LiveData to better solution for the all the file operations in the FileOperationsViewModel either by modeling result into UI state or some other solution. Do let me know what your thoughts are.

[JuancaG05]

Initially I added the usecase in FileOperationsViewModel itself. One reason to change that I was having doubts since the file name indicates doing operations on the file and other was obviously to make it available for single files which you cleared that does not need.

Well, you can see that there are some operations in that VM that take care of the deep link, setting last usage in the file, etc, so it's not only that you do operations with the file but also related to it 😄 like this one. But in case you wanted to make it available for single files, this ViewModel is already accessible from every preview.

Right now I just pushed the code with LiveData and it works fine. I think we can go forward with LiveData for this one and I can migrate from using LiveData to better solution for the all the file operations in the FileOperationsViewModel either by modeling result into UI state or some other solution. Do let me know what your thoughts are.

Let's use Flow better, there is no much complexity in adding it. Why not using SharedFlow?
But I think it would be easier if the use case just returns a boolean, not a Pair with the files we send by parameter and a boolean indicating whether they have any descendants available locally.

[JuancaG05]

Also, this will be merged or not depending on performance. It implies several accesses to DB in the worst case (very deep hierarchy and none of the files is available offline, so we have to check all of them) to just show the dialog, so we'll see when it is in QA phase if this is really worth it or it is actually better to live without it despite not having the feature 😄

[JuancaG05]

Hi @parneet-guraya! Will you continue the work started here? Just to take it into account in our planning or not 😃

[parneet-guraya]

Hi, @JuancaG05 . I'm going to, just lagging behind with my other stuff. Will update this ASAP :-).

And yes I'm aware and thought the same about the case when folder hierarchy is deep. So let's see how it performs in QA.

[parneet-guraya]

Hi @JuancaG05, 👋 About returning Pair<List<OCFile>,Boolean>, So, the thing is when we fire off the remove operation. We first pass the listOfFIles to the usecase (which decides to show local-only options or not) and upon collection we launch the dialogFragment passing the list. So, I need the list when I get the result of usecase that's why I used Pair. I see it isn't the best thing to do but I couldn't think of any other way because we need the checked list (or selected item) to remove after we're finished with usecase. Do you have any idea/suggestion I can look into?

[JuancaG05]

Hey @parneet-guraya! I think the best option here is keeping those selected files in a variable in the fragment. Indeed, we already have one variable for that in MainFileListFragment for the same reason but for different purposes (in this case showing the correct options in the menu for the selected files), checkedFiles. You could check if this variable can be reused for this as well since menu options and remove option don't collide, or maybe there are some cases in which it's incompatible, needs check.
Also, two more arguments for doing this, are: firstly, understanding the code will be easier if the use case just returns a boolean and not a Pair (and simpler in code); and secondly, think about if we want to reuse this use case in another context, maybe we're not interested on having the list of files (we do in this case to instantiate the dialog, but it's specific for this case), maybe we just want to know if we have locally available files in the hierarchy we're sending as parameter, just as the name of the use case indicates. Summarizing, returning the list of files is just useful for this particular case but we shouldn't modify a use case (that we want to make reusable, always 😃) just because of that, so it's a presentation layer responsibility.

[JuancaG05]

Hey @parneet-guraya, this seems a bit blocked, do you want us to continue the work started here or will you finish it when you have some time? 😃

[parneet-guraya]

Hi @JuancaG05 👋, unfortunately due to some ongoing situation in our state. The internet is shut down temporarily by gov in some cities and one of them is mine. So that's why I couldn't get to work on this. I was in another city today and now I saw this comment. I will finish this as soon as I get a chance :-) .

[parneet-guraya]

hi @JuancaG05 👋 , I'm back :-). I can see the point about making usecases reusable, I will remove the usage of Pair in this case.

However, I need a little help in deciding what observable should we choose to reflect ui state for this one shot operation?
I'm concerned about the case when let's say the usecase is in loading state and at that time user goes to background. While in the background the operation finishes. Now since we're emitting values from a viewmodelscope which mean the coroutine won't cancel hence the success operation would have been emitted in the background. But the view wouldn't have collected it because the lifecyle api's would have cancelled the collecting coroutines. Hence, we missed our update. Now if we use -->

• StateFlow -> We get the latest value when coming back which is good but it also means if we go through config change it again would show the dialog.
• SharedFlow -> We do not get any value and we completely lost the state.

Livedata works similar to StateFlow which is upon coming back we would get latest value and the problem of consuming same success state again on config change already have a solution in place, usage of Event class to only handle event that are not consumed earlier using MediatorLiveData.

Could you help me out here in deciding what should be ideal Flow api to use in this case?

[JuancaG05]

Hi @parneet-guraya! Nice to hear from you again!

Could you help me out here in deciding what should be ideal Flow api to use in this case?

I would say using SharedFlow. User shouldn't go to background while use case is loading (showing a dialog for that, just like when the delete operation is executing), since it shouldn't take too much time. But if what you mean is that the user puts in background the app while loading, I think it's acceptable that when the user is back, if the operation succeeded in the background, there's no effect on the app. So it's like cancelling the operation: the dialog should not appear. You can get this with SharedFlow 👍.

[parneet-guraya]

Hi @JuancaG05 👋 , It's good to be back :-)

I used SharedFlow as you said. Also I created separate property to keep track of filesToRemove because if we choose to remove single file from bottom sheet that open when click on three dot button then checkedFiles are not updated so that's why separate property.

[JuancaG05]

Nice @parneet-guraya! Let me know then when it is ready for code review. Just request me a review in the PR and I will do 😸

[parneet-guraya]

@JuancaG05, I'm not seeing any option to request review. Anyways it's ready for it.

[JuancaG05]

It's in the upper right part of the PR, where it says "Reviewers". I'll request myself a review for you to see it 😁

[parneet-guraya]

@JuancaG05 , I'm aware of this :-) but I didn't see the options to choose a reviewer maybe because I don't have permission to do that?

[JuancaG05]

Aaah, could be! Ok, no worries, I'll review it whenever I'm available 😁

[JuancaG05]

Hey @parneet-guraya! Nice work here! I just left some comments for you to review, sorry for the delay 😄. Also, two more things:

• I see you finally used SharedFlow as I recommended, but I can see that we sometimes use StateFlow with Event, just as you said that could work with LiveData to avoid processing success more than once. If you feel more comfortable with this or you don't feel sure about using SharedFlow, you can use it, just as we do in deepLinkFlow 👍
• I modified the PR first message for you to take into account that, now, in addition to calens, we also add the release note in the corresponding ViewModel in each PR so that it can be translated to different languages before releasing. The check indicates where it is located, but if you have any doubt just ask me, shouldn't be difficult 😃

[parneet-guraya]

I see you finally used SharedFlow as I recommended, but I can see that we sometimes use StateFlow with Event, just as you said that could work with LiveData to avoid processing success more than once. If you feel more comfortable with this or you don't feel sure about using SharedFlow, you can use it, just as we do in deepLinkFlow 👍

Hi @JuancaG05 , you are correct with the fact that we can use Event to avoid duplicate updates with using StateFlow. But, I encountered a strange behavior that it only emits first update and subsequent updates were not getting collected. After a deep dive I came to conclusion that it is because the StateFlow does check for equality of every emission. So, If a subsequent emission turns out to be equal it won't be collected.

But, I also tried to put some delay before emitting success and then it starts working. So there might be more to it than just equality checks. I will investigate it deeper for future and maybe it solves if we are having some strange bugs with current operations that are using StateFlow.

But to keep this one going, for now we have these options to choose from -->

• use StateFlow and put the hasBeenHandledProperty of Event wrapper in the constructor so that this property is also considered in the automatic generation of equals() method by data class.
  data class Event<out T>(private val content: T, private var hasBeenHandled: Boolean = false)

• Use SharedFlow with replay=1 and onBufferOverflow strategy to be BufferOverflow.DROP_OLDEST this way it will behave exactly like StateFlow except the state holding part. Meaning, no emission comparisons, hence every emission is collected.

• Keep it as is. Which is using SharedFlow.

Now after this only key difference of behavior we will get is, with StateFlow we will get our emission even if user goes to background in case loading takes time. In case of SharedFlow we will just loose that emission.

PS: I pushed the requested changes and kept implementation to SharedFlow for now until decision.

[JuancaG05]

Hi @parneet-guraya! Just a tiny change here for the release note and we're done. All right, let's keep SharedFlow and we'll see if the behaviour is the expected in the QA phase 😁

[parneet-guraya]

@JuancaG05 Done 👍

[JuancaG05]

LGTM!! Great job @parneet-guraya!! 🚀 Let's move it to QA, where @jesmrec will do his stuff 😁

[jesmrec]

On it...

[jesmrec]

Feature works fine and is performant for deep and wide folder structures to look for downloaded files as "removables"

While checking this, i detected a bad behaviour that will be moved to another issue because is out of scope here: If there is av. offline stuff inside the folder, the Local only option is displayed (av. offline items are downloaded). By clicking that option, the local copy is removed, and recovered when browsing or when the worker runs. This is a behaviour to avoid, probably adding a new condition to the existing one for showing the Local only option. As commented, i will address it to another issue.

This one is approved on my side