search

files-community / Files

A modern file manager that helps users organize their files and folders.
https://files.community
MIT License
35.52k stars 2.26k forks source link

Fix: Fixed issue where changing a folder's grouping preference wouldn't update other open tabs #16572

Open ferrariofilippo opened 1 month ago

ferrariofilippo commented 1 month ago

Resolved / Related Issues

To prevent extra work, all changes to the Files codebase must link to an approved issue marked as Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.

Notes I'm not sure how to handle multiple panes: currently the layout is updated only when the pane is focused, to avoid wasting resources (see video below). Is this fine? Would it be better to update the layout istantly?

Steps used to test these changes

Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.

  1. Open two tabs with the same folder
  2. Change sort/group option/order in a tab
  3. Switch to the second tab
  4. Repeat using the Sync preferences between folders setting

https://github.com/user-attachments/assets/11a4b97d-7516-4cb7-803b-b926ffe2ec2c

yaira2 commented 1 month ago

Would it be better to update the layout istantly?

How much of a performance difference is there?

ferrariofilippo commented 1 month ago

How much of a performance difference is there?

As you may think, the issue involves mainly big folders: with small ones (~ 50 items) I saw no difference. I tested a folder with ~550 items on my laptop (i7-13650HX, 16GB): sometimes it got slightly laggy when updating both panes. I'll leave two recordings below (sped-up 2x).

The real issue is with RAM usage: updating two panes uses twice the RAM. In my case, updating the 550 items folder I reached a difference of ~350 MB after 7 updates (I guess we have a memory leak). Each update required more or less 50MB per pane. I imagine this would get worse using some layouts such as Grid

https://github.com/user-attachments/assets/33fefe2f-c1d5-4222-9e35-1573b0da82cf

https://github.com/user-attachments/assets/fa936939-a12e-4f73-8c93-258d83301b08

Should I push the code to update both the panes so that you can test by yourself?

yaira2 commented 1 month ago

I think it's important to update both panes, but we should make sure that the focus remains on the active pane. While memory usage is a valid concern, I don't think it's common for users to have the same folder open in both panes and then decide to change the grouping.

ferrariofilippo commented 1 month ago

I think it's important to update both panes, but we should make sure that the focus remains on the active pane. While memory usage is a valid concern, I don't think it's common for users to have the same folder open in both panes and then decide to change the grouping.

Focus is fine, it was me changing it

0x5bfa commented 1 month ago

Code wise, looks great otherwise.