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

LGM-Doyle · 2017-02-21T19:50:18Z

This PR fixes issue #1199.

The problem was that the sitrep rows change their height in their prerender after the decision to add/remove the vertical scroll bar is made, because it changes the size of the client area.

In general this problem could be unstable. Every change to the scroll bars changes the client area, which changes the row sizes which changes the scroll bar requirements, closing the feedback loop.

The solution, I've employed is to try twice and then force the scroll bar to be added.

…der.

…ded.

This fixes the following problem. 1. Some list rows can change size when prerendered. 2. Adding/removing scrolls can change the size of the list box client area. This can result in an unstable situation where the scrolls are added or removed and the row size changes in a way that requires the opposite change in the scrolls. This commit basically tries twice and then forces the scrolls to be added.

geoffthemedio · 2017-02-21T20:21:15Z

GG/GG/ListBox.h

+    Pt ClientSizeExcludingScrolls() const;
+    /** Return a pair of dimensions of the scollable area if vscroll and/or hscroll is required.*/
+    std::pair<boost::optional<X>, boost::optional<Y>>
+        CheckIfScrollsRequired(const std::pair<bool, bool>& force_scrolls= {false, false}) const;


geoffthemedio · 2017-02-21T20:21:44Z

GG/GG/ListBox.h

@@ -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.*/


this comment is unclear to me

Expanded comment.

geoffthemedio · 2017-02-21T20:23:08Z

GG/GG/ListBox.h

+        CheckIfScrollsRequired(const std::pair<bool, bool>& force_scrolls= {false, false}) const;
+    /** 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);


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

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.

geoffthemedio · 2017-02-21T20:23:39Z

GG/src/ListBox.cpp

-    // 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


prefer single space after .

geoffthemedio · 2017-02-21T20:25:15Z

GG/src/ListBox.cpp

+    // Perform a cycle of adjust scrolls and prerendering rows and return if sizes changed.
+    auto check_adjust_scroll_size_change =
+        [this]
+        (std::pair<bool,bool> force_scrolls = {false, false})


space after ,

geoffthemedio · 2017-02-21T20:27:04Z

GG/src/ListBox.cpp

+Pt ListBox::ClientSizeExcludingScrolls() const
+{
+    // this client area calculation disregards the thickness of scrolls
+    Pt cl_sz = (LowerRight() - Pt(X(BORDER_THICK), Y(BORDER_THICK))) -


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

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

geoffthemedio · 2017-02-21T20:28:15Z

GG/src/ListBox.cpp

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


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?

I clarified the comment.

…orion#1356. 1. Improved threading of partial results through the components. 2. Improved commenting of code.

LGM-Doyle

These changes were more than just addressing the concerns. I changed/fixed how the partial results were threaded through the AdjustScrolls() function.

LGM-Doyle · 2017-02-22T10:39:23Z

GG/GG/ListBox.h

@@ -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.*/


Expanded comment.

LGM-Doyle · 2017-02-22T11:24:49Z

GG/GG/ListBox.h

+    Pt ClientSizeExcludingScrolls() const;
+    /** Return a pair of dimensions of the scollable area if vscroll and/or hscroll is required.*/
+    std::pair<boost::optional<X>, boost::optional<Y>>
+        CheckIfScrollsRequired(const std::pair<bool, bool>& force_scrolls= {false, false}) const;


LGM-Doyle · 2017-02-22T11:49:51Z

GG/GG/ListBox.h

+        CheckIfScrollsRequired(const std::pair<bool, bool>& force_scrolls= {false, false}) const;
+    /** 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);


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.

LGM-Doyle · 2017-02-22T11:52:47Z

GG/src/ListBox.cpp

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


I clarified the comment.

LGM-Doyle · 2017-02-22T11:53:48Z

GG/src/ListBox.cpp

+Pt ListBox::ClientSizeExcludingScrolls() const
+{
+    // this client area calculation disregards the thickness of scrolls
+    Pt cl_sz = (LowerRight() - Pt(X(BORDER_THICK), Y(BORDER_THICK))) -


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

LGM-Doyle · 2017-02-22T16:40:35Z

GG/src/ListBox.cpp

+    // Perform a cycle of adjust scrolls and prerendering rows and return if sizes changed.
+    auto check_adjust_scroll_size_change =
+        [this]
+        (std::pair<bool,bool> force_scrolls = {false, false})


adrianbroher · 2017-02-22T16:43:43Z

GG/src/ListBox.cpp

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


Please put the capture in front of the parameter list.

geoffthemedio · 2017-02-24T19:41:31Z

GG/src/ListBox.cpp

+    auto check_adjust_scroll_size_change =
+        [this](std::pair<bool, bool> force_scrolls = {false, false})
+        {
+            // This adjust scrolls may add or remove scrolls


Seems like unnecessary double indentation

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.

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

I've changed the indenting.

…orion#1356. 1. Improved threading of partial results through the components. 2. Improved commenting of code.

LGM-Doyle added 4 commits February 21, 2017 14:18

Break ListBox::AdjustScrolls into component functions.

474cbff

Make ListBox::ShowVisibleRows report size changes when forcing preren…

8c1b055

…der.

Allow ListBox::AdjustScrolls to force one/both of scroll bar to be ad…

2fb4d05

…ded.

LGM-Doyle added category:bug The Issue/PR describes or solves a perceived malfunction within the game. component:UI The Issue/PR deals with the game user interface, graphical or other. labels Feb 21, 2017

LGM-Doyle added this to the v0.4.7 milestone Feb 21, 2017

geoffthemedio reviewed Feb 21, 2017

View reviewed changes

Changes to ListBox::AdjustScrolls component functions for review free…

21ba245

…orion#1356. 1. Improved threading of partial results through the components. 2. Improved commenting of code.

LGM-Doyle commented Feb 22, 2017

View reviewed changes

adrianbroher suggested changes Feb 22, 2017

View reviewed changes

Style change for review freeorion#1356.

10753fb

geoffthemedio reviewed Feb 24, 2017

View reviewed changes

Change indenting for review freeorion#1356.

af968ac

geoffthemedio merged commit 9499a52 into freeorion:master Feb 26, 2017

Vezzra added the status:merged All relevant commits of this PR were merged into the master development branch. label Feb 26, 2017

Vezzra mentioned this pull request Feb 26, 2017

Sitrep list lacks a srollbar when the last item doesn't fit the window #1199

Closed

MatGB pushed a commit to MatGB/freeorion that referenced this pull request Feb 28, 2017

Changes to ListBox::AdjustScrolls component functions for review free…

ce42dc8

…orion#1356. 1. Improved threading of partial results through the components. 2. Improved commenting of code.

MatGB pushed a commit to MatGB/freeorion that referenced this pull request Feb 28, 2017

Style change for review freeorion#1356.

5267d3f

MatGB pushed a commit to MatGB/freeorion that referenced this pull request Feb 28, 2017

Change indenting for review freeorion#1356.

85628f7

LGM-Doyle deleted the fix_issue_1199_sitrep_list_lacks_scroll_bar_for_last_item branch March 11, 2017 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LGM-Doyle commented Feb 21, 2017

geoffthemedio Feb 21, 2017

LGM-Doyle Feb 22, 2017

geoffthemedio Feb 21, 2017

LGM-Doyle Feb 22, 2017

geoffthemedio Feb 21, 2017

LGM-Doyle Feb 22, 2017

geoffthemedio Feb 21, 2017

geoffthemedio Feb 21, 2017

LGM-Doyle Feb 22, 2017

geoffthemedio Feb 21, 2017

LGM-Doyle Feb 22, 2017

geoffthemedio Feb 21, 2017

LGM-Doyle Feb 22, 2017

LGM-Doyle left a comment

LGM-Doyle Feb 22, 2017

LGM-Doyle Feb 22, 2017

LGM-Doyle Feb 22, 2017

LGM-Doyle Feb 22, 2017

LGM-Doyle Feb 22, 2017

LGM-Doyle Feb 22, 2017

adrianbroher Feb 22, 2017

LGM-Doyle Feb 23, 2017

geoffthemedio Feb 24, 2017

LGM-Doyle Feb 24, 2017

geoffthemedio Feb 24, 2017

LGM-Doyle Feb 25, 2017

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

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

Conversation

LGM-Doyle commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LGM-Doyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment