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

Context Menu improvements; Select menu, Edit menu #1102

Merged
merged 42 commits into from
Sep 28, 2024

Conversation

dfordivam
Copy link
Collaborator

This includes a number of improvements like

  • GHC(s) can be selected from a list of installed GHCs in bootstrap-ghc, hadrian-ghc, and HLS target-ghcs
  • Build system can be selected from choices
  • All the text edit inputs require opening an edit box by pressing "Enter".
    The help message an error message are also shown as the user types.

With this change it will be possible to use the user's keybindings in the advance popup's navigation. @lsmor

tui-demo2

@hasufell
Copy link
Member

  • bootstrap/hadrian GHC can be a full path to a binary
    • it can also be a ghc version in PATH that is not known to ghcup
  • the titles of the popups are cut off (e.g. try the 'flavour' popup)

@hasufell
Copy link
Member

Another thing I noticed... it's very hard to distinguish required and optional settings.

Required settings should come first and maybe even be distinct from optional ones (not sure how exactly).

@dfordivam
Copy link
Collaborator Author

Another thing I noticed... it's very hard to distinguish required and optional settings.

Required settings should come first and maybe even be distinct from optional ones (not sure how exactly).

Ok, I have moved required fields to first. The simplest way to indicate the required fields is to mention them in the help message like this.

2024-07-19-121304_screenshot

the titles of the popups are cut off (e.g. try the 'flavour' popup)

Fixed

bootstrap/hadrian GHC can be a full path to a binary it can also be a ghc version in PATH that is not known to ghcup

I have included additional fields to input them, as the UI gets considerably complex to support it via custom widgets.


2024-07-19-121841_screenshot
2024-07-19-121857_screenshot

@hasufell
Copy link
Member

The simplest way to indicate the required fields is to mention them in the help message like this.

Most HTML UI uses an asterisk (* ...and possibly in red) and then have a line at the bottom: * required fields.

I have included additional fields to input them, as the UI gets considerably complex to support it via custom widgets.

I'm not too excited about additional fields UX wise, because now it's not clear that those are "either or".

What does it take to implement a composite field that allows a drop-down + just a string?

@hasufell hasufell added this to the 0.1.31.0 milestone Jul 20, 2024
@dfordivam
Copy link
Collaborator Author

I'm not too excited about additional fields UX wise, because now it's not clear that those are "either or".

What does it take to implement a composite field that allows a drop-down + just a string?

Ok makes sense. It is a bit complicated to do it the way other editable fields are handled, as the nesting of menu fields is tricky, but I did it the following style which avoids an additional overlay to edit the field's text. Does this look ok?

tui-demo3

@hasufell
Copy link
Member

hasufell commented Jul 25, 2024

Does this look ok?

Yeah. I'd actually make the input field the first and is it possible to have a "value" for it when it's not selected, which could indicate that it is a custom input field?

@dfordivam
Copy link
Collaborator Author

Yeah. I'd actually make the input field the first and is it possible to have a "value" for it when it's not selected, which could indicate that it is a custom input field?

Done.

Most HTML UI uses an asterisk (* ...and possibly in red) and then have a line at the bottom: * required fields.

In the case of compile GHC and HLS menus there is currently only one required field, so I think the current design where we show an error in red if the field is empty is a fine UI.

tui-demo4

Also note that the gif I generated above doesn't show the help messages properly, but on my terminal I can see them quite clearly. Like the "Required fields: bootstrap-ghc" text.

2024-07-31-133950_screenshot

@dfordivam
Copy link
Collaborator Author

And keeping the focus of the select menu's focusring on the first item after the text input field, is intentional.

Compare this
2024-07-31-134738_screenshot

vs this, (if the focus is on text input field when the user opens the select menu)
2024-07-31-134801_screenshot

@dfordivam
Copy link
Collaborator Author

I changed the help msg text to yellow, as it was not visible at all on the Ubuntu' gnome terminal' default theme.

Screenshot from 2024-08-07 13-51-59

Screenshot from 2024-08-07 13-54-51

@dfordivam
Copy link
Collaborator Author

I have done more changes to address #1101

  • Removed the hardcoded keybindings of Ctrl+C, Up, Down arrow keys in context menu, and tutorial navigation
  • now the default quit keybinding of 'q' is used to move back / close context menu,
  • the up/down keys can be set to custom values like 'j' & 'k', and they should work in context menu navigation.
  • The Editable input field is open in editing mode, it allows input of all keys including 'q', and it can only be closed by pressing 'Enter' (hardcoded)
  • The custom text field of select field input is a bit tricky to handle. So I have decided with this approach:
    The navigation keys like 'q', 'j', 'k' will work like normal to move focus among the different choices and select them by pressing Enter.
    But if the focus is on custom text field, then the only way to exit the edit mode is to press Enter, Up or Down keys.
    The help message in this scenario indicates that user must press Enter to finish editing like this

Screenshot from 2024-08-08 17-11-34

@dfordivam dfordivam mentioned this pull request Sep 20, 2024

-- | Passes all events to innerHandler if an overlay is opened
-- else handles the exitKey and Enter key for the Menu's "OkButton"
menuWithOverlayHandler
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a lot of formatting requirements, but :: on the next line is a no-no, because it breaks grepping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@hasufell
Copy link
Member

The PR is too big for me to review/digest in time, especially since I'm not familiar anymore with large parts of the TUI.

The functionality is fine afais.

@hasufell hasufell merged commit 45f80f9 into haskell:master Sep 28, 2024
46 of 47 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.

2 participants