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: rail station class name filtering #8706

Merged
merged 1 commit into from Mar 13, 2021
Merged

Conversation

perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Feb 20, 2021

Motivation / Problem

Players with many NewGRF enabled for train stations sometimes struggle at finding the right one to place in every situation. This PR introduces a filter edit box for quicker access when train station NewGRF are enabled in the game. This is especially helpful for those that care about station design and eye candy looking terminals.

Description

Add a text box to the Build Rail Station window for filtering train station classes. This is similar to the previous feature implemented in the Object Selection window here: #8603.

This is what it looks like:

image

Note that the sorting logic is based on the added NewGRF order given the concerns in #8817:

image

Limitations

No limitations.

Testing

The following test cases have been tested:

  • Re-opening the build train station window within the same game keeps the previously selected station class and station type.
  • Starting/Loading a new game resets the selected station to the default one.
  • Starting/Loading a new game with different NewGRFs.
  • Default station always at the top.

All seems to work perfectly.

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')

@perezdidac perezdidac marked this pull request as ready for review February 21, 2021 01:29
src/rail_gui.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@stormcone stormcone left a comment

Choose a reason for hiding this comment

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

Some minor code style fixes. Maybe @SamuXarick could take a look at it also. :)

@devs: Please add the preview label to this. :) (And thanks for that feature. :P)

src/rail_gui.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
@glx22 glx22 added the preview This PR is receiving preview builds label Feb 21, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 February 21, 2021 21:02 Inactive
@stormcone
Copy link
Contributor

Would it be possible that the filter textbox get automatically focused when the station building window opens? So we could type in immediately without to click on the textbox. Just like in the case of the build object window.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 February 22, 2021 01:38 Inactive
@perezdidac
Copy link
Contributor Author

Would it be possible that the filter textbox get automatically focused when the station building window opens? So we could type in immediately without to click on the textbox. Just like in the case of the build object window.

Done! All good.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 February 22, 2021 01:49 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 February 22, 2021 01:58 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 February 24, 2021 05:35 Inactive
@LordAro LordAro added this to the 1.11.0 milestone Feb 28, 2021
src/rail_gui.cpp Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 March 3, 2021 17:51 Inactive
@perezdidac perezdidac requested a review from LordAro March 4, 2021 23:46
LordAro
LordAro previously approved these changes Mar 6, 2021
@stormcone
Copy link
Contributor

I would be happy with a drop down menu with the sort orders here too. :)

@perezdidac
Copy link
Contributor Author

I would be happy with a drop down menu with the sort orders here too. :)

Yep that could be a good addition, however I believe it might be better to do it in a separate PR. I am also thinking about ´favoriting´ / ´starring´ classes or objects to being able to quickly use the most frequent ones,

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

More or less fine, afaict. Looks good!

src/rail_gui.cpp Outdated Show resolved Hide resolved
src/rail_gui.cpp Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 March 11, 2021 03:10 Inactive
@perezdidac perezdidac requested a review from LordAro March 11, 2021 03:18
@TrueBrain TrueBrain merged commit e708fb3 into OpenTTD:master Mar 13, 2021
@Kuhnovic
Copy link
Contributor

I'm playing the latest release candidate with a bunch of friends as we speak. Myself and several others are having issues with this feature. The filter works fine, but auto focus on the text field is a problem. For example, if you want to remove a station tile, you typically click the station button and hit R. But that R button is now simply adding an r to the filter string. Same thing if you press delete to clear all windows... you're deleting characters in the filter string. This really kills the whole flow of the UI, it's quite annoying.

One of my friends pointed out that the Town Directory window has a similar filter string, but this text field does not automatically get the focus. And then it works just fine. So I think removing the auto focus should do the trick.

@LordAro
Copy link
Member

LordAro commented Mar 19, 2021

You raise good points. I'd recommend raising a new issue though. Comments on closed/merged PRs tend to get lost

@Kuhnovic
Copy link
Contributor

Good point, I'll do it right away!

@stormcone
Copy link
Contributor

If you press the ESC, the focus is lost and you can use the hotkeys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants