Closed urwrstkn8mare closed 1 year ago
tagging in @don-de-marco so i can get a second set of eyes (ngl a lot of the stuff in this branch is not mine anymore so i'm not the best reviewer atm).
I'll look when i get a chance at merging this in. I'm hesitant to make this the global release because while it addresses a lot of the issues people have been having, the current version is stable for most users and this may have hidden bugs (i doubt it, our testers are annoyingly good at finding them) That said, i really should because the more people trying to break this the better.
In the meantime, lets see if i can get @bgebhardt @Mr-Spock1 to test your PR and see if they have similarly improved experiences.
After this merge i'll release this as version 1.2 Alpha and release it on FoG's issue board and see if anyone complains if/when it breaks lol.
I've got the MVC prototyped out for my general rewrite, once i have implementing the cleaner cache sorted out (a larger "container" for all caches, an abstract base class, implementing a run loop, update persistent storage, etc), i can hopefully slap these caches in there and be done 2.0
I'll give this one a test throughout the evening and see if the Windows part still does it's job. I'll keep you posted
I have an honest question because I really don't understand it. Why did this not break on Mac, too? The code is identical except for Regmon vs ContentLog in the constructor
2023-06-26 18:01:28,789 - galaxy.api.plugin - ERROR - Error while running plugin
Traceback (most recent call last):
File "C:\Users\XXX\AppData\Local\GOG.com\Galaxy\plugins\installed\steam_ca27391f-2675-49b1-92c0-896d43afa4f8\galaxy\api\plugin.py", line 1142, in create_and_run_plugin
asyncio.run(coroutine())
File "D:\obj\Windows-Release\37win32_Release\msi_python\zip_win32\runners.py", line 43, in run
File "D:\obj\Windows-Release\37win32_Release\msi_python\zip_win32\base_events.py", line 584, in run_until_complete
File "C:\Users\XXX\AppData\Local\GOG.com\Galaxy\plugins\installed\steam_ca27391f-2675-49b1-92c0-896d43afa4f8\galaxy\api\plugin.py", line 1129, in coroutine
async with plugin_class(reader, writer, token) as plugin:
File "C:\Users\XXX\AppData\Local\GOG.com\Galaxy\plugins\installed\steam_ca27391f-2675-49b1-92c0-896d43afa4f8\plugin.py", line 74, in __init__
self.local = LocalClient()
TypeError: Can't instantiate abstract class WinClient with abstract methods is_updated
hmm that seems to do with changing the baseclass to an abstract base class and the is_updated
method which is set as a property which seems to trip up ABC
.
also i accidentally left the logs in i'll take those out
can you be certain that the log file always contains information about all locally installed games
yes but i actually limit how much of the log file i'll read anyways. cause at the end of the day someone could just clear their logs. for state changes reading the log is more than enough but for getting installed games i just get all the manifests and then put the states read from the log on top
thanks for trying it out i've addressed basically all of your comments and fixed the issue. if you can please try it out again on windows and mac and lmk if it looks good to merge
What if you uninstall a game, does it still appear in the log file
yes but the last state entry is Uninstalled
so its even better. the game should appear uninstalled in gog asap. i've found its really reliable and fast (in sync with steam) but i still don't completely rely on it in my implementation. i think this could even be a good alternative to ready the registry on windows.
lgtm
Please have a second look at this. I think you by accident inverted the condition which may cause some unexpected behavior.
yes but the last state entry is Uninstalled so its even better.
That's why you read file backwards. That makes sense now.
I'll let Don-de-marco give it a test tonight and see if it passes. Then I'll see to merging it
Please have a second look at this. I think you by accident inverted the condition which may cause some unexpected behavior.
Thanks for the heads up
if you think its good probably best to use github's approve functionality
@UncleGoogle I have made the tests work on another merge request. However, that's a whole different mess to sort out and needs more work - feel free to lend a hand.
Sorry, I'll get to merging this in shortly. Different got users for work and this, has been annoying. So has switching gears from c# to python. Git credential manager made this easier with their latest release... three days ago.
Rebasing this onto master is on the list of things to do. I can't recall if i merged in master already (instead of the rebase) but if i did, mb.
Not enough hours in the day, lol
Rebasing this onto master is on the list of things to do
oh @ABaumher this wasn't built on master
it was built on better_proto_deps
resurrect tests for local stuff
😱 @UncleGoogle ur right but
I just run tests for local games on better_proto_deps and it works (after removing python paths config from pytest.ini). Imo worth to adjust them till they work
Im sorry, can't help more now -- work, children, home renovation this kind of stuff...
Well, as I said, I've already been working on getting this to work in #16 including removing obsolete tests and fixing the ones that are still relevant. I suspended the work until we (or more specifically Abaumher) finish getting the code base cleaned up and restructured. There's too much building work going on to keep the test up to par with everything. Also, a significant amount of tests doesn't work anymore since we completely overhauled the protobuf stuff as well as the authentication flow and the tests need to be adapted to the changes. I believe the local games stuff is one of the few tests which still work because we haven't (yet) touched that code
I'm currently testing each function the rework code as i write it, though admittedly i have no idea how to mock asyncio calls. As a workaround, I've just made the calls synchronous and tested with expected data. I'll leave the rest data in as comments. I'm assuming once the code is stable enough these "test" data objects can be mocked in a more concrete manner.
An issue we're running into is the test cases don't reflect real-world use. Some users have massive libraries or a ton of friends, etc, and the unit tests don't properly catch the issues that crop up as a result. These issues were there in FoG's code but it passed all the tests. Before the work @don-de-marco did (and is still doing), large libraries would consistently kill the plugin not due to any errors in the code but instead because it was too slow or too synchronous/blocking
A counterpoint, of course, is that proper trading would have caught the no 2FA bug that was fixed in 1.0.7. I guess the moral of the story is i need to finish the rework faster so we can properly test before releasing 2.0
I can't help you there, either. It's probably best if you just look into how the already existing tests do the asyncio stuff. I know for certain that we have the means to test asyncio methods. But I do not know if and how one would mock them.
oh god, now i need to merge all this into my conversion branch and prep all of this for nebula's clean history. can't wait! /s
Unit tests wont catch a lot of things, but should find a lot of dummy mistakes (and save time for testing broken build). I cant imagine to dont have running tests suite since I started to practice TDD. The same with the CI running tests. Ill add ci running what we have green already if find a time.
Only been tested on mac so windows logic remains largely the same just reorganised. Complete overhaul of Mac logic as at least on my mac steam's
registry.vdf
does not show an app states butcontent_log.txt
is a pretty good alternative. Consolidated all system logic and kept all common logic inbase.py
with all os dependant logic in seperate files.Note: currently this works better then even master on my mac (as states actually work) so if you'd like to start pushing this to become the latest release i could work on cleaning up the history for
better_protobuf_deps
. otherwise i think just merging it normally should be fine. it won't be pretty but maybe in the future we could work on smaller PRs so its easier to either squash or rebase