Added unsiegeable towns #315

poilet66 · 2021-07-19T20:20:00Z

Adds an unsiegeable flag to towns, run the following command to toggle unsiegeability:
/swa town setunsiegeable <true|false>

Description:

So that admins can easier create admin towns that are unsiegeable and just toggle this setting, rather than having to unecessarily give large amounts of immunity timer.

New Nodes/Commands/ConfigOptions:

2 new lines in english.yml that'll need translating, they can be found under the names:
msg_swa_town_unsiegeable_set and msg_err_cannot_start_siege_at_unsiegeable_town

Relevant Issue ticket:

I have tested this pull request for defects on a server.

By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the SiegeWar License for perpetuity.

/swa town <town> setunsiegeable <true|false>

LlmDl

A couple a small changes needed.

The only thing I would consider adding is maybe something to the town status screen event, so its visible that the town is unsiege-able from the /town screen.

LlmDl · 2021-07-19T21:02:07Z

src/main/java/com/gmail/goosius/siegewar/playeractions/PlaceBlock.java

+		}
+		else {


Undo un-needed change.

src/main/resources/english.yml

LlmDl · 2021-07-19T21:05:57Z

Rather than a completely new flag, I would like this new capability be added onto the existing siege-immunity system. (as I indicated yesterday in a comment in siegewar_admins)

They key idea is that permanent siege immunity is the same thing as unsiegable, only that the first has better integration with existing features, smaller changes to the code, and no need for a full new command.

Thus, as per my comment yesterday, I'm requesting the following changes:

For the required command, simply add one extra option to the existing adminimmunity command

ie. `/swa immunity town|nation|all 1|2|3|4|5|6|permanent'

Once this command is run, the existing immunity figure gets set to -1 and saved.

This shows on the town screen in the existing field i.e. "Siege Immunity: Permanent"

I can also agree with this, re-using the existing metadata at -1 is the move.

poilet66 · 2021-07-19T21:52:38Z

Rather than a completely new flag, I would like this new capability be added onto the existing siege-immunity system. (as I indicated yesterday in a comment in siegewar_admins)

They key idea is that permanent siege immunity is the same thing as unsiegable, only that the first has better integration with existing features, smaller changes to the code, and no need for a full new command.

Thus, as per my comment yesterday, I'm requesting the following changes:

For the required command, simply add one extra option to the existing adminimmunity command

ie. `/swa immunity town|nation|all 1|2|3|4|5|6|permanent'

Once this command is run, the existing immunity figure gets set to -1 and saved.

This shows on the town screen in the existing field i.e. "Siege Immunity: Permanent"

I can also agree with this, re-using the existing metadata at -1 is the move.

All makes sense - working on it now - as for removing a town's 'unsiegeable' status, just /swa immunity town|nation|all 0 or some other command/parameter?

LlmDl · 2021-07-19T22:16:52Z

Rather than a completely new flag, I would like this new capability be added onto the existing siege-immunity system. (as I indicated yesterday in a comment in siegewar_admins)

They key idea is that permanent siege immunity is the same thing as unsiegable, only that the first has better integration with existing features, smaller changes to the code, and no need for a full new command.

Thus, as per my comment yesterday, I'm requesting the following changes:

For the required command, simply add one extra option to the existing adminimmunity command

ie. `/swa immunity town|nation|all 1|2|3|4|5|6|permanent'

Once this command is run, the existing immunity figure gets set to -1 and saved.

This shows on the town screen in the existing field i.e. "Siege Immunity: Permanent"

I can also agree with this, re-using the existing metadata at -1 is the move.

All makes sense - working on it now - as for removing a town's 'unsiegeable' status, just /swa immunity town|nation|all 0 or some other command/parameter?

I wouldn't use 0, you can accept permanent and include 'permanent' in the tab complete.

Changed to work with immunityendtime rather than a whole new flag

Goosius1 · 2021-07-20T15:23:07Z

src/main/java/com/gmail/goosius/siegewar/command/SiegeWarAdminCommand.java

 				Integer.parseInt(args[2]);
+			}
 		} catch (NumberFormatException | ArrayIndexOutOfBoundsException e) {
 			Messaging.sendMsg(sender, Translation.of("msg_error_must_be_num"));


🚧

The above translation string is currently this: '&cAmount must be a number.'

Along with the new permanent option, this text should be changed to something more suitable e.g. '&cValue must be a number or permanent.'

Yeah, wasn't sure of how to phrase it, I'll implement this in next commit :^)

This language string still needs updating

e.g. "msg_error_must_be_num" needs to be changed

I suggest removing the current string and adding a new one right at the end of the file called: msg_error_must_be_num_or_permanent

Make sure to change this in both the english.yml and french.yml files

Can do, but how should I go about getting a french translation? Just use google translate or is there someone I should message?

To update the french.yml file

Create a new comment at the bottom of the file called #Added in 23

Add the new lines there, just like in the english.yml file

(No need to translate them yourself)

src/main/java/com/gmail/goosius/siegewar/command/SiegeWarAdminCommand.java

src/main/java/com/gmail/goosius/siegewar/playeractions/PlaceBlock.java

src/main/java/com/gmail/goosius/siegewar/listeners/SiegeWarTownEventListener.java

Goosius1

.

poilet66 · 2021-07-26T03:32:15Z

Believe I have fixed grammatical errors and resolved conflicts, not terribly experienced with managing conflicts so please do look over my changes and make sure everythings all good :^)

…into unsiegeableFlag

src/main/java/com/gmail/goosius/siegewar/listeners/SiegeWarTownEventListener.java

src/main/java/com/gmail/goosius/siegewar/command/SiegeWarAdminCommand.java

src/main/java/com/gmail/goosius/siegewar/listeners/SiegeWarTownEventListener.java

src/main/resources/english.yml

src/main/java/com/gmail/goosius/siegewar/command/SiegeWarAdminCommand.java

poilet66 · 2021-07-26T14:37:31Z

Believe I've made all those changes, just need to find someone who can translate the 2 additional lang strings for me so I can do the french lang file too =]

LlmDl · 2021-07-26T14:59:31Z

Believe I've made all those changes, just need to find someone who can translate the 2 additional lang strings for me so I can do the french lang file too =]

Adding the English lines is fine, someone french will do teh translation later.

Goosius1 · 2021-07-26T17:00:41Z

src/main/resources/english.yml

+#Unsiegeable towns strings
+msg_permanent: "permanent"
+msg_error_must_be_num_or_permanent: '&cAmount must be a number or ''permanent''.'
+


🚧

The english.yml file now needs a little rearranging

Please make the following small updates:

Update the version at the top of the file to 23

Add a new comment line at the bottom of the file called #Added in 23

Move all the new lines under the new comment line

Goosius1 · 2021-07-26T17:09:06Z

Hi Poilet, nearly there! a few more small fixes and it will be ready to merge.

Goosius1

.

Goosius1 · 2021-07-27T07:32:25Z

src/main/resources/english.yml

-msg_set_siege_immunities_all: 'Siege immunities for all towns set to %s hours.'
+msg_set_siege_immunities_town: 'Siege immunity for town %s set to %s.'
+msg_set_siege_immunities_nation: 'Siege immunities for all towns in nation %s set to %s.'
+msg_set_siege_immunities_all: 'Siege immunities for all towns set to %s.'


🚧

Apologies, I mis-typed something in the last review:

I meant to write: please move both and new and modified lines down to the bottom of the language files.

This is particularly so that the translators of the existing French file, will more easily see that they have to change their translation.

Goosius1 · 2021-07-28T21:08:01Z

Hi, there's a few things still needed here.

Replace a fixed "Permanent" string with the translation (already in a comment)
Replace another fixed "Permanent" string with the translation (already in a comment)
Copy the new english lang string into the end of the french lang file (referred to in a previous comment)
Also this branch now has 1 merge conflict in the english.yml file. Pls have a look.
- Note that the version should stay at 23, it doesn't need to go to 24

Goosius1 · 2021-08-07T11:57:17Z

Closed PR as the dev appears to have stopped work on it
Stale PR's can cause tricky technical issue, plus for reducing admin overhead, I remove stale issues regularly also.
If anyone wants to pick up this task again, feel free open a new PR
If anyone want to RE-open this specific PR, ping me on discord and we can look at that (*although as noted, if it has been closed a long time, there can be technical problems with PR staleness)

Added unsiegeable towns

1aaeb0b

/swa town <town> setunsiegeable <true|false>

Goosius1 self-requested a review July 19, 2021 20:33

LlmDl requested changes Jul 19, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

poilet66 added 3 commits July 20, 2021 13:48

Revised changes for siege immunity

9ac7174

Changed to work with immunityendtime rather than a whole new flag

Removed some old code

83d7d11

More old code removal :p

0b0a5de

Goosius1 reviewed Jul 20, 2021

View reviewed changes

src/main/java/com/gmail/goosius/siegewar/command/SiegeWarAdminCommand.java Outdated Show resolved Hide resolved

Goosius1 reviewed Jul 20, 2021

View reviewed changes

src/main/java/com/gmail/goosius/siegewar/command/SiegeWarAdminCommand.java Outdated Show resolved Hide resolved

Goosius1 reviewed Jul 20, 2021

View reviewed changes

src/main/java/com/gmail/goosius/siegewar/command/SiegeWarAdminCommand.java Outdated Show resolved Hide resolved

Goosius1 reviewed Jul 20, 2021

View reviewed changes

src/main/java/com/gmail/goosius/siegewar/playeractions/PlaceBlock.java Outdated Show resolved Hide resolved

Goosius1 reviewed Jul 20, 2021

View reviewed changes

src/main/java/com/gmail/goosius/siegewar/listeners/SiegeWarTownEventListener.java Show resolved Hide resolved

Goosius1 requested changes Jul 20, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

poilet66 and others added 2 commits July 26, 2021 04:28

Fixed grammatical errors

af25edf

Merge branch 'master' into unsiegeableFlag

b9f7c60

poilet66 added 2 commits July 26, 2021 04:38

Slightly edited message formatting to better accommodate future usage

bad17d0

Merge branch 'unsiegeableFlag' of https://github.com/poilet66/SiegeWar …

c0a1435

…into unsiegeableFlag