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.

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

Feature: make NewGRF active list react on key presses #9011

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

perezdidac
Copy link
Contributor

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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

src/newgrf_gui.cpp Outdated Show resolved Hide resolved

if (this->vscroll2->UpdateListPositionOnKeyPress(active_pos, keycode) == ES_NOT_HANDLED) return ES_NOT_HANDLED;
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already guaranteed by UpdateListPositionOnKeyPress.


active_pos = std::min(active_pos, active_count);

if (this->actives != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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().

@TrueBrain
Copy link
Member

Any chance you can process the above comments and update this PR? :)

@2TallTyler 2TallTyler added preview This PR is receiving preview builds work: minor details This Pull Request has some minor details left to do labels Nov 26, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-9011 November 26, 2022 17:50 Inactive
@TrueBrain TrueBrain removed the preview This PR is receiving preview builds label Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work: minor details This Pull Request has some minor details left to do
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants