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

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

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.

@perezdidac
Copy link
Contributor Author

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 pull request 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 pull request 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 commented Apr 6, 2021

What's the advantage to sorting by upload date?

@perezdidac
Copy link
Contributor Author

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

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

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.

@2TallTyler
Copy link
Member

I think this is still worth considering, but it needs a rebase. 🙂

@2TallTyler 2TallTyler added the work: needs rebase This Pull Request needs a rebase label Nov 9, 2022
@TrueBrain
Copy link
Member

I think we should just do this. Not the sorting, as that will give annoying problems with people rating newer content as better than older, etc. But having the information in there, should lovely.

I wouldn't call it upload date, but rather last updated. Although internally it is ofc the last upload date, but using last updated give more the illusion that there is one item, with multiple versions. Instead of considering every version its own entity.

And yes, this needs a chain of changes in other services too, which all have open PRs, but just untouched for .. 2 years? now (sorryyyyyyyyyy). But in general, I think this is just a nice addition.

@TrueBrain TrueBrain added the candidate: yes This Pull Request is a candidate for being merged label Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged work: needs rebase This Pull Request needs a rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants