Added an option to pop out chat cards automatically

[ddbrown30]

First actual feature work from me. What this does is add functionality for automatically popping out chat cards when they're created. With the exception of the damage card, this happens for the user who triggered the roll. For the damage card, it pops out the window for the owner of the actor being damaged. This means that if the GM is applying damage to a player, the pop out will appear for that player. Otherwise, it appears for the GM.

Both the damage card and incap card close themselves when moving to the step e.g. when you click "Show Incapacitation card" from the damage card, the damage card closes itself before opening the incap card. This prevents a stack of windows as the player goes through each step.

Another changes you'll see in there is the addition of the init hook. I needed to do this because the calls to renderChatMessage happen before the ready hook which means that the settings hadn't been initialized yet. I tried to minimize the amount of things I moved over.

Finally, you'll see some reworking of setFlag in a few places. This was needed because each time you call setFlag on a message, it triggers another call to renderChatMessage. This chain of renders meant that we would sometimes be calling multiple at the same time and we'd end up with thread race conditions. This minimizes the number of renders and prevents the cases where we were calling multiple times on the same message in the same callstack.

I've pretty thoroughly tested this. I've had a GM and 2 other players all active at the same time and tried every combination of card that I could think of. It looks solid.

Let me know if you have any questions.

[javierriveracastro]

First I love the idea, and it is really well done.

I'm not sure about moving setting initialization to init, I must check it out. For sure it seems to make much more sense, but I thing it can have side-effects, specially with full chat logs.

And I also need to check the removal of the awaits when setting flags. To me that could create a lot of possible race conditions, as multiple instances of the card object can start trying to update the database at the same time. I will also need to check that.

Finally Sourcery is probably right on most accounts, so check its advice. It is an AI coupled with some rules-based checking, and sometime quite dump, but I have found that it is usually right.

Finally, did I said that I loved that idea?.

[javierriveracastro]

Removing setFlag from the save() method of the card you have made it not saving the flags. And that is what that method is supposed to do, save the card with all the data (including flags).

Also not saving it, the changes are not propagated to the other browsers.

[ddbrown30]

Glad you like it. :) Let's see if we can sort out the issues. Sourcery worked alright but I downvoted the places it got wrong. I get to that below.

1. Let me know if there are any issues with init. I didn't see any but maybe there's some functionality with BR2 that I didn't test.
2. I didn't actually remove any awaits. store_render_flag is no longer async because I removed the setFlag call from inside it. It now just sets the flag directly and relies on the message.update call in save() to do the actual update (see the next point).
3. While I removed setFlag from save() it was replaced with message.update. What it's doing now is setting all the flags directly on the message and then doing a single update call at the end. This still ends up saving them all but does it as a single operation. This was required because every time you make an update on the message, it triggers renderChatMessage. Since hooks are not async, there was no way to use await on any of the logic there which was causing race condition issues when we were calling setFlag multiple times.

Do you have some repro steps for how you're ending up in a case where the data isn't propagating to other browsers? My tests were done with 3 users, 2 in Chrome and 1 in Firefox, and everything appeared to be synced correctly to me but maybe there's a specific flow I missed.

[javierriveracastro]

I'll try to get the points:

1.- If all the messages in a long chat are processed on load, that could create (or not) a performance problem.

2.- OK.

3.- I see, probably reading changelogs in github is not my best skill. It was setup like that, because render_data is supposed to be temporary, the idea is moving data out of there to properties. You are still calling update twice (well, one is in a conditional), if you are going to do it that way, we should probably create just one update delta and call update once.

I'll merge it and see what happens. Please in the future try to keep a PR with just one feature. This would have been easier for me if the popup and the save changes were on different PR.

[ddbrown30]

1. Hmm, yeah, I hadn't considered that. I think it should be fine, though, or at least no worse than it was before my change. If it is a problem, it will only be an issue the first time a user loads the game after the module is enabled/updated. Future loads after that will already have the processed flag set and so won't do anything.
2. While I am still calling update twice, there's no way around that. One of them is on the creation of the message and one of them is on the render. The render one isn't necessarily being done by the person who created the message. In the case of the GM applying damage to a player actor, it's the player that sets the processed flag because they're the one who needs to decide whether or not to show the popout based on their local setting.

I do generally try to limit PRs to a single feature but I'm not really sure how I could have split this up. The changes to how we're setting the flags was required to get the popouts working. Without it, I would often get multiple popouts appearing for the same message due to the race condition.

[javierriveracastro]

I see. You use a flag to know if you should create or not the popup, and you do it in the message render hook.

It is probably better to create the pop_up inside the object. Maybe on creation. Anyway I still haven't merged it, and I just talking from looking at the changelogs, so maybe I'm saying stupid things.

Anyway, saving less times is wise. So it is still a good idea.

[ddbrown30]

All good. Let me know if you have any questions once you've had a chance to merge and test it.

[ddbrown30]

While you're digging into this, I want to throw out a question to consider. I've now used this with my group and it was so good. Any time there were group rolls to be made (like resisting and AOE effect) it was so much easier for everyone because they didn't have to deal with the constantly scrolling chat log. For soak, unshake, and unstun rolls, it was also great to have that reminder pop up right in their face so it wasn't forgotten.

With that in mind, what do you think about turning on this option by default? With client settings, there's no way for the GM to set it for everyone else. That means that when everyone joined the game, I had to walk them all through how to enable it. I still like it being a client setting, though, so that users can decide to disable it if they want. I personally think that this improves the usability so much that it should be the default.

If you disagree, all good. I just figured I'd put it out there for you to consider.

[javierriveracastro]

I kinda agree.

I would turn it on by default, but I would add two settings a global one and client one (both default to true), and only popout the card when both are on. That way people who don't like it, and there will be, trust me, can disable it easily.

What do you think?

[javierriveracastro]

I'm merging this. I have opened a new branch to work on this (pop_out). There you can check the changes, as I want your opinion on them.

The first thing is moving pop-out creation from renderChatMessage hook to the create_foundry_message method of the BrCommonCard. That way I think I get to advantages:

• No fear of race conditions as this should be only run once.
• No need to keep a flag to see if the popout has been created.

It should be on one commit on that branch.

[javierriveracastro]

Finally I have changes the save method to save only once. The problem is that it is always saving now, but let's trust that Foundry is doing a diff and avoiding stupid roundtrips.

Now that I think about that, maybe I can get rid of the update_list thing...

[javierriveracastro]

Finally, are you sure that pop-up should be a client setting? The more that I think about it, the more that I see it as a world setting.

[ddbrown30]

I'll have a look when I get a moment. I'm at a show tonight so I might not get to this until tomorrow. We'll see if I can find some time during the day.

So without having looked at your changes, my worry about moving it out of renderChatMessage and removing the processed flag is that this will no longer work with multiple players. Either your changes will mean it only pops out for the person who triggers the roll (which breaks the soak/injury flow) or it's going to pop up for everyone which is obviously a problem. I'll have a look and let you know.

As for the setting discussion, this one is up to you but my opinion is that it should be a client setting that's true by default. Having it be a world setting means all the players are forced to have it turned on even if they don't want it or that they're not able to use it at all if the GM doesn't want it on. Having it be both a world and client setting also doesn't make much sense to me because it doesn't seem like something that the GM needs to control at all. Whether it's on or off doesn't change the overall functionality of the system; it's just a QOL improvement and each user is going to have different preferences on how it appear.

[ddbrown30]

Okay, I couldn't help myself and jumped in already. :P So yeah, I can confirm that moving it out of renderChatMessage does indeed break the damage flow.

[ddbrown30]

The changes to save look good.

With that in mind (and assuming that you want the damage flow to work), if you put back the renderChatMessage/processed code, I think this is good to go.

[javierriveracastro]

I'll take another look at it. Somehow having it on the render hook feels unnatural, I can't really point at why but it doesn't click. I'll keep it there if I must, but first let me try to find some simple way of keeping the damage workflow working with the popuot call inside the class itself.

Now the settings. I don't think it as a good Making it on by default and client based is a good idea. Lot's of people aren't going to like it (people hate change) and making it on by default and hard to deactivate (each player will have to do it). and making it coming back each time cookies are cleared...

When I talked about two settings was to give the master a simple way to deactivate it for everybody. I'll think a little more about it.

[ddbrown30]

I agree it's not the best but the "correct" way would require us to do it via socket. Basically, I'm piggy-backing off the fact that everyone will call the render hook regardless of who triggers the roll which gives us the ability to then process it on a per user basis.

If you want to put in the framework for sockets, that would definitely be the better way, but it was more than I wanted to deal with and it would have meant much more changes (and adding a dependency to the module) to get the same behaviour I was able to get just through the render hook.

[ddbrown30]

For the settings, as I said, up to you. I'm of the opinion that more people will like it and find it useful than don't which makes it a good candidate for being the default option. The annoyance for everyone needing to go in and change the setting by hand is there no matter what and it's just up to you to decide if you want to make it annoying for the people who want it on or the people who want it off.

Regardless of that, I still really don't like it being a world option. As we've been discussing, this option is going to be 100% personal preference. There's no reason why something like that should be on the GM to control. It's like making the appearance of your dice or the rollable area in Dice So Nice a world setting. It's something that is purely a UX decision and so should be on the client side.

The two options thing still makes no sense to me and is the worst of both worlds. All you've done is make it equally annoying for the players while giving a kill switch to the GM that can't be overridden by the players. Wherever you land on this, I'm very strongly of the opinion that 2 options is the wrong decision.

[ddbrown30]

Since you mentioned the cache, one other suggestion I'll toss out is making it a flag on the user. I was considering doing this before but decided to limit the changes I was making. It still means users have to set it to what they want the first time (unless what they want is the default) but then it will follow them around to other machines and will survive a cache clear.

[javierriveracastro]

Sockets are out. I prefer to keep complexity low than code that feels right. But I think it should be possible to do it without sockets. I'll try to look at it this afternoon.

About the setting... we are not going to reach an agreement on that. Can you ask in the "BR Card redesign" thread in the Discord to check what people think?. If not I will do it, but I think you should do it, as it is your beast.

[javierriveracastro]

You are right. At the end I moved it back to the renderHook. Instead of a flag I have added a property to the card, but this is probably irrelevant.

Please check the code when you can.

[ddbrown30]

Looks almost right but with removing the processed flag and replacing it with a local property, it reopens every popout when you refresh the page. There is simply no way around setting and updating a flag on the message because we need it to persist.

In that same vein, the reason why I was setting the flag outside of the scope of the auto_popout_chat setting is so that when players have the option disabled, they still "process" the message. Without this, anyone who enables the setting later will get spammed with every popout they didn't handle while the setting was disabled.

[ddbrown30]

Oh, and for the settings discussion, how about we start with what's currently there (i.e. client setting, off by default) and get that in. That's the least obtrusive way to start. We can get that out to people so they can see it and start messing around with it and then we can post on the Discord thread to see what people want. Does that work for you?

[javierriveracastro]

Kinda, I understand that if we change if to default true in other version, people who are just updating are going to keep it as false, but new players or players that change browser will start with it on.

It can be confusing, but... release early, release often...

[ddbrown30]

I don't think that's how it will work. I'm pretty sure it only saves the value if it changes so the situation you're describing will only happen to people who manually turned it on and then manually turned it off. I might be wrong, though. I would have to test. If it does work that way, though, we could easily get around that by simply renaming the setting.

[javierriveracastro]

That's a great idea (renaming the setting). I didn't think about it. :)

[javierriveracastro]

Merged.Should be in the wild the next update. Likely later this week.

[ddbrown30]

I don't see a commit that adds back in the processed flag. Did you miss my message about how it causes the popouts to reappear when you refresh the page or am I just looking in the wrong place?

[javierriveracastro]

I didn't see the message...

[ddbrown30]

Ah, yeah, I figured that must have happened. This is the message I mean: https://github.com/javierriveracastro/betteroll-swade/pull/676#issuecomment-1949141842

[javierriveracastro]

It is not a local property, it is supposed to be saved with the card (in a flag inside the message). But, yeah, it is not working, either not saved or not recovered.

[javierriveracastro]

I have forced the saving of the card after the popup is created.

I hope that it solves the reload problem (as it was inconsistent before), but sometimes the popup will be shown again when you change something in the card (it is not created again).

Truth this is very confusing to me. I should probably find some easier to understand rewrite before I make this enabled by default and feel comfortable about it.

Well, now let's focus on releasing fast the last 3.2 version and see what kind of feed back we get from users.

[ddbrown30]

Looked at the change. There's still the issue that you're saving it inside of the the option check. This means that when someone enables the option and refreshes, every card they haven't processed will pop out. The push and save calls need to be in between the user block and the settings block i.e. in between lines 131 and 132.

I'll also point out that what you've done here vs what I had done originally means that we're processing a lot more logic. With what I had, it was targeting and saving a single flag. You're now calling the entire save function. I'm not sure if that's going to cause any problems, but it's a completely different flow now than you used to have. As far as I can tell, save was only ever called by the person who created the initial card before, so that's potentially a big difference. It also means that, depending on if the option is enabled or not, clients are calling different functions and potentially making different modifications. You know this code much, much better than I do, but I figured I'd point it out.

[javierriveracastro]

What I'm trying to do is exactly the opposite. Having a clearer workflow.

Need to think more about all this.

Still not looking at the code, but moving save to line 132 will mean that we will save even when the setting is set to false. That's a no for the moment. Maybe in the future when this feature is stable, but right now I prefer to keep the code flow when this is disabled as close as possible to before.

Save was called when the card is updated and you want to persist it in the database. Anyone who can click on the card buttons as an example was saving it.

[javierriveracastro]

Ok. last commit should avoid most problems (except the enable and reload one), and it also limits to a maximum of 3 the times that renderCharMessage is called on card creation.

Now, better go on testing an releasing.

[ddbrown30]

Cool. I think we're good to go then. Excited to see it released. :)

[javierriveracastro]

I'll release this afternoon after solving what are most issues. If you check the last commit, (https://github.com/javierriveracastro/betteroll-swade/commit/3001916b7b725fa9329062704cac61af5dc3c244) it was the cause of the problems that I've been seeing And it is so stupid that I'm still crying.

[ddbrown30]

Oops. JavaScript strikes again. 😛