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

Fix #6599: Can still click on buy button in vehicle selection window even if no vehicle is selected #7049

Closed
wants to merge 2 commits into from

Conversation

Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Jan 12, 2019

Patches imported as PR
not implemented or tested by me
minor conflict resolved
Fixes #6599

@nielsmh nielsmh changed the title 6599 Fix #6599: Can still click on buy button in vehicle selection window even if no vehicle is selected Jan 12, 2019
@andythenorth
Copy link
Contributor

andythenorth commented Jan 12, 2019

Tested, solves the issues described in #6599, needs a review.

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.

Can't see anything wrong with the code
Can I be annoying and ask you to capitalise the commit messages properly? (and also fix typos) :>

src/build_vehicle_gui.cpp Outdated Show resolved Hide resolved
@Eddi-z Eddi-z force-pushed the 6599 branch 2 times, most recently from 9f521b6 to 4aaed36 Compare January 27, 2019 19:15
@@ -93,7 +94,7 @@ static const CargoID CF_NONE = CT_INVALID; ///< Show only vehicles which do not

Listing _engine_sort_last_sorting[VEH_COMPANY_END] = { { 0, false }, { 0, false }, { 0, false }, { 0, false } };
bool _engine_sort_show_hidden_engines[VEH_COMPANY_END] = {false, false, false, false}; ///< Last set 'show hidden engines' setting for each vehicle type.
static CargoID _engine_sort_last_cargo_criteria[VEH_COMPANY_END] = {CF_ANY, CF_ANY, CF_ANY, CF_ANY}; ///< Last set filter criteria, for each vehicle type.
static byte _engine_sort_last_cargo_criteria[VEH_COMPANY_END] = {CF_ANY, CF_ANY, CF_ANY, CF_ANY}; ///< Last set filter criteria, for each vehicle type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure what this change is doing here

Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

The first two changes seem fine, but the remaining four codechanges are fishy and seem to make changes then undo/rework them right afterwards. Can we get just the functional/bugfixing patches in alone, then look at the code quality ones separately?

@PeterN
Copy link
Member

PeterN commented Feb 3, 2019

In #6599, this is a problem:

I have something for this, and more.

The sortlist changes might be okay but they're nothing to do with disabling the Buy button.

@PeterN
Copy link
Member

PeterN commented Feb 22, 2019

Already fixed by #7217

@PeterN PeterN closed this Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can still click on buy button in vehicle selection window even if no vehicle is selected
5 participants