search

javierriveracastro / betteroll-swade

A Better Rolls port for SWADE
GNU General Public License v3.0
15 stars 31 forks source link

Added the ability to mark a custom world action as replaceExisting #720

Closed ddbrown30 closed 3 weeks ago

ddbrown30 commented 4 weeks ago

This adds a relatively easy way (well, easy for us so we don't have to write a bunch of new code) to allow users to override the default global actions. This came about for me because I have a player with the Blind hindrance and like at least 75% of their rolls don't need Blind enabled. I wanted to override it so that it's not on by default but there was no way to do that. The ids are not unique keys, so I just ended up with Blind in there twice. This way, we can optionally stomp over an existing action if we want it to work differently.

javierriveracastro commented 4 weeks ago

I'm not really sure about this. IDs are unique among actions defined in code (by the system or modules).

I believe that the correct way on doing this is making the ID from the world actions override global (code) actions if it is the same ID, just to keep consistency.

ddbrown30 commented 4 weeks ago

I'm not sure I follow. Isn't that exactly what I did? Or are you saying you don't want it to be controlled by a flag and instead always override if a world action has the same id? If it's the latter, I'm fine with that. I only did it using a flag so as not to break any existing logic that might be relying on that. I would definitely prefer unique ids to be strictly enforced.

javierriveracastro commented 4 weeks ago

Yes, what I mean is that I prefer ids to be unique. But the point about breaking current uses is also valid.

ddbrown30 commented 4 weeks ago

I'm fine with either. If you want me to remove the flag and make it enforced, let me know and I'll update the PR. If not, just merge it as is.

FWIW, I think the breaking case is unlikely and has an easy fix of just changing the id on the custom action.

javierriveracastro commented 4 weeks ago

After thinking about it a while, I think that you are right. Can you update the PR so that it is enforced that ids are unique?. When there is a conflict the priority should be: manual world actions > other modules > BR2.

Thank you very much :).

ddbrown30 commented 3 weeks ago

I can easily change it to force the stomp but the priority order would require more work. Since the timing of adding everything is that the other modules add theirs last, it means that we would need to be able to identify the source of an action. I'd need to refactor this all into a single API function that takes and uses a priority.

Is that what you want me to do or should I just change this current PR to remove replaceExisting?

javierriveracastro commented 3 weeks ago

We should be able to add world actions after modules. I need to think a little, but we can either do it at first card render or add some timing. No need to add priority. Off course priority would be cleaner, but still.

ddbrown30 commented 3 weeks ago

Sure, sounds good. Do you want me to just remove the flag and then you'll take care of the ordering?

javierriveracastro commented 3 weeks ago

XDDDDD

You got me. It's quite unlikely that I'll do it. So I will merge this. It is not the optimal solution, but at least it is a solution.