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 force-pushed the rail branch 2 times, most recently from 0016a83 to ed8edad Feb 21, 2021
@perezdidac perezdidac marked this pull request as ready for review Feb 21, 2021
src/rail_gui.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@stormcone stormcone left a comment

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 label Feb 21, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 Feb 21, 2021 Inactive
@stormcone
Copy link
Contributor

@stormcone stormcone commented Feb 22, 2021

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 had a problem deploying to preview-pr-8706 Feb 22, 2021 Failure
@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Feb 22, 2021

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 had a problem deploying to preview-pr-8706 Feb 22, 2021 Failure
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 Feb 22, 2021 Inactive
@perezdidac perezdidac requested a review from LordAro Feb 22, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8706 Feb 24, 2021 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 Mar 3, 2021 Inactive
@perezdidac perezdidac requested a review from LordAro Mar 4, 2021
@stormcone
Copy link
Contributor

@stormcone stormcone commented Mar 7, 2021

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

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Mar 7, 2021

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

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
@TrueBrain TrueBrain merged commit e708fb3 into OpenTTD:master Mar 13, 2021
12 checks passed
@Kuhnovic
Copy link
Contributor

@Kuhnovic Kuhnovic commented Mar 19, 2021

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

@Kuhnovic Kuhnovic commented Mar 19, 2021

Good point, I'll do it right away!

@stormcone
Copy link
Contributor

@stormcone stormcone commented Mar 19, 2021

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants