Skip to content

Fix crash when user does not have access to registry startup keys #1299

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Aug 8, 2022
Merged

Fix crash when user does not have access to registry startup keys #1299

merged 9 commits into from
Aug 8, 2022

Conversation

@nachmore
Copy link
Contributor

@nachmore nachmore commented Jul 27, 2022 •
edited
Loading

Instead log an error about non-critical functionality and continue on.

Close #1269, close #1300, close #1282, close #1281

Note: Constant.ExecutablePath cannot be null, so also simplified the if.

@nachmore
Fix crash when user does not have access to registry startup keys
c3a67d4 
Instead log an error about non-critical functionality and continue on.
@taooceros
Copy link
Member

taooceros commented Jul 27, 2022

Since you are on this part of code, is it possible to refactor it to a command in the viewmodel? So it would be possible to access the IPublicAPI to show a small notice about this error.

If not, I think it is possible to show the notification as well by accessing the helper directly.

@taooceros
Copy link
Member

taooceros commented Jul 27, 2022

At the same time, if flow cannot access the registry, maybe set the auto startup as false? Thanks

@nachmore
Copy link
Contributor Author

nachmore commented Jul 29, 2022

I did a small refactor, which makes sense especially since App.xaml.cs was calling into SettingWindow.xaml.cs to set auto startup. But:

  1. The IPublicAPI that you mention is not available to the ViewModel, it's only on the Window itself (API), so for now I've left the logging
  2. I don't think it makes sense to display a visual error message on startup if startup can't be set, but possibly if it fails to set in settings?
  3. I can flip the setting if set up fails, but not sure that's the right behaviour versus just continuing to try in case it's transient?

@taooceros
Copy link
Member

taooceros commented Jul 29, 2022 •
edited
Loading

The IPublicAPI that you mention is not available to the ViewModel, it's only on the Window itself (API), so for now I've left the logging

I don't remember exactly the code, but even if it is not available, you can send the notification via the NotificationHelper https://github.com/Flow-Launcher/Flow.Launcher/blob/dev/Flow.Launcher/Notification.cs (If I recall the name correctly) ?

I don't think it makes sense to display a visual error message on startup if startup can't be set, but possibly if it fails to set in settings?

Maybe disable autostartup by default? And add the option for setting in the widget window, then we can simply call the set startup based on the change of the value of the field (which means we don't need code behind ui).

I can flip the setting if set up fails, but not sure that's the right behaviour versus just continuing to try in case it's transient?

Maybe do it in the setter. Just don't set anything when it fails (if we use the pattern I mention before).

Otherwise the other code looks good for me. Thanks

@nachmore
Copy link
Contributor Author

nachmore commented Jul 31, 2022

I updated with Notification usage, a new string in translation. Not sure what the new string process is, so just added it to everything for now.

Note: I saw that you edited Fixes to Close - is that stylistic? Fixes will also close the linked issue when merged.

@taooceros
Copy link
Member

taooceros commented Jul 31, 2022

I just add another closes since github don't support close issues separated by comma.

@taooceros
Copy link
Member

taooceros commented Aug 1, 2022

Oh by the way you don't have to add the strings in every i18n files. Only added it to the en.xaml will be fine, and others will be added via the integration.

@taooceros
Copy link
Member

taooceros commented Aug 1, 2022

Maybe disable autostartup by default? And add the option for setting in the widget window, then we can simply call the set startup based on the change of the value of the field (which means we don't need code behind ui).

Let me show you what I mean here in another pr.

@taooceros
Copy link
Member

taooceros commented Aug 1, 2022 •
edited
Loading

I haven't tested but this is the idea (I shall start a new pr but somehow I don't want to mess up with the git remote).

@taooceros taooceros added the bug Something isn't working label Aug 1, 2022
@taooceros taooceros added this to the 1.10.0 milestone Aug 1, 2022
@taooceros
Copy link
Member

taooceros commented Aug 5, 2022 •
edited
Loading

@nachmore do you mind to take a quick debug on whether this work? I think it feels like a cleaner way. Then we can merge!
Thank you!

taooceros and others added 2 commits August 5, 2022 14:43
@taooceros
Merge branch 'dev' into bug_1269
d28fbef
@nachmore
Use direct binding to set auto-startup
ebbd1da 
Also includes better messaging (single message for registry failure)
@nachmore
Copy link
Contributor Author

nachmore commented Aug 8, 2022

Sorry for the delay, very busy period :) Found some time today; moved to the direct binding model.

@taooceros
Copy link
Member

taooceros commented Aug 8, 2022

Sorry for the delay, very busy period :) Found some time today; moved to the direct binding model.

No worries. Just take your time.

@nachmore
Copy link
Contributor Author

nachmore commented Aug 8, 2022

Ok, merged & this should be it!

@taooceros taooceros enabled auto-merge August 8, 2022 03:21
@taooceros taooceros merged commit 1d02c30 into Flow-Launcher:dev Aug 8, 2022
@taooceros
Copy link
Member

taooceros commented Aug 8, 2022

Thank you for the work!

@nachmore nachmore deleted the bug_1269 branch August 8, 2022 04:49
@jjw24 jjw24 modified the milestones: 1.10.0, 1.9.5 Sep 16, 2022
jjw24 pushed a commit that referenced this pull request Sep 16, 2022
@taooceros @jjw24
Merge pull request #1299 from nachmore/bug_1269
b38e37f 
Fix crash when user does not have access to registry startup keys
@jjw24 jjw24 mentioned this pull request Sep 18, 2022
Merged
jjw24 added a commit that referenced this pull request Sep 27, 2022
@jjw24
Release 1.9.5 (#1386)
43ac9b4 
* Merge pull request #1061 from Flow-Launcher/remove_winget_ci

* Merge pull request #991 from Flow-Launcher/context_menu_plugin_site

* Merge pull request #1080 from gissehel/caret-position-fix

* Caret position fix : Include PR #1074

* Merge pull request #1283 from nachmore/dev

* Merge pull request #1296 from nachmore/bug_1284

* Merge pull request #1294 from Flow-Launcher/pluginInfoMultipleActionKeyword

* Merge pull request #1299 from nachmore/bug_1269

* Plugin Exception Draft (#1147)

* Merge pull request #1355 from onesounds/LimitWidth

* Merge pull request #1088 from Flow-Launcher/add_spanish_latin_america

* Merge pull request #1387 from Flow-Launcher/fix_exception_duplicate_url_opening

* Merge pull request #1390 from Flow-Launcher/issue_1371

* Merge pull request #1391 from Flow-Launcher/issue_1366
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taooceros taooceros taooceros approved these changes

Assignees

@nachmore nachmore

Labels

bug Something isn't working

Projects

Work In Progress
Archived in project

Milestone

1.9.5

3 participants

@nachmore @taooceros @jjw24