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.

Sign up for GitHub

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

Fix footsteps translation #37402

Merged
merged 11 commits into from
Jan 27, 2020
Merged

Fix footsteps translation #37402

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e3bb1b3
Merge pull request #1 from CleverRaven/master
8street Dec 7, 2019
b5d17e9
Merge pull request #2 from CleverRaven/master
8street Dec 14, 2019
df84d2a
Merge branch 'master' of https://github.com/8street/Cataclysm-DDA
8street Dec 27, 2019
5c83410
Merge pull request #3 from CleverRaven/master
8street Dec 30, 2019
5830f7d
Merge remote-tracking branch 'upstream/master'
8street Dec 31, 2019
35e8708
Merge pull request #15 from CleverRaven/master
8street Jan 26, 2020
790e2e1
Add translations tags
8street Jan 26, 2020
1397119
Another fix
8street Jan 26, 2020
5fdc6ac
Update src/monstergenerator.cpp
8street Jan 26, 2020
5873a28
Change footsteps to transtalion type
8street Jan 26, 2020
a597cb1
Resolve conflict
8street Jan 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/monstergenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,7 @@ void MonsterGenerator::load_species( const JsonObject &jo, const std::string &sr
void species_type::load( const JsonObject &jo, const std::string & )
{
optional( jo, was_loaded, "description", description );
optional( jo, was_loaded, "footsteps", footsteps, "footsteps." );
optional( jo, was_loaded, "footsteps", footsteps, _( "footsteps." ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
optional( jo, was_loaded, "footsteps", footsteps, _( "footsteps." ) );
optional( jo, was_loaded, "footsteps", footsteps, translate_marker( "footsteps." ) );

So it doesn't translate the string twice at line 892.

Also, as the code currently stands, the translation won't update when switching the language in game. Could you change the type of footsteps to translation and update the code accordingly to fix it and modernize the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review.

Could you change the type of footsteps to translation

Could you provide a simple example? footsteps is defined in the following structure:

struct species_type {
    species_id id;
    bool was_loaded = false;
    translation description;
    std::string footsteps;
    enum_bitset<m_flag> flags;
    enum_bitset<mon_trigger> anger;
    enum_bitset<mon_trigger> fear;
    enum_bitset<mon_trigger> placate;
    std::string get_footsteps() const {
        return footsteps;
    }

    species_type(): id( species_id::NULL_ID() ) {

    }

    void load( const JsonObject &jo, const std::string &src );
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change std::string footsteps; to translation footsteps;, and update the code where it is used. Use to_translation( "footsteps." ) to create the default value, and footstep.translated() to get the translated string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the type of variable. I'm not sure that everything is correct, please review it. In the game, this solution works, I tested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as the code currently stands, the translation won't update when switching the language in game.

There were some performance considerations to do so when it was jsonized (see #31493)

Copy link
Contributor Author

@8street 8street Jan 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZhilkinSerg Should I return the type of the variable as it was before? Or leave it?

Copy link
Contributor

@Qrox Qrox Jan 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can investigate the performance problem anytime, but for now we support switching language without restart, so this ensures consistency. Also I don't think this particular string is used that frequently.

Anyway, here are two (immature) ideas of mine about performance optimization of the translation class: we can create a new class cached_translation, which stores an extra cached translated string and the language of the cached string (perhaps through an id or a reference), and in the next translation call, we check if the language has changed and update the cache accordingly. This of course requires a few more bytes per translation object for the cache.

Or we can add raw strings and their translation to a central repository when creating translation objects, which then point to the translated strings through a reference. When switching the language, we iterate through the central repo and update the translated string with gettext calls, so the translation objects automatically point to the new translated strings. I think this one is superior since it not only addresses the performance issue, but also reduces the memory usage of the translation class.

footsteps = _( footsteps );
const auto flag_reader = enum_flags_reader<m_flag> { "monster flag" };
optional( jo, was_loaded, "flags", flags, flag_reader );
Expand Down
2 changes: 1 addition & 1 deletion src/mtype.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,5 +234,5 @@ std::string mtype::get_footsteps() const
for( const species_id &s : species ) {
return s.obj().get_footsteps();
}
return "footsteps.";
return _( "footsteps." );
}