Feature: make NewGRF active list react on key presses #9011
Conversation
|
|
||
| if (this->vscroll2->UpdateListPositionOnKeyPress(active_pos, keycode) == ES_NOT_HANDLED) return ES_NOT_HANDLED; |
There was a problem hiding this comment.
This should refer to vscroll not vscroll2 as now it's basing the logic on the content of the wrong scrollbar.
|
|
||
| if (this->vscroll2->UpdateListPositionOnKeyPress(active_pos, keycode) == ES_NOT_HANDLED) return ES_NOT_HANDLED; | ||
|
|
||
| active_pos = std::min(active_pos, active_count); |
There was a problem hiding this comment.
This is already guaranteed by UpdateListPositionOnKeyPress.
|
|
||
| active_pos = std::min(active_pos, active_count); | ||
|
|
||
| if (this->actives != nullptr) { |
There was a problem hiding this comment.
The if-statement has no real effect. When actives is null, and you get here you have already clicked in the scrolled area so the active selection is already on 0. I would just remove the if and always run the code below.
|
|
||
| if (this->actives != nullptr) { | ||
| this->SelectActive(active_pos); | ||
| this->vscroll->ScrollTowards(active_pos == active_count - 1 ? active_pos + 1 : active_pos); |
There was a problem hiding this comment.
I would not deviate the behavior here from all the other behavior, so passing only active_pos should be fine as well. I find it even odd that it jumps two places for the last. I would for example expect that a page-down shows me the next N elements, but with this logic you can completely miss an element.
If I got 13 NewGRFs in the list and start with the minimum window size at the top, I see 1-6. After page down I see 2-7 and after another page down I see. 9-13(+1). In other words, I completely missed the NewGRF at line 8.
Removing this also removes the need to count the number of elements, and even those should already be known and stored in this->vscroll->GetCount().
|
Any chance you can process the above comments and update this PR? :) |
2TallTyler
added
preview
Motivation / Problem
Currently all key presses (up, down, home, end, page up, page down) in the NewGRF window are assumed to be used to move up or down the list of available NewGRFs. Even if the list of active NewGRFs has focus.
Description
This code change lets the user move up and down the list of active NewGRFs using the keys mentioned above, same as with the available ones.
I've made sure that when reaching the last one the scrollbar goes one extra position down to keep having room for dragging stuff there.
Please make a deployment and give it a try.
Limitations
Clicking somewhere else like in the background of the window makes the list lose focus so the next keystokes will be processed by the available list.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.