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: show upload date in the network content window #8902

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Mar 28, 2021

Motivation / Problem

Upload date is present in every content in BaNaNaS but not used in OpenTTD since it's not sent to the client. OpenTTD/bananas-server#43 changes it so it gets sent.

Description

This code change makes OpenTTD show the upload date for a given NewGRF in the online content window. In addition, a column in the content list has been added to sort by upload date as well.

image

Limitations

See OpenTTD/bananas-server#43 for full details, but in particular I've focused on making sure this doesn't break older versions. Here is what I have successfully tested:

  1. Bananas returning the new field, OpenTTD not knowing about it -> OpenTTD ignores it as it has read the latest field it knows about it (tags) and moves on to the next NewGRF. No memory leak or anything out of normal.
  2. Bananas returning the new field, OpenTTD knowing about it -> OpenTTD reads it successfully.
  3. Bananas not returning the new field, OpenTTD expecting it -> OpenTTD tries to read it but does not actually read it. I used CanReadFromPacket() to verify and make it safe in case we have to rollback Bananas and newer OpenTTD clients still want the value. finally didn't end up including this, see conversation below.

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

@LordAro
Copy link
Member

@LordAro LordAro commented Mar 28, 2021

Re: date formatting stuff. There already exists a setting for default date formats in savegame names, see date_format_in_default_names & STR_JUST_DATE_* constants. Perhaps the setting could be repurposed/renamed to be used more generally, and use the same string formatting mechanisms that are used for that?

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Mar 28, 2021

Re: date formatting stuff. There already exists a setting for default date formats in savegame names, see date_format_in_default_names & STR_JUST_DATE_* constants. Perhaps the setting could be repurposed/renamed to be used more generally, and use the same string formatting mechanisms that are used for that?

Sounds good, I'll do that, but first I need to implement a function to parse the date as it comes and convert it to a Date object. I will work on that first and once that works I'll use date_format_in_default_names to decide how to display it and potentially rename it to something more generic as you suggest. Thanks!

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Mar 29, 2021

I finally managed to make it work through passing a UNIX epoch timestamp.
It works great and backwards compatibility in case BaNaNaS rolls back or the other way around, old OpenTTD versions are not impacted.
I am still looking at how to format using the suggestions @LordAro mentioned above but I couldn't figure it out, so I left it as 2021-06-23 (ymd) for now. See screenshot.

src/network/core/packet.cpp Outdated Show resolved Hide resolved
@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 3, 2021

Hi all! I am still looking for more devs to look into it. Based on conversations with @TrueBrain they seem to agree with this but we need other devs to comment on this. Here is the PR in bananas-server that will get sent first if this moves forward: OpenTTD/bananas-server#43

perezdidac added a commit to perezdidac/bananas-api that referenced this issue Apr 5, 2021
Adding the upload-date size (4 bytes) to the packet size validation that makes sure packets don't exceed OpenTTD packet size.

This change pairs up with OpenTTD/bananas-server#43 and OpenTTD/OpenTTD#8902 which aim to pass the content upload-date to OpenTTD so it can be used to display it and for being able to sort by upload date in the network content window.
perezdidac added a commit to perezdidac/bananas-api that referenced this issue Apr 5, 2021
Adding the upload-date size (4 bytes) to the packet size validation that makes sure packets don't exceed OpenTTD packet size.

This change pairs up with OpenTTD/bananas-server#43 and OpenTTD/OpenTTD#8902 which aim to pass the content upload-date to OpenTTD so it can be used to display it and for being able to sort by upload date in the network content window.
@LordAro
Copy link
Member

@LordAro LordAro commented Apr 6, 2021

What's the advantage to sorting by upload date?

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 6, 2021

What's the advantage to sorting by upload date?

That's the question I was expecting :D

I can see a use case when as a user I want to browse random NewGRFs and know if they are fairly new or years old. In my case particularly, let's say I wanted a nice industry set to spice the game up and I get a few options, it is nice to know if it has been recently updated or the last release was 10 years ago. I am not saying old NewGRFs are implemented poorly or are bad quality or anything like that, I just think it's a feature some players will benefit from. At least to me, it's important to know I am using NewGRFs that are newer and fairly maintained.

https://bananas.openttd.org/package/newgrf seems to sort by default as well.

@LordAro
Copy link
Member

@LordAro LordAro commented Apr 6, 2021

That's all true, but I don't feel there's any benefit to actually sorting by upload date - displaying it in the general info is perfectly fine though

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 6, 2021

That's all true, but I don't feel there's any benefit to actually sorting by upload date - displaying it in the general info is perfectly fine though

Oh I remember why I added that to this PR, I did it given that #8903 was opened and deduped against this one.

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Apr 21, 2021

It's been a while. @LordAro what do you think about this feature at this point? should we move forward without the ability to sort or want to include it? I'm inclined to as the player would benefit from the ability to find latest releases.

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

4 participants