orcnog / orcnog-card-viewer

MIT License
4 stars 0 forks source link

Draw support to say the least #22

Closed kristianserrano closed 9 months ago

kristianserrano commented 1 year ago

NOTE: Macros have not yet been updated!

kristianserrano commented 1 year ago

@orcnog there is only one place where the code makes use of a hidden showToAll setting, and that's in cardDealer.draw() since it triggers the cards.deal() which triggers the hook, which triggers the display. After cards.draw() is executed and completed, it immediately sets the setting back to the default value which is false. In all other cases, a showToAll boolean value is passed from function to function.

orcnog commented 1 year ago

Hate to make you do this, but as I just pushed up a whole bunch of changes, could you redirect this PR to a new branch I just cut for you? https://github.com/orcnog/orcnog-card-viewer/tree/draw-pass-support

The branch is updated with a few of your changes from the v10 branch, but not all. There will likely be some merge conflict to resolve, but at this point, it's the latest code, so I want to make sure we're keeping our feature branches up-to-date. Thanks! @kristianserrano

kristianserrano commented 1 year ago

No problem.

kristianserrano commented 1 year ago

I'll need to take some time with this. There's some new features (e.g. default card backs) and functional changes that I want to make sure are still working after the merge. In fact, the default card backs makes some logic easier, but I think you already knew that. ;)

orcnog commented 1 year ago

@orcnog there is only one place where the code makes use of a hidden showToAll setting, and that's in cardDealer.draw()

So, still striving for a way to do it without caching a temp value globally, I experimented with the error coming from setting a custom action string, and realized that if I simply pass chatNotification: false, it still allows me to pass a custom action string and read it in the hook, but it doesn't throw the error... at least not in v11. Would you mind testing this in v10, @kristianserrano?

Example:

await deck.deal([pile], 1, {
   how: CONST.CARD_DRAW_MODES.RANDOM,
   action: shareToAll ? 'deal orcnog_card_viewer_doshare' : 'deal',
   chatNotification: false
});

Of course, it suppresses the actual chat post when I do that, but I'm willing to give up the out-of-the-box chat notifications for card draws (only those originating from macros) if it makes the mod fully compatible with v10. Actually, if it does work error-free in v10, maybe we just wrap it in a version detection condition and only suppress chatNotification in v10 (since v11 seems to throw an error but push through and complete the action anyway).

kristianserrano commented 1 year ago

That's not a bad idea. Do you have that code in a branch I can check out?

orcnog commented 1 year ago

~Working on one... might not be up for a few hours though. My REAL job is getting in the way =)~

Scratch that. I just pushed that one change up to develop, and merged it into draw-pass-support for you.

kristianserrano commented 1 year ago

No worries. I'm in the same boat. I still have to finish resolving the merge conflicts. I might cancel the merge for now though.

kristianserrano commented 1 year ago

~Working on one... might not be up for a few hours though. My REAL job is getting in the way =)~

Scratch that. I just pushed that one change up to develop, and merged it into draw-pass-support for you.

You rock.