Yes. There needs to be a linebreak character at the end of the last line.

Yep. Just make sure to add a newline to the end of all the files.

Thanks @Stan
@ShadowOfHassen you have the necessary permissions. Just click the "Update Diff" button in the top right of the page.
I'll see if I can help with D1730 as well. Maybe we can finish both now.

Well, it was just an idea and is (of course) not technically necessary by any means.

In D5297#226050, @Stan wrote:

Well just like my inline suggestion this will only work if the value needs not be updated, calling the function each time ensures the function is fresh, if you have a global, you need to ensure it's updated correctly.

In D5297#225881, @Vantha wrote:

I noticed that controlsPlayer(g_ViewedPlayer) is (maybe unnecessarily) called from many points in the code (in multiple scripts) individually. Maybe we should (in a separate diff) create a global bool g_ControlsViewedPlayer that can be checked directly to avoid repetition and unnecessary function calls. What do you think?

What do you think about that?

Yeah, my bad. Not sure how I missed those.

In D5269#226011, @Stan wrote:

Yeah been busy with other things.

No problem, I understand that. Just meant as a friendly reminder:)

This ready to be commited, no?
I've already written the olist page for the wiki and added a reference to this revision into the official mod porting guide.

This would work in CList.cpp but not in COList.cpp. Specifically because of

return selected && *m_TextColorSelected ? m_TextColorSelected : m_TextColor;

This is because in olists each column has its own textcolor and textcolorselected properties (column.m_TextColor and column.m_TextColorSelected) that need to be used in place of m_TextColor and m_TextColorSelected (see COList.cpp:443). So, if we used a GetTextColor function we'd need to pass those to it as well.

In D5269#225664, @phosit wrote:

In D5269#225648, @Stan wrote:

@phosit Does it make sense to have a GetTextColor function in CList that can be used in COList to reduce duplication? Can probably be handled in a separate patch.

Indeed, It should be done in this diff.

Are you sure that's worth it? The function would execute exactly one single line and we'd need to pass TextColor, TextColorSelected, i, (and possibly DrawSelected) to it.

Ok, I rebased it and changed colorSelected to textcolor_selected.

Nice, thanks

@phosit @sera @Stan @vladislavbelov Hey, sorry for the ping. Is there anything left to change here or in D5269? If not, can someone please review the diffs and commit them?
After that, I can also update the guide for porting mods to A27.

This diff is pretty simple. I don't think there's much to discuss here. Is it ready to be merged?

Is there anything else left to change?

I added D5278 as a parent and will update this diff as soon as that is committed.

In D5278#225245, @vladislavbelov wrote:

After its commit https://trac.wildfiregames.com/wiki/PortA26ToA27 should be updated accordingly.

Sure, I can do that.

Sorry for the late reply.
I just succesfully tested it. These should now be all files that need to be changed. As far as I can tell it's complete.

In D5269#224957, @Stan wrote:

I wonder how many files are affected.

Not that many. From a quick count, there are around 10 files with olists in the game whose column's property would have to be changed, plus around 3 files implementing the feature.

Ok. How about I leave it at "colorSelected" for now and then open a new diff to rename "color" to "textcolor" and "colorSelected" to "textcolor_selected"?

Does the font color of columns have to named color though? For all other GUI elements it is textcolor. What about renaming it to this? We could then also use the name textcolor_selected.

In D5269#224895, @Stan wrote:

color is the standard css name for font color.

Ok, that makes sense.
But I don't quite understand what you're getting at...

The textcolor attribute of columns is just called "color", so I figured its selected variant should be called "colorSelected"

Ok, I changed it accordingly.

In D5269#224839, @phosit wrote:

Sorry for not writing earlier.

In D5269#224802, @Vantha wrote:

In D5269#224746, @phosit wrote:

In this patch i wouldn't change *::Draw at all. And i wouldn't change the signature of *::DrawList.

@phosit Why not? Wouldn't that be the most logical solution?

When you change the signature you have to change other files like CDropDown.
Also i'm shy of changing signatures in class hirarchies. When you forget to change something, it might silently select the function from the parrent.

So you are recommending to not pass m_TextColorSelected to DrawList and instead read it directly? Isn't this way a bit inconsistent when compared to the handling of all other properties?

In D5269#224746, @phosit wrote:

In this patch i wouldn't change *::Draw at all. And i wouldn't change the signature of *::DrawList.

@phosit Why not? Wouldn't that be the most logical solution?

In D5269#224745, @Vantha wrote:

What is the best solution?

In this patch i wouldn't change *::Draw at all. And i wouldn't change the signature of *::DrawList.

Are you sure? That would mean accessing m_TextColorSelected directly but all other properties (including m_TextColor) through parameters of the function.

We don't need both the functions COList:Draw() and COList:DrawOList, we can merge them into one overriding CList:Draw().

We can do that in a future diff.

I mean, it's not unrelated to this diff. The diff is editing COList::Draw(O)List anyway.

In D5269#224742, @phosit wrote:

In D5269#224739, @Vantha wrote:

As far as I'm understand, the code calls the Draw() function first for each GUI object. CList::Draw() then calls CList::DrawList().

Does CList::Draw() _only_ call CList::DrawList(). Yes it does (weird/useless function).

I think the awkward calls and overrides of the two functions Draw() and DrawList() are done, so the dropdown can execute code before drawing the list.

In D5269#224738, @phosit wrote:

In D5269#224735, @Stan wrote:

@phosit any reason we're casting to unsigned int rather than size_t?

No it's better to cast to size_t.

Ok, I changed that.

Code which previously called COList::DrawList doesn't find that anymore and calls CList::DrawList instead. Which is wrong i think.

As far as I'm understand, the code calls the Draw() function first for each GUI object. CList::Draw() then calls CList::DrawList(). COList previously ìnherited Draw()and only overrode DrawList(). But now with the extra property textColorSelected that OList doesn't need, we can remove that parameter from its drawing function. I therefore made COList also override Draw() to now call the new function COList:DrawOList() instead, with parameter less than previously CList:DrawList().

In D5269#224730, @phosit wrote:

In D5269#224657, @Vantha wrote:

removed the extra parameter textColorSelected from COList::DrawList()

You didn't do that. Right?

I did. See COList.cpp line 305 and COList.h line 76.

Ok, I wrote the page (https://trac.wildfiregames.com/wiki/GUI_-_OList) and also added the missing properties here: https://trac.wildfiregames.com/wiki/GUI_-_Properties and the reference here:https://trac.wildfiregames.com/wiki/GUI_-_Scripting_Reference

In D5269#224721, @Stan wrote:

Most of the GUI pages are outdated so it's very possible the page may be not created. Do you want me to create one so you can edit it?

Is the code fine as it is or does something else need to be changed?

removed the extra parameter textColorSelected from COList::DrawList()

This should prevent the compiler warnings.

updated the copyright years

Ok. I removed the whitespace.

Thanks.
Are the problematic whitespaces still there? I couldn't find them in the files, but the inline comment is gone now.

Nice. Is there anything else you want me to change?

In D5256#224195, @Norse_Harold wrote:

elexis said that this revision should not be marked as closing ticket #3307 because it doesn't resolve all of that ticket. So, remove the reference to ticket #3307 in the ticket field. Then add a comment to the description stating that this resolves part of ticket #3307.

I addressed most of the suggestions.

Ok, updated the diff. Is this how you wanted the comments?

Like this?

I added/updated the comments. Do you also want a comment before line 95? This.CountLabel is already marked for translation at this point (and has a comment explaining what it is).

Ok, nice. Made the change and added the trac ticket.

Ok, I made the change.

OK, and what's preferred?

In D5256#223808, @phosit wrote:

Before this diff the "forat translation" is allways done. That's almost certainly an issue.

Not sure what you mean by this, though. What should I change?

In D5256#223797, @phosit wrote:

I don't like it when data is mutated. (With the +=)
If one wants to know what's in the variable one has to reason about every point where it get's mutated.
If it's const one only has to reason abot the initialization.
Ideally there should be a chosenName and a translatedChosenName:

const translatedChosenName = this.settings.isNetworked ? chosenName : translate(chosenName);

elexis just wrote me again :)
User visible strings shouldn't be concatinated by + (or +=).

In D5256#223783, @phosit wrote:

I never saw Template Literals in this codepase. I don't object it. :)

The Mauryan emperors were sorted chronologically. I put them in alphabetical order too.

When the AINames are empty or don't exist, the game already throws errors before the step of choosing the names.

changing three let to const