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

Fixes #3764. Makes common UI interactions consistent across built-in Views #3766

Merged
merged 75 commits into from
Oct 9, 2024

Conversation

tig
Copy link
Collaborator

@tig tig commented Sep 29, 2024

Fixes

Proposed Changes/Todos

This is signficant PR that changes the behavior of View as well as Button, CheckBox, RadioGroup, TextView etc.. for

  • The base set of Commands - Select, Accept, and HotKey now have default implementations in View
  • Each of those Commands has a corresponding event (e.g. View.Selecting/OnSelecting).
  • The pattern for declaring a Command as handled has been beefed up and code now MUST pay attention to setting e.Canceled properly in event handlers.

More:

See navigation.md in this PR for more...

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

@tig tig marked this pull request as ready for review October 2, 2024 21:14
@tig
Copy link
Collaborator Author

tig commented Oct 2, 2024

@tznind I would really appreciate your eyes on this one. This changes a bunch of default behavior and may impact TGD. Plus you originally implemented Command and KeyBindings...

@BDisp
Copy link
Collaborator

BDisp commented Oct 7, 2024

I think pressing F6 it's a useful way to jump from a group box to another, but I also thing that if no more subviews exist in a group box pressing tab or shift tab can jump to the next or previous group box, instead of a static position.

@BDisp
Copy link
Collaborator

BDisp commented Oct 7, 2024

The Left button isn't the right position.

image

@BDisp
Copy link
Collaborator

BDisp commented Oct 7, 2024

After setting UseMenusSingleFrame to true in the ContextMenu on reopening the menu it jump to the Languages sub-menu. When click in the back button the menu doesn't focus the item on the mouse position anymore.

WindowsTerminal_8KbOkrHsjD.mp4

@BDisp
Copy link
Collaborator

BDisp commented Oct 7, 2024

The column width should be based on the greater width of the Focused (Title) or properties.

image

@BDisp
Copy link
Collaborator

BDisp commented Oct 7, 2024

This should be fixed asap. In my opinion the Window should by default be fit dependent of MenuBar and StatusBar, without the need of any additional setting. Other solution is define that it's mandatory to setup the views layout at design time. For now a MenuBar has a height of 1, but if it's changed in the future, then the Window must be layout with Y = Pos.Bottom(menuBar). In this case we need to change the current layout in the scenarios.

image

@BDisp
Copy link
Collaborator

BDisp commented Oct 7, 2024

Please remove this line because is throwing StackOverflowException.

image

image

@BDisp
Copy link
Collaborator

BDisp commented Oct 7, 2024

Ok, Ready for Review again!

Lots of changes. Important stuff. Please be vocal!

I hope you don't think I wasn't be vocal 😄

@tig
Copy link
Collaborator Author

tig commented Oct 8, 2024

I think pressing F6 it's a useful way to jump from a group box to another, but I also thing that if no more subviews exist in a group box pressing tab or shift tab can jump to the next or previous group box, instead of a static position.

I disagree. Not going to debate it here though as I want to focus on changes I made in this PR and that was done elsewhere.

Please discuss here: #3775

@tig
Copy link
Collaborator Author

tig commented Oct 8, 2024

The Left button isn't the right position.

image

It's positioned exactly as the Scenario code says it should be. And this has nothing to do with this PR.

@tig
Copy link
Collaborator Author

tig commented Oct 8, 2024

After setting UseMenusSingleFrame to true in the ContextMenu on reopening the menu it jump to the Languages sub-menu. When click in the back button the menu doesn't focus the item on the mouse position anymore.

WindowsTerminal_8KbOkrHsjD.mp4

You're right.

However, remember that I'm in the middle of completely replacing Menu, MenuBar, and ContextMenu so it really doesn't matter.

@tig
Copy link
Collaborator Author

tig commented Oct 8, 2024

This should be fixed asap.

In my opinion the Window should by default be fit dependent of MenuBar and StatusBar, without the need of any additional setting. Other solution is define that it's mandatory to setup the views layout at design time. For now a MenuBar has a height of 1, but if it's changed in the future, then the Window must be layout with Y = Pos.Bottom(menuBar). In this case we need to change the current layout in the scenarios.

I don't think I understand what you are saying. However, here's my perspective.

  • Menubar and Statusbar should be Views just like any other view. There should not be code in any other View sub-class, such as Window that has knowledge of either.
  • If you want a MenuBar in a View, add it as a subview. Same with StatusBar. By default the new StatusBar has Y = Dim.AnchorEnd(). The new MenuBar should have Y = 0. Devs can change these is they want.
MenuBar menuBar = new MenuBar () { ... }; // Y = 0 is default
StatusBar statusBar = new StatusBar () { ...}; // Y = Pos.AnchorEnd() is default
ListView listView = new ListView () 
{
    Y = Pos.Bottom (menuBar),
    Width = Dim.Fill(),
    Height = Dim.Fill (Dim.Func (() => statusBar.Frame.Height)),
}

Add (menuBar, listView, statusBar);

This is basically how UI Catalog's main window works today. I just tweaked this PR to prove it. I had to trick Toplevel into not magically setting TopLevel.Statusbar.

The Dim.Fill (Dim.Func ... could easily be simpler: We may want to provide an enhancement to DimFIll:

Height = Dim.FIll (to: statusBar),

Or, we could be even smarter and provide a Dim.Fill variant that fills based on the largest Pos.AnchorEnd found:

Height = Dim.Fill (toTopAnchorEnd: true),

As i said, I'm not quite following what you wrote above, so I may be missing some use-case in my thinking. But it seems to me this will work.

@tig
Copy link
Collaborator Author

tig commented Oct 8, 2024

I think pressing F6 it's a useful way to jump from a group box to another, but I also thing that if no more subviews exist in a group box pressing tab or shift tab can jump to the next or previous group box, instead of a static position.

I disagree. Not going to debate it here though as I want to focus on changes I made in this PR and that was done elsewhere.

Ok, Ready for Review again!
Lots of changes. Important stuff. Please be vocal!

I hope you don't think I wasn't be vocal 😄

Great stuff @BDisp!! Keep it coming.

@tig
Copy link
Collaborator Author

tig commented Oct 8, 2024

This should be fixed asap. In my opinion the Window should by default be fit dependent of MenuBar and StatusBar, without the need of any additional setting. Other solution is define that it's mandatory to setup the views layout at design time. For now a MenuBar has a height of 1, but if it's changed in the future, then the Window must be layout with Y = Pos.Bottom(menuBar). In this case we need to change the current layout in the scenarios.

image

@BDisp Please file an issue for things that are broken in v2_develop or main. It's really frustrating to chase something like this down only to discover that this PR didn't have anything to do with breaking it.

@BDisp
Copy link
Collaborator

BDisp commented Oct 8, 2024

It's positioned exactly as the Scenario code says it should be. And this has nothing to do with this PR.

Sorry I didn't saw the code.

@tig tig merged commit 426b991 into gui-cs:v2_develop Oct 9, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize common events for Control-like Views (Select, Accept, HotKey, etc...)
3 participants