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

core: catch command handler errors #5894

Merged
merged 1 commit into from
Aug 9, 2019
Merged

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Aug 8, 2019

What it does

fix #5898 - catches command handler errors.

Also fix bug where getToggledHandler would stop on the first handler with a isToggled function defined, without executing it.

How to test

Modify some command handler to throw errors, without this patch it can prevent the command palette from opening. With this change, the error will be reported but it will not stop the processing of other contributions.

Review checklist

Reminder for reviewers

@vince-fugnitto
Copy link
Member

It works really well! I was able to test by throwing errors for a command on both isVisible and isEnabled and it was handled correctly.

Screen Shot 2019-08-08 at 8 03 08 PM

It also fixes the issue #827 👍

@akosyakov
Copy link
Member

@marechal-p please see #5898

You should fix and handle getToggledHandler as well, right now it is bogus, since it does not actually call isToggled for all handlers, only for first which has such method.

@akosyakov akosyakov added the commands issues related to application commands label Aug 9, 2019
If a contribution throws an error during basic lifecycle steps, it might
kill whatever is trying to iterate over all the contributions.

A better strategy would be to catch errorred handlers and keep looking
for other contributions.

Also fix `.getToggledHandler` to not stop prematurely and actually test
if the handler is toggled or not.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal
Copy link
Member Author

Fixed.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code looks good time

@vince-fugnitto could you give it a go please and if you are fine, let's merge

@vince-fugnitto
Copy link
Member

It still works correctly on my end :)
@marechal-p do you want to address the comment before merging?

@paul-marechal paul-marechal merged commit 11badc8 into master Aug 9, 2019
@paul-marechal paul-marechal deleted the mp/command-handler-error branch August 9, 2019 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commands issues related to application commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[commands] handle command handler errors
3 participants