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

Adding GlobalDecks Feature #4111

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Adding GlobalDecks Feature #4111

wants to merge 6 commits into from

Conversation

QuzarDC
Copy link

@QuzarDC QuzarDC commented Sep 28, 2020

Related Ticket(s)

Short roundup of the initial problem

Allow a list of decks available for download to all server users.

What will change with this Pull Request?

  • Adds new value in servatrice.ini to specify the user account whose decks will be visible to all.
  • If set, the DeckList command will return folders/decks of that user to all users.
  • Global decks will appear with a deckID that is 10000000 more than its normal, to avoid collision with user's decks.

Screenshots

No frontend changes were made.
image

@QuzarDC QuzarDC mentioned this pull request Sep 28, 2020
@ebbit1q
Copy link
Member

ebbit1q commented Sep 28, 2020

looks hacky enough it might just work 😉

@Fenrirthviti
Copy link
Contributor

Great idea! It would be useful if there was also an option to allow all users to see all other user's decks. As mentioned in the related issue, my personal use case for this feature would be sharing EDH decks between friends on a private server, so this would be very handy to have!

@Fenrirthviti
Copy link
Contributor

Fenrirthviti commented Nov 6, 2020

Testing this PR out, and it doesn't seem to be working for me. I have recompiled current master (updated for clarity) with your patches and added the appropriate servatrice.ini option for my user ID (2 in this case), but they are all still uploaded with normal deck IDs. Am I missing something on how they need to be uploaded?

Is there some way I can manually test that everything is working?

@QuzarDC
Copy link
Author

QuzarDC commented Nov 6, 2020

Testing this PR out, and it doesn't seem to be working for me. I have recompiled current master (updated for clarity) with your patches and added the appropriate servatrice.ini option for my user ID (2 in this case), but they are all still uploaded with normal deck IDs. Am I missing something on how they need to be uploaded?

Is there some way I can manually test that everything is working?

So I think my previous explanation may have been poor (or I'm misunderstanding what happened above) but decks for the global deck user should have normal deck IDs. These decks should simply also be visible, with the shifted IDs, to all other users. So to see if it's working, it should be to create a deck with user ID 2, then log in with user ID 1 and check to see if that deck shows up. To user 2, you will just see your own decks.

I'll take some time to think about your use case. From a brief attempt I'm thinking the best way to make this workable in the hack would be:

  1. Allow the global ID to be in the form of a list or *
  2. Have the offset represent the global ID that it is from (ID*offset + deckID ?)
  3. Have each global user's decks return as a different folder

@Fenrirthviti
Copy link
Contributor

Fenrirthviti commented Nov 7, 2020

Thanks, that explanation makes a bit more sense. I mostly understand the changes you're making, and why you are doing it the way you are. Allowing for multiple global IDs (with the optional wildcard for all) is probably the best short-term solution. Long-term would see this feature more refined with front-end changes where an administrator could flag decklists as global from the Deck storage tab, but I also understand this is a fairly niche feature, and sharing decks between users is pretty simple already. That said, it's certainly a nice QoL feature, and allows for collaboration in a lot of casual formats.

I've been testing with two accounts, we'll say:

Account1 (ID 2)
Account2 (ID 3)

In my config I have globaldecksid=2

Account1 (ID 2) is my admin account, which I used to upload several decks. When I log in to Account2 (ID 3), none of the previously uploaded decks show up. I'm not sure what exactly isn't working properly. Is there any kind of console or debug output I can turn on to monitor for the functions/any errors?

As I'm writing this, I realize that I'm only patching servatrice, not the client. Do I need to patch and recompile the client as well? Given the changes are entirely in the servatrice codebase (and my limited understanding of C-code) and your comment that the frontend was not changed, I'm assuming this is entirely on the server side, but just to confirm.

Additionally, I'm running everything in docker, which I'm not intimately familiar with, so I might be making a mistake on that end. I've confirmed the container is receiving the updated config file, but I am not sure how to verify that the recompiled program is working properly. I might just delete the container and start over, to be on the safe side. If this fixes the issue, I will reply back. EDIT: Confirming this was, indeed, the problem. Apparently you can't just do another docker build -t servatrice . without deleting the previous instance, or there is some additional flag I am missing while re-compiling. Only takes a min or so though on my server, so not a big deal. Working exactly as advertised now!

@QuzarDC
Copy link
Author

QuzarDC commented Nov 7, 2020

Thanks, that explanation makes a bit more sense. I mostly understand the changes you're making, and why you are doing it the way you are. Allowing for multiple global IDs (with the optional wildcard for all) is probably the best short-term solution. Long-term would see this feature more refined with front-end changes where an administrator could flag decklists as global from the Deck storage tab, but I also understand this is a fairly niche feature, and sharing decks between users is pretty simple already. That said, it's certainly a nice QoL feature, and allows for collaboration in a lot of casual formats.

I've been testing with two accounts, we'll say:

Account1 (ID 2)
Account2 (ID 3)

In my config I have globaldecksid=2

Account1 (ID 2) is my admin account, which I used to upload several decks. When I log in to Account2 (ID 3), none of the previously uploaded decks show up. I'm not sure what exactly isn't working properly. Is there any kind of console or debug output I can turn on to monitor for the functions/any errors?

As I'm writing this, I realize that I'm only patching servatrice, not the client. Do I need to patch and recompile the client as well? Given the changes are entirely in the servatrice codebase (and my limited understanding of C-code) and your comment that the frontend was not changed, I'm assuming this is entirely on the server side, but just to confirm.

Additionally, I'm running everything in docker, which I'm not intimately familiar with, so I might be making a mistake on that end. I've confirmed the container is receiving the updated config file, but I am not sure how to verify that the recompiled program is working properly. I might just delete the container and start over, to be on the safe side. If this fixes the issue, I will reply back. EDIT: Confirming this was, indeed, the problem. Apparently you can't just do another docker build -t servatrice . without deleting the previous instance, or there is some additional flag I am missing while re-compiling. Only takes a min or so though on my server, so not a big deal. Working exactly as advertised now!

Glad to hear you got it resolved. Yea, there were no changes to the client. Ideally, that would be how to do it, but should be part of a much more robust system of deck sharing between users.

Feel free to let me know if you have any issues with the current implementations (or suggestions). I don't mind trying to implement them if I can :)

@Fenrirthviti
Copy link
Contributor

Alright, so I've had a lot of time to think about this, and have a lot of varied feelings. I'm going to go off on a few tangents here that I know are not directly related to this feature, but hopefully it will all come back together.

There's value in keeping this feature braindead simple, and only allowing a server operator to specify the user (or users) who has global decklists. This would allow for a server maintainer to, say, create a dummy user and use that to upload decklists to. Bare minimum for this current implementation to be viable, IMO, is to allow for multiple users to be specified as the global deck users.

Additionally, I find the +10000000 to the ID a bit obnoxious in practice. Would it not be better to just prefix the ID with a G instead? I know the ID itself is an integer value which would be annoying to change, but perhaps something like <cockatrice_deck version="1" global=true> could be used here and then parsed client-side? Adding client/server-dependent changes would make versioning server/client versions a bit tedious, but for a new feature like this, I feel it would be worth it, and ties in to my next thought.

Ultimately, I feel like this feature should be expanded on to be a true sharing/collaboration feature. In order to do that, new permissions would need to be created per user, as well as a way to assign those permissions. Currently, the server administration tools from a client-side perspective are very lacking. You can't really view or edit users from the Cockatrice GUI, it's pretty much a "manually manipulate the sql database" process. Enhancing the options for server administrators would be fantastic, as then new permissions, such as:

  • View Global Decks
  • Upload to Global Decks
  • Download from Global Decks
  • Modify Global Decks

Again, I do know that that is a little outside scope, and the alternate methods are a little more broad in scope as it requires touching a lot more, but I wanted to at least get my thoughts down on what the current implementation is lacking, and what would allow for a better overall experience. It could even be potential for allowing a framework where users can share specific decks with other individual (or groups) of users. Lots of possibilities here!

My individual testing is only with a few players, but we have been really enjoying having the global decks (we only really play EDH) that we can share and all use without having to worry about lists getting desynced somewhere, even in this currently limited implementation.

All that said, as a final note I have experienced no bugs, issues, or crashes whatsoever using this on my server with both the current stable release and my custom builds that I am keeping up to date with master with. Everything works exactly as advertised in the PR on all clients.

@QuzarDC
Copy link
Author

QuzarDC commented Oct 5, 2022

I know I'm quite late in responding, but was going to bump my request on this and figured I'd make sure to add something here. I agree with all of your points. The only reason the single global user and prefixed deck IDs was used was to retain compatibility with the client. The basics of the implementation could easily be used to send two distinct sets of decks (private vs shared) and have a much nicer user interface to present.

With the base concept implemented, all sorts of other user sharing and interaction features could then flow out primarily as client-side changes.

I've still been using this on my home server (~20 users). I've used it both to have access to starter decks for new users as well as sets of event decks without issue.

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.

3 participants