search

files-community / Files

Building the best file manager for Windows
https://files.community
MIT License
33.95k stars 2.17k forks source link

Feature: Display error message when settings file isn't valid #710

Open yaira2 opened 4 years ago

yaira2 commented 4 years ago

If the json file isn't valid, we should notify the user of the issue.

Requirements

tsvietOK commented 4 years ago

As i told in #562 we should unify all exceptions(replace JsonSerializationException with Exception or add JsonReaderException), so if there will be any json parsing error, just reset this json to default.

lampenlampen commented 4 years ago

But if we reset the json file, the user loses all his profiles and we will have to store a default json file. If the terminal loading process goes wrong for whatever reason, i would simply default to cmd and log, that there was an error. So the user can investigate his json file, if he want's to fix it and if not, he gets the cmd terminal.

yaira2 commented 4 years ago

That would be a nice addition, however it would still make sense to include a json file for someone who wants to use the json format.

lampenlampen commented 4 years ago

Maybe we should get rid of the json file, especially if we auto-detect the common terminals (powershell (core), WindowsTerminal).

And for the few users, who want's to use a custom one, they can choose custom and then we present them an input box to specify the path.

I choosed json, because i thought then they can specify multiple terminals and icons and so on, but i don't think we will use that customization. If a user want's control over multiple terminal, he will install Windows Terminal or terminus, ...

yaira2 commented 2 years ago

Maybe we should get rid of the json file, especially if we auto-detect the common terminals (powershell (core), WindowsTerminal).

And for the few users, who want's to use a custom one, they can choose custom and then we present them an input box to specify the path.

I choosed json, because i thought then they can specify multiple terminals and icons and so on, but i don't think we will use that customization. If a user want's control over multiple terminal, he will install Windows Terminal or terminus, ...

@lampenlampen that is a possibility, and once support is added for importing and exporting the settings json file, users can technically add a custom terminal path even it's not detected automatically by Files.

d2dyno1 commented 2 years ago

It's not been resolved yet. I'm working on this. Simple try-catch should be enough.

hecksmosis commented 1 year ago

I think this issue should be modified to not conflict with #10935, and leaving this as just the error modal and reset

yaira2 commented 1 year ago

I updated the requirements as per @hecksmosis feedback.

ClockWorkElementals commented 1 year ago

Hey all! If no one's currently working on this, I want a go at implementing this feature! It'll be my first attempt at contributing to an open-source project, so I'm pretty excited honestly! -M

Josh65-2201 commented 1 year ago

Sure go ahead

d2dyno1 commented 1 year ago

@ClockWorkElementals I assigned you 🙂

HorseBandit commented 1 year ago

Heads up that I am looking into this.

ClockWorkElementals commented 1 year ago

Heads up that I am looking into this.

If Bandit's taking a shot, feel free to give it to em! I couldn't quite figure my way around everything while taking my shot

zackatack108 commented 10 months ago

What's the status of this issue?

HorseBandit commented 10 months ago

Available for work

Get Outlook for iOShttps://aka.ms/o0ukef

From: zackatack108 @.> Sent: Wednesday, November 8, 2023 12:11:39 PM To: files-community/Files @.> Cc: Estell, Jerrod Victor @.>; Comment @.> Subject: Re: [files-community/Files] Feature: Display error message when settings file isn't valid (#710)

[EXTERNAL EMAIL]

What's the status of this issue?

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/files-community/Files/issues/710*issuecomment-1802578161__;Iw!!JmPEgBY0HMszNaDT!oDmUjHFn6i4OFuPxQzgY6yUGIpgByVeCcOk8PgXRCWfVYsSf7luCyZXTLiR9Tvx4taBWc59o6sqGN6krf0kbCbKh8bxL$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/A35NWDLJXEXBAS3FZKOGESDYDPRPXAVCNFSM4MSAOJVKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBQGI2TOOBRGYYQ__;!!JmPEgBY0HMszNaDT!oDmUjHFn6i4OFuPxQzgY6yUGIpgByVeCcOk8PgXRCWfVYsSf7luCyZXTLiR9Tvx4taBWc59o6sqGN6krf0kbCS6bh5EC$. You are receiving this because you commented.Message ID: @.***>

yaira2 commented 10 months ago

It hasn't been implemented yet

RieBi commented 10 months ago

I might implement this feature in a week or something but I'm not sure if I will get it working. I've tried doing something with it today but to no avail. There's so much stuff involved with creating a new dialog and making it work. I'll try to do a research on this one and come up with a solution.

If I'm not done within a week or so then it would probably mean that I stopped working on the issue and it's free for taking.

0x5bfa commented 9 months ago

Concept from Windows Terminal: image

0x5bfa commented 5 months ago

I was working on this and make it work correctly but current implementation of json serializer for settings is not intended to display UI from their own classes and those classes are exposed as abstract classes, which means I should've add an event to notify of the parse error to AppModel after UI is loaded to prevent the dialog from having to be tried to show before UI thread starts.

@mdtauk Do you have design concept or have something to tell me before working on again, such as adding what buttons and displaying what messages?