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

Live Replay Setting #3230

Conversation

sulikdan
Copy link
Contributor

@sulikdan sulikdan commented Aug 7, 2024

Should solve issue #3093 ,
though I highly doubt it was working before.

@Sheikah45 Is it to your liking? While I was going through it thought that some refactoring is needed.

Currently there is VaultPrefs - Which contains:

  • ReplaySearchPrefs
  • LiveReplaySearchPrefs ( newly created for this feature)

I see that VaultPrefs were firstly created then ReplaySea.. was added, though I'm not sure myself to which screen belongs which as there is FAF Replay and Local Replay (could debug it). Anyway, I think It would be good to get them all in the same level.

And when I would be doing it, I've have seen that there are prefs for filters in CustomGame(Filter)Controller , it could be also somehow nested and could increase readability.

The question is - should I proceed? Or are you fine with it being this way

@Sheikah45
Copy link
Member

The problem with changing the nesting structure is that the Preferences POJO is serialiized and deserialized directly to and from the client.prefs file. So changing the nesting structure would break people's current preferences that are saved on their machines. Towards this end we do have the migrate preferences function in preferencesConfig which can be used for updating the new preferences from the old preferences structure. I have just been too lazy to do this in the past XD. That also means that you cannot simply just move objects but have to have a period where the old location is deprecated and the new location is used and the migrate function translates between the two.

@sulikdan
Copy link
Contributor Author

sulikdan commented Aug 8, 2024

The problem with changing the nesting structure is that the Preferences POJO is serialiized and deserialized directly to and from the client.prefs file. So changing the nesting structure would break people's current preferences that are saved on their machines. Towards this end we do have the migrate preferences function in preferencesConfig which can be used for updating the new preferences from the old preferences structure. I have just been too lazy to do this in the past XD. That also means that you cannot simply just move objects but have to have a period where the old location is deprecated and the new location is used and the migrate function translates between the two.

That hurts, I didn't realize it, but it will be needed, cos its starting to be real mess, it took me a while to get sorted in it.
Gonna do fixes + fix tests, then will notify you. Ty for information

@sulikdan
Copy link
Contributor Author

sulikdan commented Aug 12, 2024

@Sheikah45 Could you please, help me out with tests?
I fail already at set-up

class com.faforever.client.filter.FilterTextFieldController cannot be cast to class com.faforever.client.filter.FilterCheckboxController

but i have no Idea why it is throwing this as it should pass FilterCheckbox.. not FilterText... I already burned like 3 days of commenting thing in and out ...

@Sheikah45
Copy link
Member

I can take a deeper look later, but one thing I know is that injectMocks can sometimes be flaky when you have two parameters that are the same type so we may need to just construct the object manually ourselves.

@sulikdan
Copy link
Contributor Author

Finally fixed test, wouldn't believe that replacing

@mock
for
@SPY
would solve issue, not in theory makes sense ... but still surprised.

No test added, not sure if needed as it is all was implemented inside inherited method. And implementation is same as everywhre(? at least CustomGameFilterController )

@sulikdan sulikdan requested a review from Sheikah45 August 14, 2024 18:55
Copy link
Member

@Sheikah45 Sheikah45 left a comment

Choose a reason for hiding this comment

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

Why did all the fields change to protected in the test?

Other than that looks good

@sulikdan
Copy link
Contributor Author

Why did all the fields change to protected in the test?

Uff, ehm, I was playing around and wanted to do some inheritance, but gave it up and then thought it is not much of difference protected vs private, but will clean it in evening. My bad.

@sulikdan
Copy link
Contributor Author

Encapsulation members reverted.

@Sheikah45 Sheikah45 force-pushed the feature/3093-live-games-replays-remember-selection branch from 065d050 to 2fb7346 Compare August 15, 2024 13:55
@Sheikah45 Sheikah45 enabled auto-merge (squash) August 15, 2024 13:56
@Sheikah45 Sheikah45 merged commit 30d9c13 into FAForever:develop Aug 15, 2024
1 of 2 checks passed
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.

2 participants