-
Notifications
You must be signed in to change notification settings - Fork 18
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
Improve MenuBar #99
Improve MenuBar #99
Changes from all commits
5b9d1ae
69db932
4e09dfe
902f6fb
9c17502
561bb3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,7 @@ | |
|
||
|
||
Name: libyui-ncurses | ||
Version: 2.56.0 | ||
Version: 2.56.1 | ||
Release: 0 | ||
Source: %{name}-%{version}.tar.bz2 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
/* | ||
Copyright (C) 2020 SUSE LLC | ||
This library is free software; you can redistribute it and/or modify | ||
it under the terms of the GNU Lesser General Public License as | ||
published by the Free Software Foundation; either version 2.1 of the | ||
License, or (at your option) version 3.0 of the License. This library | ||
is distributed in the hope that it will be useful, but WITHOUT ANY | ||
WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public | ||
License for more details. You should have received a copy of the GNU | ||
Lesser General Public License along with this library; if not, write | ||
to the Free Software Foundation, Inc., 51 Franklin Street, Fifth | ||
Floor, Boston, MA 02110-1301 USA | ||
*/ | ||
|
||
|
||
/*-/ | ||
|
||
File: CyclicContainer.h | ||
|
||
Author: Jose Iván López <jlopez@suse.de> | ||
|
||
/-*/ | ||
|
||
|
||
#ifndef CyclicContainer_h | ||
#define CyclicContainer_h | ||
|
||
#include <algorithm> | ||
#include <iterator> | ||
#include <vector> | ||
|
||
/** Container class that allows cyclic navigation between its elements by moving to the next/previous | ||
* element. | ||
* | ||
* @note This class holds pointers, but it does not own the pointers. | ||
**/ | ||
template <class T> | ||
class CyclicContainer | ||
{ | ||
public: | ||
|
||
using Iterator = typename std::vector<T *>::iterator; | ||
using ReverseIterator = std::reverse_iterator<Iterator>; | ||
|
||
Iterator begin() { return _elements.begin(); } | ||
Iterator end() { return _elements.end(); } | ||
|
||
CyclicContainer() : _elements(), _current( nullptr ) | ||
{} | ||
|
||
|
||
~CyclicContainer() | ||
{ | ||
clear(); | ||
} | ||
|
||
|
||
void clear() | ||
{ | ||
_elements.clear(); | ||
_current = nullptr; | ||
} | ||
|
||
|
||
void add( T * element ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd vote to document who owns the object. That is indeed missing here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you are right. I will add documentation. |
||
{ | ||
_elements.push_back(element); | ||
} | ||
|
||
|
||
void setCurrent(Iterator element) | ||
{ | ||
if ( element != _elements.end() ) | ||
_current = *element; | ||
} | ||
|
||
|
||
Iterator current() | ||
{ | ||
return std::find( _elements.begin(), _elements.end(), _current ); | ||
} | ||
|
||
|
||
Iterator next() | ||
{ | ||
Iterator current = this->current(); | ||
|
||
if ( current == _elements.end() ) | ||
return findNext( _elements.begin() ); | ||
|
||
Iterator next = findNext( std::next( current, 1 ) ); | ||
|
||
if ( next == _elements.end() ) | ||
return findNext( _elements.begin() ); | ||
|
||
return next; | ||
} | ||
|
||
|
||
Iterator previous() | ||
{ | ||
Iterator current = this->current(); | ||
|
||
ReverseIterator rbegin; | ||
|
||
if ( current == _elements.end() ) | ||
rbegin = _elements.rbegin(); | ||
else | ||
rbegin = ReverseIterator( current ); | ||
|
||
ReverseIterator previous = findPrevious( rbegin ); | ||
|
||
if ( previous == _elements.rend() && rbegin != _elements.rbegin() ) | ||
previous = findPrevious( _elements.rbegin() ); | ||
|
||
if ( previous == _elements.rend() ) | ||
return _elements.end(); | ||
|
||
return find( _elements.begin(), _elements.end(), *previous ); | ||
} | ||
|
||
private: | ||
|
||
Iterator findNext( Iterator begin ) | ||
{ | ||
return find_if( begin, _elements.end(), [](const T * element) { | ||
return element->isSelectable(); | ||
}); | ||
} | ||
|
||
|
||
ReverseIterator findPrevious( ReverseIterator rbegin ) | ||
{ | ||
return find_if( rbegin, _elements.rend(), [](const T * element) { | ||
return element->isSelectable(); | ||
}); | ||
} | ||
|
||
|
||
std::vector<T *> _elements; | ||
|
||
T * _current; | ||
}; | ||
|
||
#endif // CyclicContainer_h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a container with wrap-around behaviour for
next()
andprevious()
, right Please add some lines of documentation to describe this.Does it also have something like
selectFirst()
andselectLast()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selecting first and last is not explicitly needed. Do you think we need to add it?