[FEATURE REQUEST] Maintain scroll position in file list #4570
Conversation
JuancaG05
changed the title
There was a problem hiding this comment.
Hi @goonerDroid! Thanks a lot for your contribution! 🍻
Here I left some comments for you to review 😀, let me know if you have any doubt.
| @@ -0,0 +1,6 @@ | |||
| Feature: Maintain scroll position when user scrolls and navigates in file/folder list | |||
There was a problem hiding this comment.
Feature is not a valid prefix for Calens. Check the Calens README.md to see the valid prefixes, I'd say in this case the best one is Enhancement
| @@ -0,0 +1,6 @@ | |||
| Feature: Maintain scroll position when user scrolls and navigates in file/folder list | |||
There was a problem hiding this comment.
There is also a limit of characters length for the title, so I would just write the way you named the PR: Maintain scroll position in file list
| saveScrollPosition() | ||
|
|
||
| fileListAdapter.updateFileList( | ||
| filesToAdd = fileListUiState.folderContent, | ||
| fileListOption = fileListUiState.fileListOption, | ||
| ) | ||
|
|
||
| restoreScrollPosition() | ||
|
|
There was a problem hiding this comment.
Hmmm I don't know if I like this to be here... there are 2 main points that make me doubt here:
- The observer where this is located. This is going to be executed everytime
fileListUiStateis updated, and that is acombinewith 5 different variables: the current folder displayed, the file list option, the search filter, the sort type and order and the current space. Everytime one of these is updated, the scroll position is going to be saved (and the previous one restored), so it looks a bit inefficient (and a potential source of glitches) - Seems we're saving just 1 scroll position, but what if we go deep into a several-level folder hierarchy? Would we be able to restore every scroll position of each level?
| @@ -94,6 +94,7 @@ class MainFileListViewModel( | |||
| fileListOptionParam: FileListOption, | |||
| ) : ViewModel() { | |||
|
|
|||
| var lastVisibleItemPositions: IntArray? = null | |||
There was a problem hiding this comment.
It's usually less secure to expose variables out of the ViewModel as in this case... but let's see how things go after resolving the previous comments 😉
|
Hi @goonerDroid! Are you willing to work further on this? It seems a bit blocked, let us know for planning purpose 😁, thank you! |
|
Hi @JuancaG05 I was quite swamped with other work, I've picked this issue again and I am still looking into it. Trying to incorporate your recent comments and will try submitting a reworked solution soon |
|
Ok! No worries and no hurries, just knowing you didn't abandon is good 😀 |
|
Hi @goonerDroid! How are the changes going? Are you still interested in working on this feature/pull request? If you have doubts, let us know and we will help you! 😄 |
Related Issues
App: #4528
QA