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

(Proposed Fix) Land info build date is also renovation date #11674

Closed
wants to merge 0 commits into from

Conversation

Gadg8eer
Copy link
Contributor

@Gadg8eer Gadg8eer commented Jan 3, 2024

Motivation / Problem

Originally posted by @DorpsGek in #4415 (comment)

We could ofcourse opt to be obnoxiously specific about what the date means: "Date at which the last construction or rebuilt at this station took place". But that bloats the interface for no good reason.

We actually need to update the build date as it's used by NewGRFs and TTDPatch/OpenTTD have behaved for a decade like this, thus changing it would break quite a few NewGRFs. Even then it would be wrong given the former example.

Originally posted by me in #11123

Suggestion for "unfixable" station date "issue"

How about "Last build/renovation date" or even "Build/renovation date"? Shorter, just as accurate and not that hard to implement, unless I'm mistaken?
I get this is an old and unjustified "issue", feel free to close this immediately if the suggestion isn't useful.

When a station layout is modified in any way, the "Built: YYYY" section of the station's land info is also the renovation date. While fixing it properly would not be trivial, I propose a simpler short-term solution.

Description

Just change the basic string from "Built" to "Built/last renovated" or possibly "Most recent construction". 2TallTyler suggested "Built/renovated" to be as short as possible.

I added a minor change to the language file. Apparently I forgot to submit it.

Limitations

A more long-term solution is to record the original construction date and then also record the most recent renovation, but nothing in-between (it doesn't seem like anyone needs a history of the whole station).

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 touches english.txt or translations? Check the guidelines
  • 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, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

Thanks for this. A couple things to fix. 😃

You'll need to fix your commit message, following the guidelines in CONTRIBUTING. There should only be one commit, for which I suggest Fix #11123: Land info build date is also renovation date.

Please update the PR title to match the commit title, and write something in Motivation/Problem and Description to tell us what the problem was and how you solved it. Some of this can be copy-and-pasted from #11123 -- we just want it in one place to be easily findable. 🙂

You'll also need to get rid of the two merge commits (which if you click on them, you'll notice don't actually add anything). Here's an article on how to do that. In this case, the commands are:
git reset --hard 8dc4334
git push -f

Finally, a tip for the future: You want to open PRs from your branch where you made the change ( Gadg8eer:langfix), not from the master branch of your fork, which should never diverge from OpenTTD:master. Otherwise, the next time you want to make a new branch from your fork's master branch, you'll have this extra commit floating around. 😉

@@ -3010,7 +3010,7 @@ STR_LAND_AREA_INFORMATION_RAIL_OWNER :{BLACK}Railway
STR_LAND_AREA_INFORMATION_LOCAL_AUTHORITY :{BLACK}Local authority: {LTBLUE}{STRING1}
STR_LAND_AREA_INFORMATION_LOCAL_AUTHORITY_NONE :None
STR_LAND_AREA_INFORMATION_LANDINFO_COORDS :{BLACK}Coordinates: {LTBLUE}{NUM} x {NUM} x {NUM} ({RAW_STRING})
STR_LAND_AREA_INFORMATION_BUILD_DATE :{BLACK}Built: {LTBLUE}{DATE_LONG}
STR_LAND_AREA_INFORMATION_BUILD_DATE :{BLACK}Built/last renovated: {LTBLUE}{DATE_LONG}
Copy link
Member

Choose a reason for hiding this comment

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

To be even more succinct, maybe:

Suggested change
STR_LAND_AREA_INFORMATION_BUILD_DATE :{BLACK}Built/last renovated: {LTBLUE}{DATE_LONG}
STR_LAND_AREA_INFORMATION_BUILD_DATE :{BLACK}Built/renovated: {LTBLUE}{DATE_LONG}

@Gadg8eer
Copy link
Contributor Author

Gadg8eer commented Jan 6, 2024

Thanks for this. A couple things to fix. 😃

You'll need to fix your commit message, following the guidelines in CONTRIBUTING. There should only be one commit, for which I suggest Fix #11123: Land info build date is also renovation date.

Please update the PR title to match the commit title, and write something in Motivation/Problem and Description to tell us what the problem was and how you solved it. Some of this can be copy-and-pasted from #11123 -- we just want it in one place to be easily findable. 🙂

You'll also need to get rid of the two merge commits (which if you click on them, you'll notice don't actually add anything). Here's an article on how to do that. In this case, the commands are: git reset --hard 8dc4334 git push -f

Finally, a tip for the future: You want to open PRs from your branch where you made the change ( Gadg8eer:langfix), not from the master branch of your fork, which should never diverge from OpenTTD:master. Otherwise, the next time you want to make a new branch from your fork's master branch, you'll have this extra commit floating around. 😉

This is all fair and I really want to follow through but I'm having a bit of trouble; I have been exclusively using the github web portal for all the things I've been doing, and since I'm banned from the forums and the discord, I have no way to ask anyone else for assistance on where the commands you posted are supposed to actually be entered. I do know enough that the cmd program is involved and those basics, but how do I "connect" to my fork using commands exactly? Even a search engine phrase to look it up myself would be useful, unfortunately I am not finding google very reliable at locating beginner's info these days (it could be me but I've noticed other people complaining that searches are not providing good results either). Thanks in advance.

Also, should I be doing the commands first and then implementing changes despite the order you listed them? Or did you have that in the exact order? Or will either way do?

@Gadg8eer Gadg8eer changed the title Patch for issue #11123 Fix #11123: Land info build date is also renovation date Jan 7, 2024
@2TallTyler
Copy link
Member

Ah, the web portal is very limited. I don't think you can contribute much within those limitations.

The place to start would be CONTRIBUTING#pull-requests. This assumes you have git installed and configured, then walks you through connecting your fork.

This change doesn't really require testing, but if you do anything that does, COMPILING will also be useful. 🙂

@Gadg8eer
Copy link
Contributor Author

Ah, the web portal is very limited. I don't think you can contribute much within those limitations.

The place to start would be CONTRIBUTING#pull-requests. This assumes you have git installed and configured, then walks you through connecting your fork.

This change doesn't really require testing, but if you do anything that does, COMPILING will also be useful. 🙂

Is GitHub Desktop any better? Also I've already closed this pull request and done my best to give myself a fresh "master" branch and a fresh "langfix" branch, so should I make a new pull request?

@Gadg8eer Gadg8eer changed the title Fix #11123: Land info build date is also renovation date (Proposed Fix) Land info build date is also renovation date Jan 10, 2024
@2TallTyler
Copy link
Member

I haven't used GitHub Desktop so I don't know what it can and can't do. It took me a while to get used to the command line but it was well worth it.

Yes, it's probably easiest to make a new PR.

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.

None yet

2 participants