Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tooltip on leader icon with its side's color name (GNA #16116) #1217

Closed
wesnoth-bugs opened this issue May 8, 2017 · 27 comments · Fixed by #4366

Comments

@wesnoth-bugs
Copy link

@wesnoth-bugs wesnoth-bugs commented May 8, 2017

Original submission by anonymous on 2010-06-04

A tooltip on the leader icon that shows its side's color name would be useful for color blind folks who can't tell the difference between black and purple (and other colors). In on-line multiplayer games, people frequently address each other by their colors (e.g. "purple, go caves").

(Reproduced on GNU/Linux)
Release: 1.8.2
Priority: 5 - Normal
Severity: 1 - Wish

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 5, 2019

Not sure why I kept selecting Campaign label by mistake. Also I know this isn't specific to MP but it's more relevant for that mode.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 11, 2019

That's actually easy:

patch
--- a/src/reports.cpp
+++ b/src/reports.cpp
@@ -233,6 +233,7 @@ static config unit_side(reports::context & rc, const unit* u)
        std::ostringstream tooltip;
        if (!u_team.side_name().empty()) {
                tooltip << _("Side:") << " <b>" <<  u_team.side_name() << "</b>";
+               tooltip << " (" << team::get_side_color_id(u->side()) << ")";
        }
        add_image(report, flag_icon + mods, tooltip.str(), "");
        add_text(report, text.str(), tooltip.str(), "");

2019-09-11-142425_175x146_scrot

And it could be added to the status table too with little effort. I suppose we need an advanced preference or something, though?

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 11, 2019

Looks good. Is there a problem with leaving it on permanently?

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 12, 2019

It's redundant for the majority of players.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 12, 2019

Hmm, but tool-tips don't show unless the cursor is hovered over the item. Yes, it might be unnecessary but I don't think it's a huge deal. But I suppose it's nicer to have the option to switch them off.

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

@ProditorMagnus ProditorMagnus commented Sep 12, 2019

People who need this might not notice this preference. I think showing it always in that tooltip is good enough. Most people just have no need to regularly use that tooltip.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 13, 2019

If we have a consensus that making this optional may not be necessary, can/should we just add this?

@jostephd jostephd self-assigned this Sep 13, 2019
@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 13, 2019

I also got feedback in private to the effect of "options are bad". If no one disagrees, I'll assume this should be done without a new preference.

As implemented, the color name will be in English regardless of the UI language. Would that be a problem?

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 13, 2019

I wouldn't say bad... but sometimes they may not be really necessary. Hmm, I remember Vult used to say options are bad. XD

I'm happy for this to be implemented without the option (was going to have a go at it myself later but go ahead and do it now if you can). As for translations... it would seem inconsistent to not translate it. Is it a lot of extra work to make translations available for the colours? (We don't have translated colours already?)

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

@ProditorMagnus ProditorMagnus commented Sep 13, 2019

From the code it looked like it would show color id not name. And custom colors do not have English name even.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 13, 2019

The image seems to show that it's displaying the colour correctly.

There are custom colours for teams (as in, you can choose the colour itself, not just choose a specific pre-defined colour for a given team)? If that's the case that would cause complications for this idea. And I was going to suggest that having some colour text would be better than none at all in the case that only English colour names are available.

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

@ProditorMagnus ProditorMagnus commented Sep 13, 2019

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 13, 2019

And I was going to suggest that having some colour text would be better than none at all in the case that only English colour names are available.

When you're colorblind, sure. But when you're not colorblind and see red (in English) or Rav_pink in the middle of the UI, that's kinda odd :)

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 13, 2019

Hmm, so I wonder how did jostephd get the result in his screen shot, then.

Edit: Near simultaneous post. Then maybe we leave this as low priority then.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 13, 2019

Hmm, so I wonder how did jostephd get the result in his screen shot, then.

I posted the patch I used right next to the screenshot. It just puts the color id there, but the color id is "red" which is an English word, so it all looks fine.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 13, 2019

I see, so it's dependent on the IDs being self-descriptive.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 13, 2019

It's as you said:

having some colour text would be better than none at all in the case that only English colour names are available.

@soliton-

This comment has been minimized.

Copy link
Member

@soliton- soliton- commented Sep 13, 2019

I would display the translatable name instead of the id. If necessary there could be a fallback to the id if no name is given.

"Options are bad" means there is always a trade-off to consider when adding an option since it causes an implementation and maintenance burden. If the benefit of the option is big enough it can still be implemented.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 13, 2019

https://github.com/ProditorMagnus/Color_Modification/blob/master/color_mod.cfg#L1 only id and color values exist.

Names do exist:

[color_range]
id=red
rgb=FF0000,FFFFFF,000000,FF0000
name= _ "Red"
default=yes
[/color_range]

So we should be able to use the name if specified, and fall back id otherwise - as @soliton- said while I was writing this :)

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

@ProditorMagnus ProditorMagnus commented Sep 13, 2019

name: A translatable name for the color. This may be ignored for non-core colors?

Documentation can be updated then.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 13, 2019

"Options are bad" means there is always a trade-off to consider when adding an option since it causes an implementation and maintenance burden. If the benefit of the option is big enough it can still be implemented.

I understand that, it just sounds like a blanket statement to say you should 'never' add options.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 13, 2019

I just noticed this enhancement could benefit #2682. While I can still see the difference between blue and purple there, clearly it's difficult for some to distinguish them even if they would not normally consider themselves colour-blind.

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 14, 2019

Documentation can be updated then.

Honestly, I fail to see how "may be ignored for non-core colors" could possibly be true. It's not like there's a boolean field in the parsed config object that says whether the color range had been parsed from data/core or from ~add-ons, after all...

So, yeah, the plan is:

  1. Use the name of the color_range, if available, else id
  2. Add it to the sidebar, top bar, and game status dialogs
  3. No new preference
@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 16, 2019

Use the name of the color_range, if available, else id

Translated color names are stored in game_config::team_rgb_name.

@Wedge009

This comment has been minimized.

Copy link
Member

@Wedge009 Wedge009 commented Sep 17, 2019

Thanks. I know I said I'd revisit this by Tuesday but I don't think I'll have time this week - though I do plan to come back to it (and a number of other issues I've marked for myself to look into) at some point (this year I mean).

@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 17, 2019

Thanks for the info. I might beat you to it, then. We'll see.

jostephd added a commit to jostephd/wesnoth that referenced this issue Sep 19, 2019
@jostephd

This comment has been minimized.

Copy link
Member

@jostephd jostephd commented Sep 19, 2019

PR: #4366

jostephd added a commit that referenced this issue Sep 26, 2019
For colorblind MP players

Fixes #1217
jostephd added a commit that referenced this issue Sep 26, 2019
jostephd added a commit that referenced this issue Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.