-
Notifications
You must be signed in to change notification settings - Fork 99
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 bug when selling vehicle equipment or ammo #374
Conversation
Fixed typo in buy and sell screen. Related to: No aequipement type matching ID "VEQUIPMENTTYPE_BOLTER_4000_LASER_GUN" #353 Selling vehicle equipment/ammo resulted in selling non existing agent equipment and ammo.
Which one is correct one? My guess is VEQUIPMENTAMMOTYPE_ or are they supposed to be different prefixes? This seems to fix errors related to wrong prefixes when entering agent equipment screen. Related to: VEHICLE (VAmmoType) Errors throughout log when on AGENT equipment #367
This reverts commit 7cf76fe.
…vents - New notification if there is not enough ammo or fuel to refuel or rearm the vehicle. - Notifications when vehicle is succesfully rearmed and refuelled.
Great stuff, many thanks @Jarskih <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - if you can fix the small items (or even if you disagree with them, they're pretty minor) I'll merge it.
Thanks!
@@ -124,9 +124,9 @@ UString GameVehicleEvent::message() | |||
return tr("An illegal flyer has been detected."); | |||
} | |||
case GameEventType::NotEnoughAmmo: | |||
return tr("Not enough ammo to rearm vehicle"); | |||
return format("%s %s", tr("Not enough ammo to rearm vehicle:"), vehicle->name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably have the "%s" placeholder thingie in the tr() string - just in case other languages don't make sense having it at the end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean by this. Maybe you can change this one to fit other languages?
game/state/city/vequipment.cpp
Outdated
@@ -138,8 +139,27 @@ void VEquipment::update(int ticks) | |||
} | |||
} | |||
|
|||
void VEquipment::noAmmoToReload(GameState &state, VEquipment *equipment) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe both "gamestate" and "equipment" arguments can be const here?
(I know it's not consistently applied through the codebase right now, but I prefer "const if possible")
Actually I didn't notice the lint pass failed - we use clang-format (specifically version 4.0, the newer 5.0 is probably ok but it does some things slightly differently) to format the code according the standards we use. Hopefully the ide/platform you use has some way of integrating that (many IDEs, like visual studio or qtcreator, have plugins that can automate it and use the config in the OpenApoc git, or you can install it and run it manually with "clang-format -i path/to/file.cpp". Or in the worse case, the output of the travis "lint" CI pass is a diff file that you could apply, or just manually look over for the fixes) |
Thanks @JonnyH for review. I think I fixed all the issues now. I am not sure how to fix the translation issue you mentioned. |
Fixed typo in buy and sell screen.
Related to:
No aequipement type matching ID "VEQUIPMENTTYPE_BOLTER_4000_LASER_GUN"
#353
Selling vehicle equipment/ammo resulted in selling non existing agent
equipment and ammo.