Add color=value to [message]

[Pentarctagon]

Made this request, then realized I could do it myself.

[CelticMinstrel]

Well, I'm not against something that means less explicit Pango markup... though a better markup would be even better.

[Vultraz]

Can't you simplify it to this:

local function add_formatting(cfg)
    -- if no message, do nothing
    if cfg.message == nil then
        return nil
    end

    -- add color
    if cfg.color then
        return string.format("<span color='%s'>%s</span>", cfg.color, cfg.message)
    end

    -- or return unmodified message
    return cfg.message
 end

I think it's clearer to read.

[Pentarctagon]

On one hand, I do agree that your code is clearer. On the other hand, the intent I had when writing it as I did was that it could be easily expanded to cover all pango markup options with a simple copy/paste/edit. So supporting fonts, for example, would look like this:

 local function add_formatting(cfg)
    -- span tag
    local formatting = "<span"  

    -- if no message, do nothing
    if cfg.message == nil then
        return nil
    end

    -- add color
    if cfg.color ~= nil then
        formatting = formatting .. " color='" .. cfg.color .. "'"
    end

    -- add font
    if cfg.font ~= nil then
        formatting = formatting .. " font='" .. cfg.font .. "'"
    end

    -- wrap in span tags and return if a color was added
    if formatting ~= "<span" then
        return formatting .. ">" .. cfg.message .. "</span>"
    end
    -- or return unmodified message
    return cfg.message
end

[Vultraz]

That's a point, though I don't know if it's a good idea to add more markup shortcut tags. I'd prefer if we kept to raw markup.

[Pentarctagon]

I suppose. Though in the situation where the intent is to apply markup to the whole message, wrapping the entire thing in a <span> tag seems (in my opinion) to be rather boilerplate-ish. For short messages especially, you can end up with the <span> markup being longer than the actual text of the message.

[CelticMinstrel]

I hate the raw markup, so I wouldn't be strongly opposed to adding more format options to [message]. However, the downside of implementing format options in this way is that it only applies to [message], and if you want to add similar support to other tags, you'll need to do that separately. (Which could mean reimplementing it, or potentially moving the current function to wml-utils.lua and making it a little more generically usable.)

It'd be nice if we could use something simpler such as Markdown for formatting, but that's a separate issue.

[Elvish-Hunter]

return string.format("<span color='%s'>%s</span>", cfg.color, cfg.message)

cfg.message is usually a translatable string, which Lua sees as a userdata type. This means that using it in a string formatting won't work, unless cfg.message is first wrapped in a tostring() call.

if cfg.message == nil then

That can be simplified as just if cfg.message then.

It'd be nice if we could use something simpler such as Markdown for formatting, but that's a separate issue.

Actually, until version 1.6 we used to have something to that effect. I'm copying this from an old version of our InterfaceWML wiki page:

A tilde (~) as the first character causes the line to be boldfaced.
An at symbol (@) as the first character causes the line to be green, as done with victory conditions.
A pound symbol (#) as the first character causes the line to be red, as done with defeat conditions.
An asterisk (*) as the first character causes the line to be bigger.
A backquote (`) as the first character causes the line to be smaller.
If used, the caption key text is boldfaced.
An RGB colour code in the beginning causes the line to be the given colour. This can still be preceded by the above characters. Example: message=_"<255,0,0>Red!"

[CelticMinstrel]

I think the translatable string metatable does include __tostring though, so it might not be necessary to wrap it?

Actually, until version 1.6 we used to have something to that effect.

That's also a terrible system though, because it only checks the first character. (Or maybe the first character of every line?) It's not a proper markup syntax. Still, something based on that would be better than the Pango markup in my opinion.

[Pentarctagon]

I updated the add_formatting function to simplify it a bit, as well as to make it more generic - it is now able to wrap any string passed to it in a <span> tag.

As for the whole markup debate, I would probably have still submitted the PR regardless of what was used, since it will always look cleaner to separate the meaningful text from the description of how to display the text. Ultimately, I think the best solution would be if we could make our own stylesheets, so instead of things like <span color='#BCB088' size='large' font-style='italic'>You are victorious!</span> you would write <style id='win'>You are victorious!</style> or something similar.

I'd still be here asking for a style=value attribute though.

[Pentarctagon]

I don't mean to nag or anything, but is there something else I need to do for this to move forward? Or do I just wait?

[CelticMinstrel]

Mostly I think it's just waiting for someone to get around to it.

It would be good to add changelog and (if this is your first contribution) credits entries though.

[Pentarctagon]

And, um... how would I go about adding files to this pull request?

[Vultraz]

What do you mean? If you're adding files, just commit them regularly and push them to the forked branch.

[Pentarctagon]

I've made the changes to my forked branch to changelog and about.cfg, but they aren't showing up here.

[Vultraz]

Are they in the Pentarctagon-message-color branch? Did you make sure to push the changes to that branch?

[Pentarctagon]

Well... I can't anymore, actually. I merged that into my fork, and it closed the pull request I used to open this one. In fact the 2nd and 3rd commits made to message.lua here aren't even in my fork.

[Vultraz]

You need to have the original base branch in order to add new commits, I think.

[ln-zookeeper]

While a shortcut like that is convenient, I'm a bit bothered by how it's different from every other color-defining key. Others such as [label], [unstore_unit], [print] and such always let you specify the color as normal 0-255 RGB (either as color= or red,green,blue=), whereas this one breaks that rule and only takes a pango color.

So I'd say this would need to take the color as 0-255 RGB and pango-specific syntax shouldn't "leak" outside the actual pango markup.

[Pentarctagon]

So you're saying it should allow both color= and red,green,blue= ? Or just one of those?

Also, should it specifically not support hex colors and only allow decimal RGB?

[Vultraz]

color= is enough. Don't use red= green= blue=.

[Pentarctagon]

Added support for RGB decimal values. The follow code all displays as expected:

[event]
  name="start"

  [message]
    message="hello1"
    color="34,35,56"
  [/message]

  [message]
    message="hello2"
    color="green"
  [/message]

  [message]
    message="hello3"
    color="#0000ff"
  [/message]

[/event]

edit - unless there are any objections, I will add support for all the available pango markup attributes as well.

[CelticMinstrel]

1. What the heck is fallback? Maybe a bit more descriptive attribute name would be good.
2. Are some of those boolean attribtues? Did you test that they actually work?

[Pentarctagon]

1. All message attribute names were kept the same as the pango attribute names from the wiki.
2. Tested yes, but I forgot to commit the fixed code.

[Pentarctagon]

Also, I read through the whole PR for changing the copyrights, and if this ends up committed, the copyright can go to the BfW project.