City/District Strike Button Tweaks

[JamieNyanchi]

Quick Explanation

CQUI makes changes to the City/District strike buttons. The most obvious is the movement of the button locations. However, more changes than that are made, but there are bugs with the implementations. This is purely a bug fix update. No new features are added in this PR. The goals/changes of this PR as as follows:

1. Ensure the strike view is always entered and exited on mouse click 2. Clicking on another strike button when one is currently selected properly opens the new strike view 3. Entering the strike view always shows the strike range and attack arrow 4. Selecting a unit while currently in the strike view exits the strike view and shows the unit panel 5. Encampment strike button is now updated on settings change 6. Updated action panel encampment ranged attack text to be consistent with the city ranged attack text 7. The action panel now ignores city centers when looking for a district that can do a ranged attack

Detailed Explanation

Detailed explanation of changes

--- **1.** Ensure the strike view is always entered and exited on mouse click. * Base game behavior: Clicking on a strike button opens the strike view. Clicking on it again does not close it. * Current CQUI behavior: The city strike button opens/closes the strike view on click. The district strike button *only* opens. * Expected CQUI behavior: Both the city strike button and district strike buttons should open/close the strike view on click. * Files changed: `CityBannerManager_CQU.lua`, `actionpanel.lua`, `citypanel.lua`, `NotificationPanel_CQUI.lua` --- **2.** Clicking on another strike button when one is currently selected properly opens the new strike view. * Base game behavior: Clicking on another strike button changes the strike view to that city/district. * Current CQUI behavior: When in a city strike view, clicking a district or other city strike button closes the view and does not open a new one. When in a district strike view, clicking a city strike button properly closes and opens a new one. * Expected CQUI behavior: Both the city/district strike buttons should close their views and open a new one when selecting on another strike button. * Files changed: `CityBannerManager_CQUI.lua` --- **3.** Entering the strike view always shows the strike range and attack arrow. * Base game behavior: Entering the strike view always shows the strike range and attack arrow. * Current CQUI behavior: Entering the strike view when a lens was previously active will not show the strike range and attack arrow. * Expected CQUI behavior: Entering the strike view should always show the strike range and attack arrow. * Files changed: `minimappanel.lua` --- **4.** Selecting a unit while currently in the strike view exits the strike view and shows the unit panel. * Base game behavior: Selecting a unit from the city strike view hides the unit panel. Selecting a unit from the district strike view does not close the strike view. * Current CQUI behavior: Selecting a unit from any strike view hides the unit panel. * Expected CQUI behavior: Selecting a unit from any strike view should always show the unit panel. * Files changed: `UnitPanel.lua`, `worldinput_CQUI.lua` --- **5.** Encampment strike button is now updated on settings change. * Base game behavior: N/A (Doesn't have strike button settings). * Current CQUI behavior: Only the city strike button's location is updated on settings change. The district strike button is not changed. * Expected CQUI behavior: Both strike buttons should update their location when the settings are updated. * Files changed: `CityBannerManager_CQU.lua` --- **6.** Updated action panel encampment ranged attack text to be consistent with the city ranged attack text. * Base game behavior: N/A (Doesn't have encampment ranged attack notifications). * Current CQUI behavior: > City Strike Notification: CITY RANGED ATTACK - Your city can perform a ranged attack. > District Strike Notification: Encampment Ranged Strike - Your encampment is able to make a ranged strike. * Expected CQUI behavior: > City Strike Notification: CITY RANGED ATTACK - Your city can perform a ranged attack. > District Strike Notification: ENCAMPMENT RANGED ATTACK - Your encampment can perform a ranged attack. * Files changed: `cqui_text_general.xml` --- **7.** The action panel now ignores city centers when looking for a district that can do a ranged attack. * Base game behavior: N/A (Doesn't have encampment ranged attack notifications). * Current CQUI behavior: When clicking the district ranged attack notification, sometimes a city center will be selected instead of a district. * Expected CQUI behavior: When clicking the district ranged attack notification, districts other than the city center should be selected. * Files changed: `actionpanel.lua`

[Infixo]

Please stop making such massive changes in one PR. It is difficult to process these huge explanations. Like before - please specify first what the actual issue is and how the game behaves normally, with no CQUI.

CQUI is supposed to change only the location of the button. This is it. Why 8 tweaks? Is the vanilla implementation so broken or did someone break CQUI's code?

[JamieNyanchi]

I put these all in one PR as they are all closely related. Would it be preferred to split it into several PRs? I'd be willing to do it, but that feels a bit excessive for several closely related changes. As for my explanations, I did try to explain what the issue was and what the PR does. I broke it down into several points to try to make it clear what all the code changes do, but if it's not helpful the way it is, I can try to restructure how I write these. My goal is, after all, to make it clear and easy to review and I am open to changing how I submit changes if it would make your review easier.

In the future, instead of doing paragraph explanations of each point, I can try this instead:

1. One sentence explanation of change A. Description of original behavior (base game's behavior and CQUI's) B. Description of problem C. Description of changes

This would ultimately be the same information, but hopefully presented into a nicer way? I can also split the commit into several small commits. I am used to having to merge them into one single commit for other repositories, but if that isn't an issue here, I'd be happy to leave them as separate commits.

As for why there are 8 tweaks, they're more related than that I'd say. I broke it into 8 points to try to clearly explain the intention of the entire PR. Two of these changes address vanilla bugs, yes. Those would be point 4 and the end of point 1 As for the rest, they are issues with CQUI's and MoreLenses' (in the case of 3) implementations. Most of the issues are with the implementation of the Encampment Strike Button in particular.

I made my changes based off the comments in the code itself and based off of inconsistent behavior while playing. Like with 1, clicking on the city strike button multiple times opens/closes it, but that didn't happen with the encampment. Based off the comments in the code, it was intended that it would open/close it, but did not work because of how it was implemented.

Another example would be 7. The code explicitly allows for right clicking away the city strike notification, but did not properly ignore city centers if that was the case. This would lead to you being selected on a city center when you click the encampment strike notification.

In the case of 6, this isn't really an issue, but rather just an inconsistency. I felt the encampment strike notification text should match the same formatting (such as the all caps) as the other notifications.

CQUI clearly intended to do more than just move the button and my goal was just to fix the implementation that was made and make the behavior consistent.

[JamieNyanchi]

I still think grouping all of these changes in one PR is appropriate given that they are all very closely related and there are no new features added. However, I agree that the formatting of the explanations can be better. I have rewritten the entire original post now to hopefully make it easier to digest. Please let me know what you think.

[Infixo]

1. Ensure the strike view is always entered and exited on mouse click [ok]
2. Clicking on another strike button when one is currently selected properly opens the new strike view [ok]
3. Entering the strike view always shows the strike range and attack arrow [ok]
4. Selecting a unit while currently in the strike view exits the strike view and shows the unit panel [ok - this was problematic sometimes]
5. Encampment strike button is now updated on settings change [ok]
6. Updated action panel encampment ranged attack text to be consistent with the city ranged attack text [see below]
7. The action panel now ignores city centers when looking for a district that can do a ranged attack [see below]

Ad. 6 What about Oppidum? What message will appear if Oppidum can attack? Actually, any district could attack, if modded. What message would be displayed then? If district name can be used then the message should/could be: District Strike Notification: XXX RANGED ATTACK - Your XXX can perform a ranged attack. If not, then more generic: District Strike Notification: DISTRICT RANGED ATTACK - Your district can perform a ranged attack.

Ad. 7 What happens when both the center and a district can shoot? Are there 2 notifications or just 1? If 1, then I'd prefer the city center to be the default one. If 2, then I understand this fixes a bug somewhere?

[Infixo]

I put these all in one PR as they are all closely related. Would it be preferred to split it into several PRs?

I'd rather have an issue registered first, because that actually tracks the origin of a change/PR. Once the issue is cleared (i.e. understood, accepted, etc.), then no extra explanations are needed in the PR unless asked for. I think this just saves the time spent on writing explanations.

Also, you can fix few issues in a single PR if you want and even put more than 1 issue in one ticket, which generally is not recommended but if the issues are really, really similar, then I prefer flexibility. Like points 1,2,3 maybe could be 1 issue and the other ones probably separated.

Last but not least. Big PR takes more time to process. One small issue can block the entire PR.

[JamieNyanchi]

Thank you for your responses! So it'd be ideal for me to open an issue post before making a PR? I can do that for sure! Sorry for not doing that so far. I often just find these small bugs while playing and I get excited to fix them as soon as possible, which makes me overlook opening issues.

Regarding 6, this is a good point! I originally just went with the text that was already there, which only used the term "Encampment", but I like the idea of using the actual district name! I have updated the code to try to get the actual district name. If that fails, it'll just use the localized word for "District" instead. Only English is updated since I don't know the other languages, but the Lua code should work for all languages if and when the others are updated. While doing this change, I also replaced mentions of "Encampment" with "District" in actionpanel.lua to be more consistent.

Regarding 7, there are two separate notifications in this case. The base game only has the one notification for City Ranged Attack, but CQUI adds in the District Ranged Attack notification. The City Ranged Attack notification takes priority and will show before the District Ranged Attack notification. You have to do your city range attacks or right-click away the City Ranged Attack notification to see the District Ranged Attack notification (Right-clicking this notification is base game behavior that CQUI preserved). Currently in CQUI, if you were to right-click away the City Ranged Attack notification though, clicking the District Ranged Attack notification could select you on a City Center district. Since these are two separate notifications, and the user has to explicitly right-click away the City Ranged Attack one, I think the District Ranged Attack notification should only select districts that aren't the city center, which is what this PR does.

[Infixo]

Ad 7. OK. Thank you for the explanation.