gui-cs / TerminalGuiDesigner

Forms Designer for Terminal.Gui (aka gui.cs)
MIT License
423 stars 28 forks source link

Click and drag firing when modal context menu is open #272

Open tznind opened 8 months ago

tznind commented 8 months ago

There is code that is supposed to lock operations and prevent move events firing. This may be a regression or a new bug or something that came from upstream. Needs investigation

Maybe we just need to expand the return conditions to also include context menus e.g.

// if another window is showing don't respond to mouse
if (!this.IsCurrentTop /* TODO: or context menu showing*/)
{
    return;
}

mouse-down-in-context-menu-bad

dodexahedron commented 8 months ago

The MouseManager has some problems that I've notated in the updated MouseManagerTests.cs file.

They're basic issues with how conditions are identified, so might be a wise place to begin before directly addressing this. Building on a foundation of stone rather than sand and all that.

This branch has the refactored MouseManager tests, with some specific additions to one of the tests to aid in debugging the root cause of the problem.

Some of those tests should be failing, and would be if I had left the "correct" assertions in (I committed at least one commented-out example of what I think should be tested in at least one of the cases.. I think the comments I left in the MouseManagerTests.cs file should be pretty self-explanatory, but definitely let me know if you have any questions/concerns/thoughts/letters to the editor about any of that.

dodexahedron commented 8 months ago

Specifically about locking:

I know there's at least one instance of booleans being used for locking, which isn't safe and has lots of scary all-caps and red-x bearing notes in the dotnet docs.

One of them that I am already aware of and which is one of the parts of what requires the unit tests to run mostly serially and which is directly relevant to this issue posting is SelectionManager.LockSelection, which isn't thread-safe for parallelism or reentrancy.

It's another class of issues, though, that I think are best served by waiting til the test project is finished being updated and enhanced to provide more complete and more accurate information for us to work off of.

I should be able to start looking at some of that kind of thing pretty soon. I've been spending less time improving the tests, for the past couple of test fixtures, opting to focus more on the ultra-basics of converting them to the constraint model and that sort of thing. I intend to keep taking that approach (aside from when I get ADHDistracted by something 😅), so I should probably be able to at least convert the rest of the remaining test fixtures pretty soon. Maybe within the next week or two, even. 🤞

I went ahead and started a discussion here for thread safety, so we can chat about that when we get around to it or whenever inspiration strikes. :)