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.

Sign up for GitHub

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

Jump to bottom

Fix issue 1199 sitrep list lacks scroll bar for last item. #1356

Merged
merged 7 commits into from Feb 26, 2017
Merged

Fix issue 1199 sitrep list lacks scroll bar for last item. #1356

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
474cbff
Break ListBox::AdjustScrolls into component functions.
LGM-Doyle Feb 21, 2017
8c1b055
Make ListBox::ShowVisibleRows report size changes when forcing preren…
LGM-Doyle Feb 21, 2017
2fb4d05
Allow ListBox::AdjustScrolls to force one/both of scroll bar to be ad…
LGM-Doyle Feb 21, 2017
cede324
Change ListBox::PreRender() to handle unstable row with scroll sizes.
LGM-Doyle Feb 21, 2017
21ba245
Changes to ListBox::AdjustScrolls component functions for review #1356.
LGM-Doyle Feb 22, 2017
10753fb
Style change for review #1356.
LGM-Doyle Feb 23, 2017
af968ac
Change indenting for review #1356.
LGM-Doyle Feb 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions GG/GG/ListBox.h
View file
Open in desktop
Expand Up @@ -544,16 +544,19 @@ class GG_API ListBox : public Control
Row& ColHeaders(); ///< returns the row containing the headings for the columns, if any. If undefined, the returned heading Row will have size() 0. non-const for derivers
//@}

void AdjustScrolls(bool adjust_for_resize); ///< creates, destroys, or resizes scrolls to reflect size of data in listbox
/** creates, destroys, or resizes scrolls to reflect size of data in listbox. \p force_scroll
forces the scroll bar to be added.*/
void AdjustScrolls(bool adjust_for_resize, const std::pair<bool, bool>& force_scrolls = {false, false});

void DropsAcceptable(DropsAcceptableIter first, DropsAcceptableIter last,
const Pt& pt, Flags<ModKey> mod_keys) const override;
void HandleRowRightClicked(const Pt& pt, Flags<ModKey> mod);

private:
/** Show only rows that are within the visible list box area and hide all others. If
\p do_prerender is true then prerender the visible rows.*/
void ShowVisibleRows(bool do_prerender);
\p do_prerender is true then prerender the visible rows. Return true if prerender
resulted in any visible row changing size. */
bool ShowVisibleRows(bool do_prerender);
void ConnectSignals();
void ValidateStyle(); ///< reconciles inconsistencies in the style flags
void VScrolled(int tab_low, int tab_high, int low, int high);///< signals from the vertical scroll bar are caught here
Expand All @@ -569,6 +572,15 @@ class GG_API ListBox : public Control
/** Restore cached selected, clicked and last browsed rows.*/
void RestoreCachedSelections(const SelectionCache& cache);

/** Return the client size excluding the scroll bar sizes.*/
Pt ClientSizeExcludingScrolls() const;
/** Return a pair of dimensions of the scollable area if vscroll and/or hscroll is required.*/
Copy link
Member

Choose a reason for hiding this comment

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

this comment is unclear to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expanded comment.

std::pair<boost::optional<X>, boost::optional<Y>>
CheckIfScrollsRequired(const std::pair<bool, bool>& force_scrolls= {false, false}) const;
Copy link
Member

Choose a reason for hiding this comment

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

space b4 =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

/** Add vscroll and/or hscroll if \p required_total_extents x or y dimension exists. Return a
pair of bools indicating if added or removed vscroll or hscroll.*/
std::pair<bool, bool> AddOrRemoveScrolls(const std::pair<boost::optional<X>, boost::optional<Y>>& required_total_extents);
Copy link
Member

Choose a reason for hiding this comment

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

this seems like a rather odd function signature. why not take bools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've expanded the descriptions in the functions which hopefully clarifies this.

The client size excludes the scroll bar because it is the size used to decide is scroll bars are needed.

The extent is the full size of the underlying list box, which will be larger than the client area unless the scrolls were force on.
The extent is only meaningfull if the scroll bar is to be added.


std::list<Row*> m_rows; ///< line item data

Scroll* m_vscroll; ///< vertical scroll bar on right
Expand Down
143 changes: 109 additions & 34 deletions GG/src/ListBox.cpp
View file
Open in desktop
Expand Up @@ -853,20 +853,43 @@ void ListBox::PreRender()
NormalizeRow(row);
}

AdjustScrolls(false);

// Reset require prerender after call to adjust scrolls
Control::PreRender();
// Adding/removing scrolls and prerendering rows may change the row sizes and require a change
// in added/removed scrolls. This may not be stable. Perform two cycles and if it is not
Copy link
Member

Choose a reason for hiding this comment

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

prefer single space after .

// stable then force the scrollbar to be added if either cycle had a scroll bar.

// Perform a cycle of adjust scrolls and prerendering rows and return if sizes changed.
auto check_adjust_scroll_size_change =
[this]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put the capture in front of the parameter list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

(std::pair<bool,bool> force_scrolls = {false, false})
Copy link
Member

Choose a reason for hiding this comment

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

space after ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

{
// This adjust scrolls may add or remove scrolls
Copy link
Member

Choose a reason for hiding this comment

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

Seems like unnecessary double indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unsure of your meaning. What I see is,

auto variable_name =
    []()  // this line is indented because it is a continued line of code
    {
        AdjustScrolls()  //this  line is indented because it is the first line of code within the braced compond statement '{'
        ...
    };  // end of continue line of code.

Copy link
Member

Choose a reason for hiding this comment

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

I would de-indent everything from the { to the } line (inclusive). Right now, the actual function body is double-indented from the surrounding scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the indenting.

AdjustScrolls(true);

// Resize rows to fit client area.
X row_width(std::max(ClientWidth(), X(1)));
for (Row* row : m_rows)
row->Resize(Pt(row_width, row->Height()));
bool visible_row_size_change = ShowVisibleRows(true);

ShowVisibleRows(true);
bool header_size_change = false;
if (!m_header_row->empty()) {
auto old_size = m_header_row->Size();
GUI::PreRenderWindow(m_header_row);
header_size_change |= (old_size != m_header_row->Size());
}
return visible_row_size_change | header_size_change;
};

// Try adjusting scroll twice and then force the scrolls on.
if (check_adjust_scroll_size_change()) {
bool any_vscroll = (m_vscroll != nullptr);
bool any_hscroll = (m_hscroll != nullptr);

if (check_adjust_scroll_size_change()) {
any_vscroll |= (m_vscroll != nullptr);
any_hscroll |= (m_hscroll != nullptr);
check_adjust_scroll_size_change({any_hscroll, any_vscroll});
}
}

if (!m_header_row->empty())
GUI::PreRenderWindow(m_header_row);
// Reset require prerender after call to adjust scrolls
Control::PreRender();

// Position rows
Pt pt(m_first_row_offset);
Expand Down Expand Up @@ -946,8 +969,9 @@ void ListBox::SizeMove(const Pt& ul, const Pt& lr)
RequirePreRender();
}

void ListBox::ShowVisibleRows(bool do_prerender)
bool ListBox::ShowVisibleRows(bool do_prerender)
{
bool a_row_size_changed = false;
// Ensure that data in occluded cells is not rendered
// and that any re-layout during prerender is immediate.
Y visible_height(BORDER_THICK);
Expand All @@ -961,15 +985,19 @@ void ListBox::ShowVisibleRows(bool do_prerender)
(*it)->Hide();
} else {
(*it)->Show();
if (do_prerender)
if (do_prerender) {
auto old_size = (*it)->Size();
GUI::PreRenderWindow(*it);
a_row_size_changed |= (old_size != (*it)->Size());
}

visible_height += (*it)->Height();
if (visible_height >= max_visible_height)
hide = true;
}
}

return a_row_size_changed;
}

void ListBox::Show(bool show_children /* = true*/)
Expand Down Expand Up @@ -2122,7 +2150,18 @@ void ListBox::ValidateStyle()
m_style &= ~(LIST_NOSEL | LIST_SINGLESEL | LIST_QUICKSEL);
}

void ListBox::AdjustScrolls(bool adjust_for_resize)
Pt ListBox::ClientSizeExcludingScrolls() const
{
// this client area calculation disregards the thickness of scrolls
Copy link
Member

Choose a reason for hiding this comment

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

meaning of "disregards" is unclear. is the area smaller than the total client area by the scroll area? or is the presence or absence of scrolls irrelevant to the calculation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I clarified the comment.

Pt cl_sz = (LowerRight() - Pt(X(BORDER_THICK), Y(BORDER_THICK))) -
Copy link
Member

Choose a reason for hiding this comment

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

the way the lines are broken here is confusing. prefer keeping a single expression together with linebreaks after a ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried a different parsing so that each Pt starts its own line in the summation.

(UpperLeft() + Pt(X(BORDER_THICK), static_cast<int>(BORDER_THICK)
+ (m_header_row->empty()
? Y0
: m_header_row->Height())));
return cl_sz;
}

std::pair<boost::optional<X>, boost::optional<Y>> ListBox::CheckIfScrollsRequired(const std::pair<bool, bool>& force_scrolls) const
{
// this client area calculation disregards the thickness of scrolls
Pt cl_sz = (LowerRight() - Pt(X(BORDER_THICK), Y(BORDER_THICK))) -
Expand All @@ -2137,11 +2176,13 @@ void ListBox::AdjustScrolls(bool adjust_for_resize)
total_y_extent += row->Height();

bool vertical_needed =
force_scrolls.second ||
m_first_row_shown != m_rows.begin() ||
(m_rows.size() && (cl_sz.y < total_y_extent ||
(cl_sz.y < total_y_extent - SCROLL_WIDTH &&
cl_sz.x < total_x_extent - SCROLL_WIDTH)));
bool horizontal_needed =
force_scrolls.first ||
m_first_col_shown ||
(m_rows.size() && (cl_sz.x < total_x_extent ||
(cl_sz.x < total_x_extent - SCROLL_WIDTH &&
Expand All @@ -2161,15 +2202,30 @@ void ListBox::AdjustScrolls(bool adjust_for_resize)
total_y_extent += cl_sz.y - m_rows.back()->Height();
}

std::shared_ptr<StyleFactory> style = GetStyleFactory();
boost::optional<X> x_retval = horizontal_needed ? boost::optional<X>(total_x_extent) : boost::none;
boost::optional<Y> y_retval = vertical_needed ? boost::optional<Y>(total_y_extent) : boost::none;

return {x_retval, y_retval};
}

std::pair<bool, bool> ListBox::AddOrRemoveScrolls(
const std::pair<boost::optional<X>, boost::optional<Y>>& required_total_extents)
{
const std::shared_ptr<const StyleFactory> style = GetStyleFactory();
const Pt cl_sz = ClientSizeExcludingScrolls();

bool vscroll_added_or_removed(false);
bool horizontal_needed = (required_total_extents.first ? true : false);
X total_x_extent = required_total_extents.first ? *required_total_extents.first : X0;
bool vertical_needed = (required_total_extents.second ? true : false);
Y total_y_extent = required_total_extents.second ? *required_total_extents.second : Y0;

bool vscroll_added_or_removed = false;

// Remove unecessary vscroll
if (m_vscroll && !vertical_needed) {
vscroll_added_or_removed = true;
DeleteChild(m_vscroll);
m_vscroll = nullptr;
vscroll_added_or_removed = true;
}

// Add necessary vscroll
Expand All @@ -2185,14 +2241,6 @@ void ListBox::AdjustScrolls(bool adjust_for_resize)
}

if (vertical_needed) {
if (adjust_for_resize) {
X scroll_x = cl_sz.x - SCROLL_WIDTH;
Y scroll_y(0);
m_vscroll->SizeMove(Pt(scroll_x, scroll_y),
Pt(scroll_x + SCROLL_WIDTH,
scroll_y + cl_sz.y - (horizontal_needed ? SCROLL_WIDTH : 0)));
}

unsigned int line_size = m_vscroll_wheel_scroll_increment;
if (line_size == 0 && !this->Empty()) {
const Row* row = *begin();
Expand All @@ -2207,14 +2255,18 @@ void ListBox::AdjustScrolls(bool adjust_for_resize)
MoveChildUp(m_vscroll);
}

bool hscroll_added_or_removed = false;

// Remove unecessary hscroll
if (m_hscroll && !horizontal_needed) {
hscroll_added_or_removed = true;
DeleteChild(m_hscroll);
m_hscroll = nullptr;
}

// Add necessary hscroll
if (!m_hscroll && horizontal_needed) {
hscroll_added_or_removed = true;
m_hscroll = style->NewListBoxHScroll(m_color, CLR_SHADOW);
m_hscroll->NonClientChild(true);
m_hscroll->MoveTo(Pt(X0, cl_sz.y - SCROLL_WIDTH));
Expand All @@ -2225,14 +2277,6 @@ void ListBox::AdjustScrolls(bool adjust_for_resize)
}

if (horizontal_needed) {
if (adjust_for_resize) {
X scroll_x(0);
Y scroll_y = cl_sz.y - SCROLL_WIDTH;
m_hscroll->SizeMove(Pt(scroll_x, scroll_y),
Pt(scroll_x + cl_sz.x - (vertical_needed ? SCROLL_WIDTH : 0),
scroll_y + SCROLL_WIDTH));
}

unsigned int line_size = m_hscroll_wheel_scroll_increment;
if (line_size == 0 && !this->Empty()) {
const Row* row = *begin();
Expand All @@ -2246,8 +2290,39 @@ void ListBox::AdjustScrolls(bool adjust_for_resize)
MoveChildUp(m_hscroll);
}

return {hscroll_added_or_removed, vscroll_added_or_removed};
}

void ListBox::AdjustScrolls(bool adjust_for_resize, const std::pair<bool, bool>& force_scrolls)
{
const auto required_total_extents = CheckIfScrollsRequired(force_scrolls);

bool vscroll_added_or_removed;
std::tie(std::ignore, vscroll_added_or_removed) = AddOrRemoveScrolls(required_total_extents);

const Pt cl_sz = ClientSizeExcludingScrolls();

if (!adjust_for_resize)
return;

if (m_vscroll) {
X scroll_x = cl_sz.x - SCROLL_WIDTH;
Y scroll_y(0);
m_vscroll->SizeMove(Pt(scroll_x, scroll_y),
Pt(scroll_x + SCROLL_WIDTH,
scroll_y + cl_sz.y - (m_hscroll ? SCROLL_WIDTH : 0)));
}

if (m_hscroll) {
X scroll_x(0);
Y scroll_y = cl_sz.y - SCROLL_WIDTH;
m_hscroll->SizeMove(Pt(scroll_x, scroll_y),
Pt(scroll_x + cl_sz.x - (m_vscroll ? SCROLL_WIDTH : 0),
scroll_y + SCROLL_WIDTH));
}

// Resize rows to fit client area.
if (vscroll_added_or_removed || adjust_for_resize) {
if (vscroll_added_or_removed) {
RequirePreRender();
X row_width(std::max(ClientWidth(), X(1)));
for (Row* row : m_rows) {
Expand Down