Code Quality: Fixed ArgumentNullException in ShellViewModel.CheckForBackgroundImage() #15796
Conversation
| public void CheckForBackgroundImage() | ||
| { | ||
| var filesAppSection = DesktopIni.FirstOrDefault(x => x.SectionName == "FilesApp"); | ||
| var filesAppSection = DesktopIni?.FirstOrDefault(x => x.SectionName == "FilesApp"); |
There was a problem hiding this comment.
@0x5bfa I found that LayoutModeChangeRequested() might call this method before desktop.ini was loaded, so I added a null-safe operator to ignore the call.
There was a problem hiding this comment.
Should we add the loader inside this method? This doesn't make much sense since even if ini file exists you'll have to reload until it's loaded
There was a problem hiding this comment.
EnumerateItemsFromStandardFolderAsync should always load desktop.ini when the folder is successfully loaded, so I don't think it is necessary to load it every time.
There was a problem hiding this comment.
Correct me if I'm wrong
once this desktop background check method called ahead of time, the background is still null. So unless you don't refresh and pray that desktop ini data is loaded first, the background won't be loaded.
There was a problem hiding this comment.
CheckForBackgroundImage is called without loaded desktop.ini if LayoutModeChangeRequested is called before the folder load is completed or after the folder load fails. But the former will always call GetDesktopIniFileData and CheckForBackgroundImage in EnumerateItemsFromStandardFolderAsync, and the latter is an error state and can be ignored.
There was a problem hiding this comment.
Dang. Should we set a setter to call CheckForBackgroundImage in the property?
There was a problem hiding this comment.
That's one way. I don't have a preference.
There was a problem hiding this comment.
If the Folder background has a fade in transition, then you can load the folder view, and once the contents are done loading, you can then check and fade in the folder image, if there is an ini file. Lazy load essentially.
|
|
||
| await OrderFilesAndFoldersAsync(); | ||
| await ApplyFilesAndFoldersChangesAsync(); | ||
| await dispatcherQueue.EnqueueOrInvokeAsync(CheckForSolutionFile, Microsoft.UI.Dispatching.DispatcherQueuePriority.Low); |
There was a problem hiding this comment.
CheckForSolutionFile is not related to GetDesktopIniFileData and CheckForBackgroundImage, so I separated the queuing.
yaira2
added
ready to merge
yaira2
changed the title
4 participants
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.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.