Skip to content
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

Show a startup error dialog in case of invalid syntax of the config.json #1220

Closed
dxps opened this issue Apr 25, 2024 · 15 comments
Closed

Show a startup error dialog in case of invalid syntax of the config.json #1220

dxps opened this issue Apr 25, 2024 · 15 comments
Labels
enhancement New feature or request

Comments

@dxps
Copy link

dxps commented Apr 25, 2024

Is your feature request related to a problem? Please describe.

I just noticed an issue that should be quick to solve and improve the UX part by telling to the user what's the problem.

If by mistake you leave a comma at the end of the last key in the JSON config file, like this:

{
        "disableNotificationSound": true,
}

and you start the app, nothing visually appears: no systray icon, nor Gnome Dock symbol that tells if the app is started (as it's the case for others, see the white dots below the icons for the apps that are started/running).

image

But the app's processes are running:

~ ❯ ps -ef | grep teams
dxps      567424    6428  6 09:15 ?        00:00:00 /opt/teams-for-linux/teams-for-linux
dxps      567428  567424  0 09:15 ?        00:00:00 /opt/teams-for-linux/teams-for-linux --type=zygote --no-zygote-sandbox
dxps      567429  567424  0 09:15 ?        00:00:00 /opt/teams-for-linux/teams-for-linux --type=zygote
dxps      567431  567429  0 09:15 ?        00:00:00 /opt/teams-for-linux/teams-for-linux --type=zygote
dxps      567465  567428  2 09:15 ?        00:00:00 /opt/teams-for-linux/teams-for-linux --type=gpu-process --enable-crash-reporter=2a4601bf-5f85-41cb-8dcf-ce6f48625117,no_channel --user-data-dir=/home/dxps/.config/teams-for-linux --gpu-preferences=WAAAAAAAAAAgAAAEAAAAAAAAAAAAAAAAAABgAAEAAAA4AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAGAAAAAAAAAAYAAAAAAAAAAgAAAAAAAAACAAAAAAAAAAIAAAAAAAAAA== --shared-files --field-trial-handle=3,i,10697308941186590350,4592701824263653,262144 --enable-features=kWebSQLAccess --disable-features=HardwareMediaKeyHandling,SpareRendererForSitePerProcess --variations-seed-version
dxps      567469  567424  0 09:15 ?        00:00:00 /opt/teams-for-linux/teams-for-linux --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --enable-crash-reporter=2a4601bf-5f85-41cb-8dcf-ce6f48625117,no_channel --user-data-dir=/home/dxps/.config/teams-for-linux --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,10697308941186590350,4592701824263653,262144 --enable-features=kWebSQLAccess --disable-features=HardwareMediaKeyHandling,SpareRendererForSitePerProcess --variations-seed-version
~

Of course, removing that last comma and doing a killall teams-for-linux it makes all back to normal. The app starts and shows the main window.


Describe the solution you'd like

I would expect to get an popup message with the root cause of the problem.

@jijojosephk
Copy link
Collaborator

Is it throwing an error in terminal ?

@dxps
Copy link
Author

dxps commented Apr 25, 2024

I haven't open it in the terminal, just use the desktop (Gnome Dock) shortcut.

@woernsn
Copy link
Contributor

woernsn commented Apr 25, 2024

Is it throwing an error in terminal ?

Yes, it does.

❯ teams-for-linux
[WARN] No config file found, using default values
A JavaScript error occurred in the main process
Uncaught Exception:
SyntaxError: Expected double-quoted property name in JSON at position 224 (line 6 column 1)
at JSON.parse (<anonymous>)
at Conf._deserialize (/opt/teams-for-linux/resources/app.asar/node_modules/conf/dist/source/index.js:67:43)
at get store (/opt/teams-for-linux/resources/app.asar/node_modules/conf/dist/source/index.js:278:43)
at new Conf (/opt/teams-for-linux/resources/app.asar/node_modules/conf/dist/source/index.js:131:32)
at new ElectronStore (/opt/teams-for-linux/resources/app.asar/node_modules/electron-store/index.js:69:3)
at new AppConfiguration (/opt/teams-for-linux/resources/app.asar/app/appConfiguration/index.js:16:49)
at Object.<anonymous> (/opt/teams-for-linux/resources/app.asar/app/index.js:15:19)
at Module._compile (node:internal/modules/cjs/loader:1391:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1451:10)
at Module.load (node:internal/modules/cjs/loader:1214:32)

@IsmaelMartinez
Copy link
Owner

I mean, we could make the app crash instead if that is the preferred approach.

@dxps
Copy link
Author

dxps commented Apr 26, 2024

The problem would be that the user won't get a clue of the problem.
I'm just proposing to tell the user about the source of the problem: in this case, the config file having an invalid syntax.

@IsmaelMartinez
Copy link
Owner

aye @dxps, I leave it open but I consider it lower priority that other parts of the system that are currently just not working due the changes from MS from moving from angular to react. Thanks for reporting.

If you want to work on this, do let us know and I can give you some pointers on where to look at.

@IsmaelMartinez IsmaelMartinez added the enhancement New feature or request label Apr 26, 2024
@dxps
Copy link
Author

dxps commented Apr 26, 2024

@IsmaelMartinez Yeah, absolutely, it's a low prio thing. Thanks!

Sure, I may try to contribute, even though nowadays I'm a little bit obsessed and caught in the Rust space.

@woernsn
Copy link
Contributor

woernsn commented Apr 26, 2024

aye @dxps, I leave it open but I consider it lower priority that other parts of the system that are currently just not working due the changes from MS from moving from angular to react. Thanks for reporting.

If you want to work on this, do let us know and I can give you some pointers on where to look at.

I tried to implement it but am not really a big electron developer.
Let me know if I'm on a totally wrong track 😉.

@IsmaelMartinez
Copy link
Owner

Hi @woernsn , thanks a lot for contributing! I can see the approach you have taken and, in general, it should work, but you will need to not fix that config path or it will only work for you. Thanks again for the PR!

@palves
Copy link

palves commented May 6, 2024

With 1.4.38, if you don't have a config.json file at all, you see:

$ [WARN] No config file found, using default values

on the console, and then a "Configuration error" dialog box. That seems unexpected? Is one forced to have a config.json nowadays? If so, why doesn't the app create a minimal empty one for you automatically?

@woernsn
Copy link
Contributor

woernsn commented May 6, 2024

Is one forced to have a config.json nowadays? If so, why doesn't the app create a minimal empty one for you automatically?

Good catch!
I never even thought about the usecase to not have a config.

@woernsn
Copy link
Contributor

woernsn commented May 7, 2024

@palves, can you test 1.4.39 again when it is available?
The issue should be fixed now.

@palves
Copy link

palves commented May 7, 2024

@palves, can you test 1.4.39 again when it is available? The issue should be fixed now.

Sure thing. Thanks for fixing!

@IsmaelMartinez
Copy link
Owner

It should be ready under https://github.com/IsmaelMartinez/teams-for-linux/releases/tag/v1.4.39 (pre-release as I like to keep the blast radius smaller). Thanks for testing it!

@palves
Copy link

palves commented May 7, 2024

Thanks for the URL. I just tried it (deb on Ubuntu 22.04), and it starts fine now, no dialog box. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants