Fix missing UserString() #1

Merged
merged 1 commit into from Apr 2, 2015
@vincele

A colony building, blocked by an habitability condition is showing as :
"...that is not a uninhabitable planet for the species SP_ABADDONI"

That SP_ABADDONI should be translated, so add the missing UserString()
call.

Signed-off-by: Vincent Legoll vincent.legoll@gmail.com

@vincele vincele Fix missing UserString()
A colony building, blocked by an habitability condition is showing as :
"...that is not a uninhabitable planet for the species SP_ABADDONI"

That SP_ABADDONI should be translated, so add the missing UserString()
call.

Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
d910c96
@adrianbroher
FreeOrion member

The species_str should not always be translated. The old version is already fine as it is.

You mean that this is the way to fix it :

if (m_species_name)
     species_str = UserString(m_species_name->Description());
FreeOrion member

You mean that this is the way to fix it

Not really. The Description member function guarantees to return an already translated description of the object. This is because only the Description implementation can know how to translate a potentially composed description text.

That's why this pull request doesn't fix this bug properly. The instance of whatever m_species_name is, possibly a Species instance, should return a properly translated string. If it doesn't the bug lurks somewhere in the code of said instance.

@adrianbroher adrianbroher self-assigned this Mar 30, 2015
@geoffthemedio
FreeOrion member

In this case, the Description function being called is a member of ValueRef::Constantstd::string which does not (and cannot) translate the string before returning, because it doesn't know what kind of string it's returning.

And sometimes m_species_name will be a ValueRef::Constantstd::string constant, such as "SP_ABBADONI", but sometimes it will be a human-readable constructed description like "the species on the source planet".

What might need to happen is for the code in PlanetEnvironment::Description (and similar places) to check if the ValueRef::ValueRefBasestd::string that it's getting a description for is a constant ValueRef. If so, it can check if the string is in the stringtable. If it is, then it can insert the translated string in the the description it's generating. Otherwise, use whatever ->Description() returns without attempting to lookup with UserString().

@vincele

@adrianbroher : I see you self-assigned that to yourself, does that mean you're going to fix it and I shouldn't bother digging further ?

To all, thanks for the explanation, that surely helps to get good directions to follow...

@Dilvish-fo
FreeOrion member

Another possibility regarding m_species_name would be to check if the SpeciesManager will return a species with that name, if so then translate it, and if not, then don't translate it.

Sidenote: I notice that Geoff's comment only appears here, whereas adrian's and vincele's also appear directly in the code view area. Is that because those comments were entered there instead of being entered directly here? Is that a preferred practice, to comment with the code and have that comment get pulled here automatically?

@adrianbroher
FreeOrion member

I see you self-assigned that to yourself, does that mean you're going to fix it and I shouldn't bother digging further ?

No, feel free to investigate further and possibly fix the bug. I just assigned myself to the pull request to make clear, that I am responsible for reviewing and merging this PR/bug into the repository.

@Dilvish-fo

Is that because those comments were entered there instead of being entered directly here? Is that a preferred practice, to comment with the code and have that comment get pulled here automatically?

Use whatever makes sense in you context. I commented on the commit because I was referring to the actual code change.

@vincele

I've been staring at Condition.cpp for an hour or so, and cannot understand what is different between the conditions that properly translate the species name from the PlanetEnvironment one...

Screenshot showing the bug

@vincele

Or maybe that's it :

diff --git a/universe/Condition.cpp b/universe/Condition.cpp
index f595ebf..78b3c44 100644
--- a/universe/Condition.cpp
+++ b/universe/Condition.cpp
@@ -3278,7 +3278,9 @@ std::string Condition::PlanetEnvironment::Description(bool negated/* = false*/)
     }
     std::string species_str;
     if (m_species_name)
-        species_str = m_species_name->Description();
+        species_str = ValueRef::ConstantExpr(m_species_name) ?
+                        UserString(boost::lexical_cast<std::string>(m_species_name->Eval())) :
+                        m_species_name->Description();
     if (species_str.empty())
         species_str = UserString("DESC_PLANET_ENVIRONMENT_CUR_SPECIES");
     return str(FlexibleFormat((!negated)
@vincele

I've pushed another branch with that :
vincele@4a41a5b

@geoffthemedio
FreeOrion member

Re: "that", probably yes. As I wrote, "...check if the ValueRef::ValueRefBasestd::string that it's getting a description for is a constant ValueRef. If so, it can check if the string is in the stringtable."

@geoffthemedio geoffthemedio merged commit 38a17ee into freeorion:master Apr 2, 2015
@geoffthemedio
FreeOrion member

Please make another pull request then, so if / when I attempt to merge, I get what I wanted instead of whatever is left in this pull request... (Why make a separate branch?)

@adrianbroher
FreeOrion member

@geoffthemedio Please don't take PRs from others.

Instead of merging letting vincele doing a rebase would have been perfectly fine.

@geoffthemedio
FreeOrion member

"Don't take pull requests"? What's the point of having them then? And wouldn't he need to make a pull request so I can merge it into master with him getting proper credit in git? What difference does it make if he does a rebase vs. makes a new branch with an updated version of the changes?

@adrianbroher
FreeOrion member

"Don't take pull requests"? What's the point of having them then?

The PR was assigned to me. It's not about having PRs or not.

What difference does it make if he does a rebase vs. makes a new branch with an updated version of the changes?

A new pull request creates a new thread/discussion, which is unrelated to this thread/discussion.

@geoffthemedio
FreeOrion member

Ah, OK: it's a turf issue, not a mechanics-of-github issue. The meaning of "take" was unclear to me.

@adrianbroher
FreeOrion member

it's a turf issue

Of course it is a turf issue as "from others" already indicated.

@geoffthemedio
FreeOrion member

I initially thought "the others" were people submitting the pull requests.

@vincele

I did another branch because I didn't know what I should do with this one, and also to keep the discussion here, I did not do another PR for the new branch...

What's the preferred way of handling this ?

I'd personally prefer the pullable branch to only have good commits, and not a messed history...

Anyways, I'll prepare a new patch with the added code to check if the string is translatable, and then follow your instructions on how to get it upstream...

@vincele

Would that be what you wanted ?

@@ -3277,8 +3278,14 @@ std::string Condition::PlanetEnvironment::Description(bool negated/* = false*/)
         }
     }
     std::string species_str;
-    if (m_species_name)
-        species_str = m_species_name->Description();
+    if (m_species_name) {
+        if (ValueRef::ConstantExpr(m_species_name)) {
+            species_str = boost::lexical_cast<std::string>(m_species_name->Eval());
+            if (UserStringExists(species_str))
+                species_str = UserString(species_str);
+        } else
+            species_str = m_species_name->Description();
+    }
     if (species_str.empty())
         species_str = UserString("DESC_PLANET_ENVIRONMENT_CUR_SPECIES");
     return str(FlexibleFormat((!negated)
@geoffthemedio
FreeOrion member

m_species_name->Eval() should return std::string so I don't think there's any need to cast to std::string...

@vincele vincele deleted the vincele:fix-missing-userstring branch Apr 3, 2015
@adrianbroher adrianbroher added this to the Gateway to the Void milestone Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment