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

locales init #563

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nadenislamarre
Copy link

Hi,

i'm from the batocera.linux project.
we have migrated to es-retropie for our system, because it is really the best es fork.
by doing this, we would like to become retropie-es contributors.
i've done a first pr here to include gettext into retropie-es.
it is minimal, but the aim is first to validate the process, the rules, and then, i'll add all the strings to translate.

Signed-off-by: Nicolas Adenis-Lamarre nicolas.adenis.lamarre@gmail.com

@tomaz82
Copy link
Collaborator

tomaz82 commented May 13, 2019

2 issues with this PR is the 2 spaces instead of tabs and that this will not compile on windows

@nadenislamarre
Copy link
Author

  • do you have the indentation rules to follow please ? (ideally, the emacs indentation method to use)
  • why do you think it will not compile on windows ? ("/usr/share/locale" will not work for sure but not avoid compilation). This is (approximately) the same functions i used on the xmoto game and it compiled it successfully via mingw with cross compilation (from linux for windows).
    i've no way to compile/work on windows. Should i set #define to avoid gettext on windows ?

@cmitu
Copy link

cmitu commented May 13, 2019

@nadenislamarre There is an error because #libintl.h is not found out-of-the-box on Windows (included in localeES.h). It probably needs gettext installed and configured through CMake.
Tried with Visual Studio 2017 (Community).

EDIT: adding the proper includes/libs makes the compilation succeed, so it probably just needs an adjustment to the CMakeLists.txt. For starters, IMHO only Linux should be included and leave Windows under a #define.

@Gemba
Copy link

Gemba commented May 19, 2019

  • do you have the indentation rules to follow please ? (ideally, the emacs indentation method to use)

In general not. I raised this question a while ago since I felt it is getting more and more important as contributions are gaining momentum (and for several other obvious reasons).

@pjft
Copy link
Collaborator

pjft commented May 20, 2019

I don't think there's anything set in stone. The default setting, inherited from the original ES code is tabs, each of them 4-spaces wide, I believe.

Signed-off-by: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
@nadenislamarre
Copy link
Author

updated.
tabs fixed.
compilation fixed in case gettext is not found.

Signed-off-by: Nicolas Adenis-Lamarre <nicolas.adenis.lamarre@gmail.com>
Copy link
Collaborator

@jrassa jrassa left a comment

Choose a reason for hiding this comment

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

When running make, locale/emulationstation.pot and any language specific po files are being updated. Ideally this should only happen if there are no new messages.

Also currently as part of that regeneration, the order of the messages is being changed. I was able to prevent this by adding the -j option. This resulted in only the POT-Creation-Date entry being updated. Not sure if that is the best approach. But, as I said above, it would be preferred if these files only updated when there are new messages.

Also, for now can we remove the locale/lang directory? We are discussing how we want to handle accepting translations, but for now we don't want to get a bunch PRs here to add various translations.
The thought is that for now we can at least include support for localization making it easier for anyone downstream to implement.

@@ -196,6 +197,28 @@ bool verifyHomeFolderExists()
return true;
}

void locales_init() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use tabs vs 2 spaces

else
ss << gameCount << " GAMES AVAILABLE";
else {
snprintf(strbuf, 256, ngettext("%i GAME AVAILABLE", "%i GAMES AVAILABLE", gameCount), gameCount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

use tabs vs 2 spaces


#ifndef HAVE_GETTEXT
char* ngettext(char* msgid, char* msgid_plural, unsigned long int n) {
if(n != 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use tabs vs 2 spaces

#define _LOCALE_H_

#ifdef HAVE_GETTEXT
#include <libintl.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

use tabs vs 2 spaces

message (STATUS "Native language support enabled.")
add_definitions(-DHAVE_GETTEXT)
add_subdirectory (locale)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add an else message indicating 'Native language support disabled'?

find_program (MSGFMT_EXECUTABLE msgfmt)
find_program (MSGMERGE_EXECUTABLE msgmerge)
find_program (XGETTEXT_EXECUTABLE xgettext)
if(MSGFMT_EXECUTABLE AND MSGMERGE_EXECUTABLE AND XGETTEXT_EXECUTABLE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add an option to disable even if all programs are available?


#define LOCALEDIR INSTALLATION_PREFIX_DIR "/share/locale"
// local dir ; uncomment if you want to use locally compiled .mo files instead of .mo from the installation dir (present only after make install)
//#define LOCALEDIR "./locale/lang"
Copy link
Collaborator

Choose a reason for hiding this comment

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

RetroPie doesn't rely on make install for installation. Can we make ./locale/lang the enabled option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants