This commit stylizes the language list by replacing applicable first characters in language names with uppercase symbols, and manually fixes some languages that are otherwise not displayed (on windows and ubuntu for now). Example given on the picture is Esperanto.
Manual fixes only occur when the initial string is empty.
If there are any other languages not displayed correctly on mac systems, let me know.
Before and after (Ubuntu 16, with stylizing):
104 | - if(langStr.contains("_")) 105 | + if(locale.nativeLanguageName().length()==0) /* If locale name is an empty string, fix it manually. */ 106 | { 107 | - /** display language strings as "native language - native country (locale name)", e.g. "Deutsch - Deutschland (de)" */ 108 | - ui->lang->addItem(locale.nativeLanguageName() + QString(" - ") + locale.nativeCountryName() + QString(" (") + langStr + QString(")"), QVariant(langStr)); 109 | + if(langStr.contains("szl"))
Take a look at the coding guidelines please
Where could I find that?
@GChuf: I would guess @emilengler is referring to https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md
@jonatack thank you. I did a quick search but was not able to find where the guidelines are. @emilengler I've since updated the file.
Concept NACK. In some languages it might be grammatically incorrect to write it uppercase. Also in languages like "Deutsch" or "espanol de Espana" it is already grammatically correct
Yes, uppercase might be gramatically incorrect in some cases. (It could be argued however that these names are not in the middle of a sentence, but rather at the start - which would justify the use of capital letters ...)
Anyway, gramatically correct or not, the updated list seems much clearer / easier to glance through to me. Looks nicer as well. That's what I was trying to improve. I don't think grammar plays a big role in this list, even though I like to see correct grammar being used everywhere.
173 | @@ -148,7 +174,7 @@ void OptionsDialog::setModel(OptionsModel *_model) 174 | { 175 | this->model = _model; 176 | 177 | - if(_model) 178 | + if (_model)
Please don't include unrelated styling changes (even if they may be correct) in commits.
ok - I thought that was an appropriate thing to do after emil mentioned coding guidelines. is there another way/place to do this then?
Not unless you happen to be changing the same line while making another significant change. We don't want style change/formatting only PRs.
Concept NACK - Can this be fixed upstream or in the translation files somehow, rather than adding language specific workarounds to the GUI code? This seems brittle; i.e is it possible that another language string in the future contains pam or szl?. Will defer to @laanwj.
Anyway, gramatically correct or not,
I think a drop-down list of language names is probably one of the places you actually would want to be grammatically correct. Otherwise I'd assume after the next release we'll have someone opening a PR to "fix" the incorrect display of their language.
Can this be fixed upstream or in the translation files somehow, rather than adding language specific workarounds to the GUI code?
No, the locale names are pulled from the host OS I think. In other words, linux and windows don't know what language "szl" is.
is it possible that another language string in the future contains
pamorszl?
Very unlikely, there are no other languages on transifex that would contain these strings. I was thinking about whether or not to include and langStr.length()==3 for pam and szl as well - I suppose I should have anyway.
I think a drop-down list of language names is probably one of the places you actually would want to be grammatically correct. Otherwise I'd assume after the next release we'll have someone opening a PR to "fix" the incorrect display of their language.
"Incorrect display of their language" seems like an overkill to me - like I mentioned, it could be argued that the display is indeed gramatically correct. My goal was to stylize the list. If the language names were in the middle of a sentence, there would be no dispute over grammar - some languages must stay lowercase. But in a list? I myself can search through that list faster if all first symbols appear the same - not some uppercase, some lowercase.
Seems like Windows (also Microsoft Word) would agree with my reasoning - lists use uppercase.
@GChuf here is a document I've been compiling for the past six months that you might find useful (apologies if not). Also, if you really want to open PRs, one possible humble suggestion (that I myself should heed more often) is to watch for "would-be-nice-to-have" requests by long-term contributors (that you will see when reviewing PRs and observing on IRC) and to look at the issues tagged as "good first issues".
104 | - if(langStr.contains("_")) 105 | + if (locale.nativeLanguageName().length()==0) /* If locale name is an empty string, fix it manually. */ 106 | { 107 | - /** display language strings as "native language - native country (locale name)", e.g. "Deutsch - Deutschland (de)" */ 108 | - ui->lang->addItem(locale.nativeLanguageName() + QString(" - ") + locale.nativeCountryName() + QString(" (") + langStr + QString(")"), QVariant(langStr)); 109 | + if (langStr.contains("szl") and langStr.length()==3)
Use && instead of and -- applies throughout this PR :-)
fixed!
Thanks for trying to improve things there ! ACK on adding languages that are completely missing. That fixes an actual issue. NACK on "stylizing" this list if it adds all kind of special-case custom code.
In some languages it might be grammatically incorrect to write it uppercase.
Yes, the assumption that names are capitalized by the first letter is very much a western one. Or maybe even English. I often see my name capitalized as "Van der Laan", for example.
@jonatack thank you for sharing the doc. Unfortunately most of requests and other PRs exceed my knowledge, as some already know I'm sure.
No worries, same for me, when it comes to Bitcoin we all start from somewhere! There is a self-study club at https://bitcoin-core-review-club.github.io, don't hesitate to prepare and participate :)
@laanwj I see your point. Given that there's not much support for capitalizing anyway, I removed that code and only left "manual fixes". If there are any other languages not being displayed correctly (I can't test for macOS), let me know and I'll include them.
Manually fixes some language names which were not displayed correctly
NACK, this seems like a hack for upstream/OS issues. Presumably whoever uses these languages will have an OS that can handle them anyway?
NACK, this seems like a hack for upstream/OS issues. Presumably whoever uses these languages will have an OS that can handle them anyway?
You might be right, but I'm not sure - I do have the option to use silesian/esperanto... languages on my Windows 10 machine for example, but they're still not displayed correctly in bitcoin-qt. This is an upstream hack and it's not elegant, but it does fix the display issue and it might actually help someone who wants to use silesian.
Could you create or link to an upstream bug, to make sure that upstream is aware of this and maybe see their opinion on this issue?
@GChuf Is there no possible other way? I dont't think that hard coded stuff is the right way to fix.
Maybe we could get a list of the languages by the os and then have a list with the languages bitcoin-qt supports. It will then drop out all elements which are not supported by bitcoin-qt
@emilengler there are other ways, this was the only way I could make it work though. If someone finds a better solution, by all means, create a PR.
p.s. check this link about qt and locales.
97 | @@ -98,15 +98,39 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) : 98 | { 99 | QLocale locale(langStr); 100 | 101 | - /** check if the locale name consists of 2 parts (language_country) */ 102 | - if(langStr.contains("_")) 103 | + if (locale.nativeLanguageName().length()==0) /* If locale name is an empty string, fix it manually. */ 104 | { 105 | - /** display language strings as "native language - native country (locale name)", e.g. "Deutsch - Deutschland (de)" */ 106 | + if (langStr.contains("szl") && langStr.length()==3)
instead of this long if() statement and repeated code, please make this data-driven, e.g. add a structure
/** Additional language names that aren't covered by Qt locale as of [version] */
static const struct {
const char *abbrev;
const char *name;
} language_names[] = {
{"szl", "Silesian"},
{"pam", "Kapampangan"},
{"la", "Latin"},
{"eo", "esperanto"},
};
or more C++11ish
static const std::map<std::string, std::string> language_names = {
{"szl", "Silesian"},
{"pam", "Kapampangan"},
{"la", "Latin"},
{"eo", "esperanto"},
};
97 | @@ -98,15 +98,39 @@ OptionsDialog::OptionsDialog(QWidget *parent, bool enableWallet) : 98 | { 99 | QLocale locale(langStr); 100 | 101 | - /** check if the locale name consists of 2 parts (language_country) */ 102 | - if(langStr.contains("_")) 103 | + if (locale.nativeLanguageName().length()==0) /* If locale name is an empty string, fix it manually. */
Nit: use isEmpty
if (locale.nativeLanguageName().isEmpty())
I'm closing this; I don't think this is making any progress, and the amount of controversy it's created is not worth the scope of the change. Feel free to poke me if you want to re-start work on this, but I'm not sure this particular rabbit hole is worth getting into too much given the scope of this project. Might want to pursue an upstream change instead.