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

Add MenuBar #97

Merged
merged 18 commits into from
Aug 12, 2020
Merged

Add MenuBar #97

merged 18 commits into from
Aug 12, 2020

Conversation

joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Aug 4, 2020

This PR replaces #93.

Trello

https://trello.com/c/4GtDpmMo/

Overview

This adds a MenuBar widget to the NCurses UI.

For more details, see the main PR for this feature:

/~https://github.com/libyui/libyui/pull/169

Travis Build Failure

This doesn't build because the image used by Travis does not yet contain the libyui with the new features. This is expected.

Related PRs

Proposed Solution

There is a new class NCMenuBar that manages the top menu list. This class has the following responsibilities:

  • To draw the menus.
  • To select the next/previous menu.
  • To skip over disabled menus.
  • To open a popup with the selected menu (using a NCPopupMenu).
  • To handle shortcuts for the menus.
  • Cyclic navigation between options.
  • When a menu is open, key left opens the previous menu (if any).
  • When a menu is open, key right opens the next menu (if any and current item has no submenu).
  • Esc to close the menu.

The class NCPopupMenu was extended, gaining these responsibilities:

  • To skip over disabled options.
  • Cyclic navigation between options.
  • Auto-select the first enabled item when the first item is disabled.

The NCPopupMenu internally uses a NCTable, and this could have some drawbacks:

  • How to include check- and radio-boxes in the submenu?

Screenshot from 2020-08-12 17-02-42

Screenshot from 2020-08-12 16-57-29

Pending Tasks:

  • Shortcuts for top level menus.
  • Represent a separator with a line?

NOTE: Pending tasks will come in another PR. We want to merge this now to synchronize SO version of all libyui related packages.

src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCMenuBar.h Outdated Show resolved Hide resolved
src/NCMenuBar.h Outdated Show resolved Hide resolved
src/NCMenuBar.h Outdated Show resolved Hide resolved
src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCPopupMenu.cc Outdated Show resolved Hide resolved
src/NCPopupMenu.h Outdated Show resolved Hide resolved
src/NCMenuBar.cc Outdated Show resolved Hide resolved
src/NCMenuBar.cc Outdated Show resolved Hide resolved
src/NCMenuBar.cc Outdated Show resolved Hide resolved
src/NCMenuBar.cc Outdated Show resolved Hide resolved
@shundhammer
Copy link
Contributor

LGTM for an intermediate merge so we can move forward with this.

@joseivanlopez joseivanlopez marked this pull request as ready for review August 12, 2020 08:15
Copy link
Contributor

@shundhammer shundhammer left a comment

Choose a reason for hiding this comment

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

approved

@anaselli
Copy link
Contributor

anaselli commented Aug 26, 2020

I would add on pending task also:

  • pressing space or return should open menus not doing nothing

And yes please add something to show separator, leaving an empty line is not looking like a separator

@anaselli
Copy link
Contributor

anaselli commented Aug 26, 2020

In a first try I also changed "..." to show that there is a sub menu with ">" maybe you can think to do something like that pretty better looking, I gave up just thinking to R2L languages

I like the menu selection and move left and right that i haven't implemented so well.

Please follow also discussion here

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.

3 participants