-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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: Fixed issue where changing a folder's grouping preference wouldn't update other open tabs #16572
base: main
Are you sure you want to change the base?
Fix: Fixed issue where changing a folder's grouping preference wouldn't update other open tabs #16572
Conversation
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. The real issue is with RAM usage: updating two panes uses twice the RAM. Partial_update.mp4Complete_update.mp4Should I push the code to update both the panes so that you can test by yourself? |
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 |
Code wise, looks great otherwise. |
Co-authored-by: Yair <[email protected]>
2c33a81
to
ee8a6ad
Compare
…errariofilippo/Files into bug_update_layout_between_tabs
As a side note, I found out there's a similar issue with layouts not being updated between panes... I remember it used to work, didn't it? |
if (string.IsNullOrWhiteSpace(path)) | ||
return; | ||
|
||
var preferencesItem = GetLayoutPreferencesForPath(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to reload the preferences, or can we simplify this further by just invoking the related events (eg GroupDirectionPreferenceUpdated
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot just invoke the event because the properties are not updated unless we reload them. I can add a parameter that tells which is the only one to update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW should we sync sorting preferences even when they are set clicking on the details view headers? If so I'll implement it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot just invoke the event because the properties are not updated unless we reload them.
I see, in that case I would just leave it as is.
BTW should we sync sorting preferences even when they are set clicking on the details view headers? If so I'll implement it
It might be best to wrap up this PR first, but we can look into this next.
This reverts commit dcafc4b.
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.
sort/group
option/order
in a tabSync preferences between folders
settingPanes_behaviour.mp4