Skip to content

Conversation

esibruti
Copy link
Contributor

@esibruti esibruti commented May 24, 2022

Resolved / Related Issues
Items resolved / related issues by this PR.

Details of Changes
Add details of changes here.

  • With this code, the Downloads folder will be grouped by date modified on the first start.

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

@esibruti esibruti changed the title download path Set downloads folder to group by date May 24, 2022
@gave92
Copy link
Member

gave92 commented May 24, 2022

We already have this logic in FolderSettingsViewModel 🤔

​            ​else​ ​if​ (folderPath​ ​==​ ​CommonPaths.DownloadsPath) 
​            { 
​                ​//​ Default for downloads folder is to group by date created 
​                ​return​ ​new​ ​LayoutPreferences 
​                { 
​                    ​...
​                    ​DirectoryGroupOption​ ​=​ ​GroupOption.DateCreated, 
​                }; 
​            }

Doing it in ItemsViewModel would stop the user from changing it to something else.

@esibruti
Copy link
Contributor Author

@gave92 I hadn't even noticed it 😅
So but that means that the issue was already resolved before... Now I'm confused.

@gave92
Copy link
Member

gave92 commented May 24, 2022

Lol confused as well as to why that issue exists 😆 Could be there's an issue preventing it to work. Problem is you only notice it if you've never ever changed the layout/sorting in Downloads folder. Reinstalling Files may not reset it so it's not easy to test.

@gave92
Copy link
Member

gave92 commented May 24, 2022

Aaah found the issue, indeed that logic does not work. That's because we're passing a wrong path to be compared with the downloads folder path: ReadLayoutPreferencesFromSettings(folderPath.TrimEnd('\\').Replace('\\', '_') Ln355 -> We're triyng to compare C:_Users_USER_Downloads to C:\Users\USER\Downloads.

@esibruti
Copy link
Contributor Author

esibruti commented May 24, 2022

Aaah found the issue, indeed that logic does not work. That's because we're passing a wrong path to be compared with the downloads folder path: ReadLayoutPreferencesFromSettings(folderPath.TrimEnd('\\').Replace('\\', '_') Ln355 -> We're triyng to compare C:_Users_USER_Downloads to C:\Users\USER\Downloads.

So the solution is to remove this replace or replace with '/'?

@gave92
Copy link
Member

gave92 commented May 24, 2022

So changing "\" with "_" is done for a reason, that is LocalSettings throw error if you try to use a key/setting name that contains "\". We need to separate "ReadLayoutPreferencesFromSettings" which reads from LocalSettings and should use the paths with "_" from getting the default layout part that should use the "normal" path.

@esibruti
Copy link
Contributor Author

But that means on line 355 it should be comparing with the '\' instead of the '_', right?

@esibruti esibruti marked this pull request as ready for review May 24, 2022 20:09
@esibruti
Copy link
Contributor Author

@gave92 When you can, do the review, I think I found the solution.

@esibruti esibruti requested a review from gave92 May 25, 2022 12:35
@gave92 gave92 added the ready to merge Pull requests that are approved and ready to merge label May 25, 2022
@yaira2 yaira2 merged commit a141e0d into files-community:main May 25, 2022
@esibruti esibruti deleted the issue6586 branch May 26, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set downloads folder to group by date
3 participants