Skip to content

Conversation

@ABaumher
Copy link
Collaborator

@ABaumher ABaumher commented Jul 4, 2023

Marking this as a draft - it's the current release made public, but it's not as tested as i'd like. Includes fixes from @don-de-marco and @urwrstkn8mare. Most of these are aimed at getting the plugin to run well with large libraries, etc. Tagged in Don-De-Marco because it's largely their code, so any questions are best sent there.

I just want to get a review going so the later merges won't be so painful. Most of these changes are how we cache data, with one exception being how we handle OS-Dependant code of launching games, file sizes, etc.

ABaumher and others added 30 commits May 16, 2023 14:26
… being snake_case instead of PascalCase so i need to check this and git diff is easiest.
…rks properly. now properly type hints all messages betterproto generates.
…entication cache since it's not a cache. fixed imports broken by better proto i missed earlier
…illegal but confusing af. hopefully fixed that issue with the protobufs. Updates protobuf to 22.0
…ient. Actually fixed the enums error. idk how it got reverted.
to be able to provide defaults for message fields, betterproto inspects type hints
however, this requires the type-hinted classes to be imported at run-time, too
hence we need to remove the conditional
update the dev.txt so it doesn't cause a merge conflict. I think this branch is one behind and x ahead of my current. Technically 2 behind, but the one is only on my local repo and i can just stash and throw it out.
Added back the To_TwoFactorMethod (i think it's still used), but now with proper type hints. Properly type hinted the helper function as well.
Added a comment explaining the optimistic checking so it's more obvious why the code is written this way
updates presence to handle the lack of HasName, which means the rich_presence dictionary may not be null, but could be empty instead.
…uf_deps

betterproto all the things; also fixes cache issue
…ted out to_TwoFactorMethod as it's unused. Commented out the proto2 hack, it's not required but might be if we need to revert to the older protoc.
fix deadlock caused by ancient protobuf message
@ABaumher ABaumher marked this pull request as draft July 4, 2023 23:12
@ABaumher
Copy link
Collaborator Author

ABaumher commented Jul 4, 2023

Note that this is built off of the un-rebased version, which we might be switching back to anyway. Our current release is equivalent to d4970b3

However, some commits were merged in after this commit that were worked on before this date. It makes more sense in the Tree than it does here on github.

Here's the relative compare: https://github.com/GOG-Nebula/galaxy-integration-steam/compare/d4970b3..d59d893
...And it's still a ton of files. GG. Once again, most of the files are proto related, because once one thing changes there it updates like 40 files. We're now using betterproto so every file gets updated. Unfortunately, that updates any code that refers to those protobuf files as well, and that quickly propegates.

I should rebase out each fix so the changes are a bit more incremental, but i don't have as good of control over this process as i should, i guess.

Copy link
Member

@urwrstkn8mare urwrstkn8mare left a comment

Choose a reason for hiding this comment

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

please fix/update current_version.json, manifest.json, and version.py. btw the version number should be smth like 2.0.0-beta (https://semver.org/). the descriptions and stuff should also be more consistent. just before merging it into master we can change the version number to 2.0.0

other than that this looks good to me 👍

i do have write perms for dev as well. if you would like i can contribute directly to the dev branch and make changes but i dont wanna step on any toes so i'll ask first.

@ABaumher
Copy link
Collaborator Author

ABaumher commented Jul 7, 2023

You're good to make changes. Ngl the less work i have to do the happier i am lol. Not enough hours in the day as it is.

@UncleGoogle
Copy link

After a few days, I'm afraid I won't find time to review this this week. The amount of changes overwhelms. I can try next week

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.

4 participants