Add ability to add more than one account per service #4487
Conversation
|
May I humbly request a review of this? Also, feedback on the mentioned caveats would be greatly appreciated… |
There was a problem hiding this comment.
Overall this code is pretty good, but rather uncommented. The id/type/name thing could stand explanation somewhere.
Yes, you should be able to tell which account is which in the UI, of course.
Minor thing- you search search for 'service_name'. You missed a couple and they are actually ids, I think.
Big thing- you're going to hate this, but this PR is in my view both too big and too small.
This is a lot of change at once, and likely to introduce bugs. Yet it is not a complete feature- there's no UI to actually use this.
The 'right thing' here would be to introduce a series of refactoring PRs to prepare Lutris for a change like this. Like adding get_service() and using it everywhere, say. Or introducing the _tmpl mechanism. You'd want to find things that are improvements on their own, ideally, but you should be explaining what these changes are meant to support.
This takes time, but allows for more testing and makes the final PR for the actual feature more manageable. And if @strycore rejects your PRs with "I don't want this feature" you'll have wasted less time building all this.
I guess it's a bit late for that kind of feedback, but if @strycore rejects this as too big, you might try to resubmit in stages like that.
I won't merge this because it is clearly a feature, and @strycore should approve it. But if it were my call, I might accept it if you added a UI for managing accounts, and only right after a .1 release- way too risky to introduce this before a release.
Then other people would have time to try this feature for non-GOG accounts and any problems there could be shaken out, so that would work out, I think.
|
@danieljohnson2: Thank you for your detailed feedback!
Agreed! I’ll look into that!
I can easily add this. I only wanted to know if devs would prefer the account user name to always be visible, or only if more than one account of the given type has been added?
I’ll check all uses of
I always try doing that by making sure each commit is independently reviewable and merge (in order of course). Perhaps these are still too large for @strycore’s tastes each or perhaps you would simply prefer each of these being submitted as independent PRs instead?
I did not want to start working on an account management UI yet, since it would likely double the size of the patch while almost certainly not ending up the way maintainers want it to look like in any case (the best way to do this is highly subjective IMHO). If there is serious interest at this point already, I can try and draft up a design document though.
Very understandable stance! |
|
I don't know how serious the interest is; I wouldn't use this feature myself, but you asked for review. I think you should not trouble with a design document. Too waterfall. Put the simplest UI you can on it; perhaps something involving extra buttons in the sidebar, or context menus, or something. Just keep it simple. You want to get this to 'basically usable'; it can evolve from there. |
|
I'm ok with this feature existing but I find this patch is over-reaching and modifying too much of the current architecture. It makes things quite harder to understand and I do not think the implementation of this feature require that much change on the side of service modules (it should mostly be a database thing). |
I’m sorry I re-read this part of your comment several times and I’m still not sure what you mean with “a database thing”? Could you clarify please?
Thought about this for a bit and I think you are correct. – The service media is per service-type rather than per service-account, so splitting stuff into per-account caches like this really does not make much sense… |
|
Oh, and do you agree with @danieljohnson2 that I should cook up some dummy multi-account management settings UI for this PR already? And any preference on how and when to distinguish accounts in the sidebar:
|
Actually, the those ServiceMedia instance still need to know the service ID, so that they can query the list of games from the database, so I kept that part in and only reverted the part where it would parameterize the cache directory path on said service ID (so images of same game from the same service but from different accounts would now be stored only once). |
|
Your patch makes zero changes to the lutris/database package which is precisely where the changes should be. |
|
I find that a bit unclear, @strycore. By "services files", do you mean the .py files in lutris/services? If so, that would suggest to me that you'd need to have many 'service-account' objects, each referring to a service- probably a better design. But how does lutris/database come into it? A new table for these accounts maybe? But that does not seem to fit with how Lutris stores service credentials now. I can't see how you do it without changing the service classes. |
I do not believe adding an extra class for “service accounts” would in any way constitute a “better design” as all it would do is needlessly complicate the codebase: The current service classes already manage account-related data (but are only instantiated for a single account ever) and there is no cross-account but per-service state to maintain anywhere… So what’s the point of adding another class for the per-account state, when the current service classes will just wind up devoid of any state as a result?
That is correct, hence why I change them. All changes are strictly to make service objects aware of the account they are supposed to represent. Even if you move the management of accounts to the database these changes will still be necessary.
@strycore: I do not appreciate the smug tone, see above. |
|
Well, any reply would be appreciated at some point. I’ll regularily keep this up-to-date until then, but I won’t rewrite everything or work on the remaining todos until it becomes clear what @strycore actually wants to happen… |
|
You can ignore my views on this PR if you like; it is @strycore you need to satisfy if you are going to get merged. You might start by appreciating his smug tone. In this PR here, he's being straight with you, and not insulting or offensive. I don't think he's entirely clear, but he's trying to help you. You can listen, or you can not get merged. Your call. |
Let’s let that slide for now then… 🙄 Seriously though, I’m trying but it’s a bit hard:
No, no, no. He’s not clear at all, not one bit! The last statement I have received was 22 days ago and sounded like:
… and it left you, judging by your comment, as confused as it left me. I have tried to explain why the changes are as they are, I have tried to receive further guidance on what it exactly is that @strycore wants, but I still don’t have the slightest idea frankly. Even your idea of adding extra “service account” classes, which I believe to be a bad idea for reasons previously explained, was hardly more than a wild guess on your part unless there was some behind-the-scenes communication going on that I’m not seeing (in which case it would have been very forthcoming to publish the results of such conversion having taken place).
I will listen if actually talked to… I will not blindly rewrite this thing several times until it just happens to agree with @strycore’s vision. |
|
A good start would be to revert the |
|
@ntninja Thank you for your effort, I can absolutely understand that it is frustrating to you. I would love this feature to become reality. @strycore's tone here is a bit harsh first, but if you read it twice it seems mostly informative to me. He just wasn't clear in stating what he wanted you to do, while being clear in stating his opinion on your PR. This obviously was some miscommunication on his side imho. Are you still working on it or do you want to continue your effort at some point as @strycore clarified a bit on what his vision is? Even if the change from id to type sounds logical to some outsider like me, just from a naming convention standpoint (without any knowledge about the Lutris code base and python in general). EDIT: One compromise could be to rename that in another PR (if that is something that would be accepted). |
|
@Root-Core: Thank you for your kind message! I was planning on returning to this PR eventually, but in recent times have mostly been focused on other things in life. I’ll probably get to it eventually, but don’t hold your breath. If you, or anyone else, feels like taking over, by all means please do so! Regarding the rename from type to id: It is already an independently reviewable commit (7facce8) and that could indeed be submitted as a separate PR… Thing is, there is zero point in performing that change alone without the context of the subsequent commits – hence why I grouped these changes together in this PR in the first place. I guess everyone just looked at the whole list of changes and thought “that’s too much” without ever noticing that its made up of several smaller (and IMHO reasonably sized) changes/commits? @ALL: Is there any point in doing what @Root-Core suggested and moving each of these commits into a separate PR? |
…s extra information to distingish between different service accounts
…editing the configuration file and whitelist GoG for this
Adds the hidden ability to associate more than one instance of some service accounts (individually whitelisted where it makes sense) and whitelists GoG for this which I could actually test and confirm working.
Caveats / Todos:
Related to #4301 (although it doesn’t do anything for Epic launcher) – maybe there is a better issue somewhere?