Improve MenuBar #99
Improve MenuBar #99
Conversation
joseivanlopez
force-pushed
the
improve_menu_bar
branch
from
b960ab7
to
4e09dfe
Compare
| #include <vector> | ||
|
|
||
| template <class T> | ||
| class CyclicContainer |
There was a problem hiding this comment.
So this is a container with wrap-around behaviour for next() and previous(), right Please add some lines of documentation to describe this.
Does it also have something like selectFirst() and selectLast() ?
There was a problem hiding this comment.
Please run make docs and make sure that the HTML docs make sense for the new code. A one line description for a new class is a bare minimum!
There was a problem hiding this comment.
Selecting first and last is not explicitly needed. Do you think we need to add it?
| } | ||
|
|
||
|
|
||
| void add( T * element ) |
There was a problem hiding this comment.
NO! NO! NO!
This is a "naked pointer". Nobody knows who owns (deletes) the data pointed to.
Use smart pointers instead. If you must use a dumb pointer you must document the ownership.
There was a problem hiding this comment.
I'd vote to document who owns the object. That is indeed missing here.
There was a problem hiding this comment.
Since this class is ultimately using YMenuItems, ownership will remain with the YSelectionWidget, as usual; but that knowledge is implicit here. Somebody reading this code will probably not know that.
There was a problem hiding this comment.
Yes, you are right. I will add documentation.
|
LGTM |
Problem
A new MenuBar widget was added in a previous PR, see #97. But the management of hot-keys for the top level menu options was completely missing.
Solution
Hot-keys for top level menu options are now properly handled.
Bonus: A new class
CyclicContainerwas created to extract some repeated logic.Note: This PR does not fix the shorcuts conflicts in the top level menu options. This will come with a separate fix, see https://bugzilla.suse.com/show_bug.cgi?id=1175142.