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

Attempt to repair the station each time the primary formspec is opened #54

Closed
wants to merge 2 commits into from

Conversation

oversword
Copy link
Collaborator

This PR runs the repair action each time the form is opened, except when it has not yet been configured.

I also fixed the repair logic and added in a condition to stop the max network count being exceeded, or a duplicate network being added. Any error when re-attaching the station will be reported via chat, and the primary formspec shown anyway so the player can remove the station if needed.

In my opinion #52 (adding a repair button) is much preferred because:

  • Only someone with permissions can run the repair logic
  • The repair logic is run intentionally, not all the time
  • Formspec error messages can be used, instead of a mix of formspec & chat

All these things improve the user experience of re-attaching the network

@S-S-X
Copy link
Member

S-S-X commented Feb 22, 2022

Only someone with permissions can run the repair logic

Use travelnet.allow_attach. API function is currently in bit bad shape as it requires knowledge of internal implementation but check itself is very simple, bit surprised why it is not already here.

The repair logic is run intentionally, not all the time

Should be run when network is broken.
Best situation would be if networks would not be broken from beginning, automatic repair simply makes it look like that until there's conflict (you're handling conflicts right?).

Formspec error messages can be used, instead of a mix of formspec & chat

If wanted this can be redone to use formspec error messages.

@oversword
Copy link
Collaborator Author

Use travelnet.allow_attach. API function is currently in bit bad shape as it requires knowledge of internal implementation but check itself is very simple, bit surprised why it is not already here.

That's not the original behaviour, I thought you wanted me to re-implement the original behaviour exactly?

Should be run when network is broken. Best situation would be if networks would not be broken from beginning, automatic repair simply makes it look like that until there's conflict (you're handling conflicts right?).

There is a condition that checks if it's broken, but that's not the point. What if you want to leave it (temporarily) broken so you can go and fix the network elsewhere? Doing it automatically for the player could cause confusion in the event of data loss. Fixing it automatically on interaction will not make it look like networks aren't broken, if more than one station is lost then when you interact with a particular station it will look like the other stations are simply gone without knowing why - "THIS station looks fine (because it repaired itself), so why are the others missing?" If instead we informed the player and got them to manually fix the problem, they would know they have to go and fix it for other stations too.

If wanted this can be redone to use formspec error messages.

No, it can't. As I mentioned on the other issue this could cause some travelnets to become inoperable

#52

doing this on every formspec open could cause the travelnet to become inoperable: if you had a network that lost a station somehow, then added more stations to reach the limit, then tried to repair a station while opening the formspec you would instead get an error about the max station count, meaning you could never remove the station because you can't open the formspec to do so.

@S-S-X
Copy link
Member

S-S-X commented Feb 22, 2022

I guess I'll have to do implementation to explain what my proposal is, I can try to find time for that. Probably easier to understand than trying to explain what behavior I'm thinking. Carefully reading comments on #52 should however give basic idea.

@BuckarooBanzay
Copy link
Member

bump! is this relevant? with the move to modstorage as backend i don't think this is needed 🤔

@BuckarooBanzay BuckarooBanzay added the invalid This doesn't seem right label Oct 27, 2023
@S-S-X
Copy link
Member

S-S-X commented Oct 27, 2023

If everything for server side network lookup and box lookup is in the db then this shouldn't be needed, it was anyway more about temporary workaround to make things seem fine and hopefully restore after certain problems with data storages.

This also isn't really a good way for client side stuff / restoring from crippled / partial backup. For that I'm pretty sure either configurable LBM or asking for actual DB would probably be best options. Both can be completely isolated and allow complete db reconstruction in special cases where it is needed.

I guess there's already solution for tn-box removal that skips normal callbacks and db writes or was it added back after refactoring things? If it wasn't yet restored then that part could be actually useful now.

@BuckarooBanzay
Copy link
Member

I guess there's already solution for tn-box removal that skips normal callbacks and db writes or was it added back after refactoring things? If it wasn't yet restored then that part could be actually useful now.

i don't think we have such a thing yet 🤔

@Niklp09
Copy link
Member

Niklp09 commented Jul 15, 2024

Closing since this is outdated and marked as invalid.
As always, re-open if needed.

@Niklp09 Niklp09 closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants