Skip to content

Commit

Permalink
GUI2 MP lobby: fix the game list jumping to top when a new game starts
Browse files Browse the repository at this point in the history
The fix is to restore the previous scroll position after the window is
laid out as a result of the new list item.

I also demoted one log message from info to debug level. It was way too
frequent for the info level.
  • Loading branch information
jyrkive committed Oct 8, 2016
1 parent 0d4e0b0 commit 1935829
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 18 deletions.
2 changes: 1 addition & 1 deletion src/gui/widgets/control.cpp
Expand Up @@ -428,7 +428,7 @@ void tcontrol::definition_load_configuration(const std::string& control_type)
{
// TODO: Some widgets (toggle panel, toggle button) have a variable canvas count which is determined by its definition.
// I think we should remove the canvas_count from tcontrols constructor and always read it from the definition.
LOG_GUI_L << "Corrected canvas count to " << config()->state.size();
DBG_GUI_L << "Corrected canvas count to " << config()->state.size() << std::endl;
canvas() = std::vector<tcanvas>(config()->state.size());
}
for(size_t i = 0; i < canvas().size(); ++i) {
Expand Down
32 changes: 15 additions & 17 deletions src/gui/widgets/listbox.cpp
Expand Up @@ -35,6 +35,8 @@

#include "utils/functional.hpp"

#include <boost/optional.hpp>

#define LOG_SCOPE_HEADER get_control_type() + " [" + id() + "] " + __func__
#define LOG_HEADER LOG_SCOPE_HEADER + ':'

Expand Down Expand Up @@ -313,26 +315,22 @@ bool tlistbox::update_content_size()

void tlistbox::place(const tpoint& origin, const tpoint& size)
{
boost::optional<unsigned> vertical_scrollbar_position, horizontal_scrollbar_position;
// Check if this is the first time placing the list box
if (get_origin() != tpoint{-1, -1})
{
vertical_scrollbar_position = get_vertical_scrollbar_item_position();
horizontal_scrollbar_position = get_horizontal_scrollbar_item_position();
}

// Inherited.
tscrollbar_container::place(origin, size);

/**
* @todo Work-around to set the selected item visible again.
*
* At the moment the listboxes and dialogs in general are resized a lot as
* work-around for sizing. So this function makes the selected item in view
* again. It doesn't work great in all cases but the proper fix is to avoid
* resizing dialogs a lot. Need more work later on.
*/
const int selected_item = generator_->get_selected_item();
if(selected_item != -1) {
const SDL_Rect& visible = content_visible_area();
SDL_Rect rect = generator_->item(selected_item).get_rectangle();

rect.x = visible.x;
rect.w = visible.w;

show_content_rect(rect);
if (vertical_scrollbar_position && horizontal_scrollbar_position)
{
LOG_GUI_L << LOG_HEADER << " restoring scroll position" << std::endl;
set_vertical_scrollbar_item_position(*vertical_scrollbar_position);
set_horizontal_scrollbar_item_position(*horizontal_scrollbar_position);
}
}

Expand Down
16 changes: 16 additions & 0 deletions src/gui/widgets/scrollbar_container.cpp
Expand Up @@ -1004,6 +1004,22 @@ void tscrollbar_container::set_vertical_scrollbar_item_position(
scrollbar_moved();
}

unsigned tscrollbar_container::get_horizontal_scrollbar_item_position() const
{
assert(horizontal_scrollbar_);

return horizontal_scrollbar_->get_item_position();
}

void tscrollbar_container::set_horizontal_scrollbar_item_position(
const unsigned position)
{
assert(horizontal_scrollbar_);

horizontal_scrollbar_->set_item_position(position);
scrollbar_moved();
}

void tscrollbar_container::scroll_vertical_scrollbar(
const tscrollbar_::tscroll scroll)
{
Expand Down
13 changes: 13 additions & 0 deletions src/gui/widgets/scrollbar_container.hpp
Expand Up @@ -194,6 +194,19 @@ class tscrollbar_container : public tcontainer_
*/
void set_vertical_scrollbar_item_position(const unsigned position);

/**
* Returns current position of the horizontal scrollbar.
*
*/
unsigned get_horizontal_scrollbar_item_position() const;

/**
* Move the horizontal scrollbar to a position.
*
* @param position The position to scroll to.
*/
void set_horizontal_scrollbar_item_position(const unsigned position);

/**
* Scrolls the vertical scrollbar.
*
Expand Down

4 comments on commit 1935829

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that this would push the currently-selected item down (potentially offscreen) if the new item happens to be above it, unless I'm missing something?

@Vultraz
Copy link
Member

@Vultraz Vultraz commented on 1935829 Oct 8, 2016

Choose a reason for hiding this comment

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

This commit also seems to make it so the initially selected rows aren't scrolled to (see MP Create and the initial selection).

@jyrkive
Copy link
Member Author

@jyrkive jyrkive commented on 1935829 Oct 9, 2016

Choose a reason for hiding this comment

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

@CelticMinstrel To be honest, I don't even care about that. Most lists in Wesnoth are static. For the few dynamic lists we have, it's not a big deal that the selected item may scroll offscreen when the list changes.

@Vultraz You're right. Fixed in 41a49f7

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

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

Well, maybe the few dynamic lists we have can be made to correct for it, as well. Especially if the items are of roughly constant height.

Please sign in to comment.