Skip to content

Fix: Catch COMException when creating submenus to avoid errors #11363

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

Merged
merged 2 commits into from
Feb 19, 2023

Conversation

hishitetsu
Copy link
Member

Resolved / Related Issues
Items resolved / related issues by this PR.

Details
Here is another possible solution. This way, if "Cast to Device" is not a problem, it can be displayed in the menu.
I can't reproduce the error, so I would like someone to check if applying this PR will stop the error from occurring.

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

@hishitetsu hishitetsu mentioned this pull request Feb 19, 2023
2 tasks
@hishitetsu hishitetsu marked this pull request as ready for review February 19, 2023 06:55
@hishitetsu
Copy link
Member Author

I'm not sure if this will completely solve the problem, but there is always a possibility that COMException may occur due to a problem on the DLL side, so I think it is better to merge this code.

@yaira2
Copy link
Member

yaira2 commented Feb 19, 2023

This is a lot better! Should the error notification be removed for these as well?

@hishitetsu
Copy link
Member Author

I think so because users cannot do anything if they are notified of an error, and it is difficult to fix as a bug.
If the submenu could not be loaded, it may be a good idea to display "Couldn't load the submenu" on the submenu.

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 merged commit 36351f9 into files-community:main Feb 19, 2023
@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Feb 19, 2023
@hishitetsu hishitetsu deleted the CatchCOMException branch February 19, 2023 15:34
@yaira2
Copy link
Member

yaira2 commented Feb 21, 2023

I'm still getting a few com exceptions but it's a lot better than before.

@hishitetsu
Copy link
Member Author

Perhaps we need to review all the places where COMException can occur.

@yaira2
Copy link
Member

yaira2 commented Feb 21, 2023

We can place breakpoints but the errors don't always occur so it's hard to find them all.

@yaira2
Copy link
Member

yaira2 commented Feb 21, 2023

@hishitetsu what do you think about private static void EnumMenuItems()? It looks like we can use some error handling there.

@hishitetsu
Copy link
Member Author

hishitetsu commented Feb 22, 2023

Is there any possibility that QueryContextMenu() outside of EnumMenuItems() throws an error? I am suspicious of this as well.
The menu cannot be displayed if this throws a COMException and we catch it, but is this better than being notified of an error?

@yaira2
Copy link
Member

yaira2 commented Feb 22, 2023

I think so, worst case the user can open the menu again.

@hishitetsu
Copy link
Member Author

Certainly. I'll work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
3 participants