Fixes #3678. ContextMenu accesses disposed MenuBar.

[BDisp]

Fixes

• Fixes ContextMenu accesses disposed MenuBar #3678

Proposed Changes/Todos

• Call UngrabMouse before disposing _menubar

Pull Request checklist:

• I've named my PR in the form of "Fixes #issue. Terse description."
• My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• My code follows the Terminal.Gui library design guidelines
• I ran dotnet test before commit
• I have made corresponding changes to the API documentation (using /// style comments)
• My changes generate no new warnings
• I have checked my code and corrected any poor grammar or misspellings
• I conducted basic QA to assure all features are working

[BDisp]

@tznind is there any problem the _menuBar being disposing here for the TGD?

Edit:
In my opinion I think that is ok because _menuBar is a ContextMenu's private static field and it's itself that handles for his disposing and not by another view, like the Toplevel was doing for the MenuBar instance, right?

[tznind]

Should be fine. Maybe some of the Undo operations might access a closed sub menu. E.g.

• open menu
• add new entry
• close menu
• hit ctrl+z

[BDisp]

Should be fine. Maybe some of the Undo operations might access a closed sub menu. E.g.

• open menu
• add new entry
• close menu
• hit ctrl+z

Can you please test if that really happens when ctrl+z is hit? If affirmative I've to found a solution to fix it. _menuBar is never added to a superview, but initialized calling BeginInit/EndInit. Only the menus are added to superview when they are opened. Maybe subscribing to the Application.Top.Dispose event to dispose the _menuBar instead of disposing on the MenuAllClosed event. But this is only needed after you confirm that this may be an issue. Thanks.

[dodexahedron]

Similar commentary, but with a little more detail, to what I was talking about with @tig in #3675 about ObjectDisposedException.

[dodexahedron]

That view should be the object responsible for throwing this exception, not a class using it.

Same exception is fine - just belongs elsewhere.

If an exception is desired here, it should be most likely be InvalidOperationException, but only if the MouseGrabView View didn't already throw ObjectDisposedException anyway. I'd argue that the need for the exception being thrown by this class is itself a bug, though.

Typically, the object that was disposed is the one who is supposed to throw ODE.

Assuming that principle is followed, and a class like this one that owns a disposable object tries to use that object after that object was disposed, this is how it's supposed to be handled:

• Highest preference/best option, if possible, is to try to avoid the situation entirely by performing a safe check before trying to use the IDisposable object and creating a new one if necessary. That's one of the main uses of a Disposed event - letting subscribers holding a reference to that object know it has been disposed, so they can react appropriately, such as by setting their reference to it to null, removing it from any collections they may have put it in, or any other implementation details that the IDisposable doesn't know about and can't be expected to know about; OR
• Next best is to try to recover from the situation, if you catch an ODE that you weren't able to avoid. Usually that just means making a new instance of the object that was disposed, if there's sufficient information to do so. OR
• If the object trying to use the IDisposable can't do either of the above safely, it should then do one of:
  • Let the ODE bubble up. OR
  • If it caught the ODE because it has clean-up to do before things can safely continue, re-throw it as-is at the end of the catch. OR
  • If additional context is important to add at that point in the code that the stack trace wouldn't already have been sufficient for, wrap the ODE in an InvalidOperationException, which is the type of exception that is expected to be used when the object is in an invalid state (which it is, because of the disposed object, which is part of its state, and thus the operation is invalid for the current state of the object)

All objects owning an IDisposable instance or static member field or property should do the above procedure, at minimum, as part of the contract that IDisposable is supposed to convey.

The IDisposable objects themselves are responsible for their own cleanup, part of which is informing others that disposal has happened.
That's the easiest, most timely, and most efficient way to do the first bullet (avoiding/preventing the situation entirely), and prevents any exceptions from being thrown, which saves a lot of expensive implicit stuff that happens when an exception is thrown, like generating the stack trace and system event logging and everything else that an exception involves.

At that point, if we've implemented our end of the contract properly, any consumer who doesn't avoid the exception can't blame it on us, as it is not our fault - It's their bug for not respecting the contract, just like it is currently our fault for not respecting our own contract.

[tig]

That view should be the object responsible for throwing this exception, not a class using it.

My bad. I must have mis-read you before, because I thought you were recommending the opposite.

[BDisp]

@tig do you want I made any change here?

[BDisp]

Maybe to this, because it's passing the object that is failing?

ObjectDisposedException.ThrowIf (MouseGrabView.WasDisposed, MouseGrabView);

[BDisp]

Maybe to this, because it's passing the object that is failing?

ObjectDisposedException.ThrowIf (MouseGrabView.WasDisposed, MouseGrabView);

@dodexahedron is this more appropriate?

[dodexahedron]

Maybe to this, because it's passing the object that is failing?
ObjectDisposedException.ThrowIf (MouseGrabView.WasDisposed, MouseGrabView);

@dodexahedron is this more appropriate?

Not quite. It's not the exception - it's where it's being thrown.

I can pull this down to take an actual look at it in a few minutes.

Is whatever is latest currently pushed to your branch?

[dodexahedron]

That view should be the object responsible for throwing this exception, not a class using it.

My bad. I must have mis-read you before, because I thought you were recommending the opposite.

I was pretty certain there would be at least one or five reversals. 😅

That's part of why those comments took me a while to write - I had to keep going back and making sure I wasn't reversing things, which was super easy to do and I did do (but therefore fixed before I posted....I think).¹ 😆

The pseudo-logic way I stuck it in this thread is a lot easier for my eye to follow, at least, than how I babbled it out in your PR. 🤔

Footnotes

1. Aaaannnnnnd I just realized that sentence is anything but clear, as well. 🤦‍♂️ ↩

[BDisp]

I think I found a better solution than what I first presented. It's only worth preserving the last opened MenuBar. In any case, the ContextMenu information is always present when the object is created and assigned to the MenuItems property. ContextMenu just creates a static instance of the MenuBar with the information collected from the MenuItems property. At least now, if there is a need to access information from the last MenuBar of the ContextMenu opened, it will be available while the application is running.

[dodexahedron]

Cool. I'll have a look at the new hotness in a few minutes.

It's only worth preserving the last opened MenuBar.

Depending on other specifics, it is either easy or easy-ish to make at least this argument against that reasoning: Why should we reconstruct objects that are pretty heavy to construct (due to all the hotkey parsing and all that sort of stuff, especially), when they could very well be needed again, possibly very soon? Just seems somewhat unconventional and kinda wasteful.

I'm sure seeing the code will help answer some of the underlying technical bits, though, so I'll come back here once I get a good look at it.......assuming I don't get distracted. But I told alexa to remind me every 30 minutes to check my focus, just in case. 😅

[dodexahedron]

Oh yeah also while it's in my mind...

Something both of you might find useful is the .net Community Toolkit has a standalone component for its CommunityToolkit.Diagnostics namespace (no other dependencies), which has a TON more throw helpers and guard methods, covering a bunch more exception types as well as a lot more use cases, in a semi-fluent style, similar to how Asserts look.

And it's another code generation-based thing, so it doesn't add a runtime dependency on the CommunityToolkit.Diagnostics package nor a design or build-time dependency for consumers of the Terminal.Gui library, either. Probably one we should add and make use of, for the rather nice consistency it can help provide, just like the built-in throw helpers do for the few exception types those are implemented for. 👍

I have something partially written up for #3675 that I'll send over to @tig when it's done that also demonstrates a much cleaner way of dealing with all those conditionals, rather than having all the preprocessor directives for them. In short, it's primarily just making use of the [Conditional(string)] attribute, which allows you to call a method it is placed on as if it is always there, but the compiler will skip it without the need for messy preprocessor directives if the symbol given to the attribute is not defined. That and [DoesNotReturn] (on a method) or [DoesNotReturnIf(bool)] (on a parameter of a method) give cleaner and slightly DRY-er code, plus great static analysis like most of the throw helpers also provide (they use those attributes for exactly that).

Anyway.... Off to the git machine!

[BDisp]

The unit test error isn't belong from this PR but reproduces in the v2_develop branch.

[BDisp]

@dodexahedron please don't send anymore PR to my branches that aren't directly related with the issue that the PR was created. If you want to improve and cleanup code that you think the TG needs to, then submit a PR yourself to the TG repository and not for the forked repositories. I spend a while to fix an erroneous code you submitted to my branch but that was unnecessary for me take the responsibility to worry about that. I admit that was my fault when I merged your PR, but please don't send me more unrelated PR to my branches. Sorry if I offended you but that wasn't my intention. Thanks.

[tig]

Great work @BDisp