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

Fix: Keep sort and group options consistent between tabs #16572

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ferrariofilippo
Copy link
Contributor

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
Panes_behaviour.mp4

@yaira2
Copy link
Member

yaira2 commented Dec 12, 2024

Would it be better to update the layout istantly?

How much of a performance difference is there?

@yaira2 yaira2 requested a review from 0x5bfa December 12, 2024 03:23
@ferrariofilippo
Copy link
Contributor Author

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

Partial_update.mp4
Complete_update.mp4

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

@yaira2
Copy link
Member

yaira2 commented Dec 13, 2024

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
Copy link
Contributor Author

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
Copy link
Member

0x5bfa commented Dec 13, 2024

Code wise, looks great otherwise.

ferrariofilippo and others added 2 commits December 13, 2024 19:09
Co-authored-by: Yair <39923744+yaira2@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Grouping and sorting options are not consistent between tabs/panes
3 participants