Closed parneet-guraya closed 3 months ago
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 ->
isLocallyAvailable
and I put it in the app
module's usecases package as I believe it is closely related to UI.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.
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:
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 🤠).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.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! 💪
- 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
orFileActivity
), and it is well thought, but if you realise, we also have aFileOperationsViewModel
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 theowncloudDomain
module (domain layer). It's true that we have some use cases inowncloudApp
, but these are exceptional cases, and all of them have one thing in common: they have dependencies toWorkManager
, 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 toowncloudDomain
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 theelse
. As I stated, that snippet is not tested but the idea would be something like that 🙂.
Thankyou for clearing my doubts :+1: @JuancaG05
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.
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.
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 😄
Hi @parneet-guraya! Will you continue the work started here? Just to take it into account in our planning or not 😃
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.
Hi @JuancaG05, :wave: 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?
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.
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? 😃
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 :-) .
hi @JuancaG05 :wave: , 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 -->
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?
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
👍.
Hi @JuancaG05 :wave: , 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.
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 😸
@JuancaG05, I'm not seeing any option to request review. Anyways it's ready for it.
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 😁
@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?
Aaah, could be! Ok, no worries, I'll review it whenever I'm available 😁
I see you finally used
SharedFlow
as I recommended, but I can see that we sometimes useStateFlow
withEvent
, just as you said that could work withLiveData
to avoid processing success more than once. If you feel more comfortable with this or you don't feel sure about usingSharedFlow
, you can use it, just as we do indeepLinkFlow
👍
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 Done :+1:
On it...
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
Fixes: #3936
ReleaseNotesViewModel.kt
creating a newReleaseNote()
with String resources (if required)https://github.com/owncloud/android/assets/111801812/cc69bac6-7988-4652-b667-1339c4baaa8e