Skip to content

Conversation

FelixZacher
Copy link
Contributor

So i think i'm ready.

Copy link
Contributor

@lukeblevins lukeblevins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but I noticed we encounter a crash on app startup when running in a debugger which only occurs if File System access is denied. I would like to see this fixed before we merge the changes into settings-vm just to keep track of everything. Regardless, the newly modified branch will spend more time in testing before I incorporate the changes in v0.7.1 . Thanks!

@lukeblevins
Copy link
Contributor

@lampenlampen Startup Crash info:
Windows.Graphics.Display: GetForCurrentView must be called on a thread that is associated with a CoreWindow.

@lukeblevins
Copy link
Contributor

I'm guessing it has to do with the consent dialog which displays on startup because this exception only occurs when running in a debugger and file system access is disabled.

@FelixZacher
Copy link
Contributor Author

So i can reproduce the crash, if the debugging option "Only my code" is enabled. If disabled, the crash does not occur.

@FelixZacher
Copy link
Contributor Author

A guess:
consentDialog.ShowAsync() is somewhere called, before the Application CoreWindow is created, thus it fails to display the dialog ui.
And somehow the debugger delays the creation of the CoreWindow.

@FelixZacher
Copy link
Contributor Author

I could fix it, by delaying the creation of the SettingsViewModel, until the constructor of App is called.

@lukeblevins
Copy link
Contributor

@lampenlampen I'm still encountering a crash for some reason on the latest changes

@FelixZacher
Copy link
Contributor Author

Yes there is a NullReferenceException at https://github.com/duke7553/files-uwp/blob/69902d4bb3e3f2d46b33c842057d2a982d66aaa5/Files/Filesystem/ListedItem.cs#L45
because datetimeformat is null at a fresh install.

We need to set a default value at startup, if it is null.
At https://github.com/duke7553/files-uwp/blob/69902d4bb3e3f2d46b33c842057d2a982d66aaa5/Files/View%20Models/SettingsViewModel.cs#L45
we should set a default value, if datetimeformat is null.

What should the default value be?

@yaira2
Copy link
Member

yaira2 commented Jan 15, 2020

The setting view model should have a default for every setting, the default for date and time format should be friendly date and times. For example "2 days ago".

@FelixZacher
Copy link
Contributor Author

So we can choose between Application and System. I set it now to System.

@yaira2
Copy link
Member

yaira2 commented Jan 15, 2020

I think Application is the one with friendly dates...

@FelixZacher
Copy link
Contributor Author

They are both with friendly dates.

Application `4 days ago`, `15 hours ago`, `Saturady, October 12, 2019`
System `4 days ago`, `15 hours ago`, `10/12/2019 5:14 PM`

@yaira2
Copy link
Member

yaira2 commented Jan 15, 2020

Ok, so lets set the default to application. That is how it was in previous versions of Files as well.

@yaira2
Copy link
Member

yaira2 commented Jan 15, 2020

Great, is this pull request ready for review now?

@FelixZacher
Copy link
Contributor Author

FelixZacher commented Jan 15, 2020

Yes fixed a last bug and should be ready now 👍.

@lukeblevins lukeblevins merged commit 620e7df into files-community:settings-vm Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants