First step to a localized client

[anderssonfilip]

hey, awesome piece of software you've written. I've made some changes to cater for a localized version, all strings are externalized and there is crowd sourced effort on the way to get other languages added.

Would really like to see this in the base version at some time so please check out my changes!

[ripper234]

@zathras-crypto I think it's worth taking a look at this pull request.

[dexX7]

@zathras-crypto please take another look at this. To my understanding this needs only be implemented once without huge maintenance afterwards. If you need to connect with tradespoke, you may also use trade-spoke at yandex dot com.

It looks like Masterchest is already translated in almost 4-5 languages:
https://www.transifex.com/projects/p/masterchest-wallet-1/

[zathras-crypto]

Can you please rebase this pull on the latest master branch? I'll take another look.

Thanks

[anderssonfilip]

I merged with build 21, can you please take a look.

Don't worry, I'll take care of the last details once we have this on the main repository

[dexX7]

What's the easiest way to test this? Replacing the files does not yield an executable project.

I'm wondering how you handle the decimal delimiter and I'm furthermore curious about the fallback to a Swedish culture setting a few times? What's the purpose of this?

It would be neat, if you could use proper and consistent naming conventions, e.g. a prefix to indicate the type of an object (textbox, label) and a descriptive name. This is more or less done in most cases, but using camel case may help to increase readability. All translation related resources should be prefixed, too, to indicate it's purpose. It would also be great, if the English default text is included as comment. Please don't get me wrong, this is not a request, but only a suggestion for the future. I'm impressed by the amount of changes and think this is an awesome addition. :)

[anderssonfilip]

Hi dex,

I can send you a compiled binary if you want to test this?

The fallback culture is English-US.

The swedish culture is a trick to refresh the datagrids with the correct
decimal delimiter. US culture uses dot, Swedish uses comma so by swapping
before the culture change and then setting the correct culture refreshes
the datagrids with the correct delimiter. I think that's why I put it there
at least, but I can look again.

I agree that the naming convention could be improved.

The English default text is visible on mouse hover in Visual Studio, we are
not using a text editor to to edit the code are we?

On Sun, Apr 6, 2014 at 3:55 AM, dexX7 [email protected] wrote:

What's the easiest way to test this? Replacing the files does not yield an
executable project.

I'm wondering how you handle the decimal delimiter and I'm furthermore
curious about the fallback to a Swedish culture setting a few times? What's
the purpose of this?

It would be neat, if you could use proper and consistent naming
conventions, e.g. a prefix to indicate the type of an object (textbox,
label) and a descriptive name. This is more or less done in most cases, but
using camel case may help to increase readability. All translation related
resources should be prefixed, too, to indicate it's purpose. It would also
be great, if the English default text is included as comment. Please don't
get me wrong, this is not a request, but only a suggestion for the future.
I'm impressed by the amount of changes and think this is an awesome
addition. :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/6#issuecomment-39664511
.

[dexX7]

Haha no, I'm using VS, too, but I'm unable to use the files, because I don't know how to import or replace the resources in the existing project. The path "Project - Add - Add Existing Item" + file replacing is not sufficient, because the resources are not recognized per se. A hint how to do so or a copy of your project would be very appreciated. (I'm also available via email: dexx at bitwatch co)

[anderssonfilip]

OK,

First you need to click "Show all files" in the VS solution explorer, then
add the resource files as Embedded Resources. If after building there are
folders in the output directory, fr-FR, sv-SE etc with a resource dll
inside you are all set.

You will also need to add the settings file as a embedded resource and
reference System.Configuration

Good luck!

On Sun, Apr 6, 2014 at 11:37 AM, dexX7 [email protected] wrote:

Haha no, I'm using VS, too, but I'm unable to use the files, because I
don't know how to import or replace the resources in the existing project.
The path "Project - Add - Add Existing Item" + file replacing is not
sufficient, because the resources are not recognized per se. A hint how to
do so or a copy of your project would be very appreciated. (I'm also
available via email: dexx at bitwatch co)

Reply to this email directly or view it on GitHubhttps://github.com//pull/6#issuecomment-39676247
.

[dexX7]

Thanks, I was able to compile your fork. There are quite a few problems and I don't suggest to merge at this point.

Here is a list of my notes:

• A point delimiter must be used all the time for all underlying operations, even if the display language uses a comma. Some of the resulting issues are documented in the Masterchest thread, but one example: when trying to send some coins the following check is performed in L2462:

If Val(txtsendamount.Text) = 0 Then
    'nothing to send
    Exit Sub
End If

In the case of a comma delimiter the value of txtsendamount.Text is "0,025" for example. Val("0,025") == 0.

• Another example: start the wallet and switch to a non-point-delimiter language at the very beginning. Watch the transaction processing fail:

[2014-04-07T04:23:21] BLOCKSCAN: Found pending MSC transaction (simple send): 15accb60cf20256658405dbc30efd94e783267328ed06cb199d06f4f4138bf0f
[2014-04-07T04:23:22] BLOCKSCAN: Démarrage de la résolution des transactions...
ERREUR : Le scan du blockchain a produit une "exception" : (...) [ Column name count = 5,Source expression count = 6 ]
[2014-04-07T04:23:23] DEBUG: Thread exited with error condition.

• Due to the delimiter issue the chart is sometimes broken.
• Several labels are missing.
• Changing the language may result in wrong labels or a wrong state. For example switching the language always resets the "synchronization" field to "not synchronized", even in the case of ongoing synchronization or after a successful synchronization.
• Labels overlap in some cases, depending on the language.
• The language combo box has the effect of making the form non-moveable by click-and-move in the upper area.
• The usage of point and comma delimiter is inconsistent.

A visual comparison (Mod: France - Mod: English - Original):
http://i.imgur.com/EzCgIvs.png

[anderssonfilip]

Thank you,

Some of the things I was already aware of, especially the missing and overlapping labels. I'll work through the list and ask zathras if there is anything unclear in the original code. Still think I can salvage this;)

[anderssonfilip]

Fixed a few of them:

1. Fixed: instead of using Val which is culture independent the amount is parsed as
   Double.TryParse(txtsendamount.Text, NumberStyles.AllowDecimalPoint, activeCulture, amount)
2. Fixed: Error due to writing number with comma to database. Changed variable unitspurchased,paymentpaidlong to type Long and casted to string as ToString(“D”). Other variables casted( tmpunitprice, tmpofferamount, saleamount) as .ToString(defaultCulture)

Bullet 5. Suggested fix: Updating culture during sync or processing has no effect. Labels are updated only when synced

Bullet 7. Fixed by reattaching Rectangle1 to Form1_MouseDown

[anderssonfilip]

@zathras-crypto, please let me if you can take a look at this merge.

The inconsistency is unsolved, both for the localization and the controls. There are lots of controls called, Label61, Label75 etc so to come up with a consistent naming for localization requires more changes than I'm ready to make

Overlapping labels are there but is easy to solve in the UI. Again, I don't want todo any further changes until you looked at it properly.

Some of my criticism (meant as help) on the code after looked at it for a day or two:

• A lot of the methods are several hundred lines long (maybe thousand) and there are no separation between UI, models, database etc.
• String concatenation for database queries is bad practice and vulnerable.
• Forms are reused, i.e. duplicate code
• Lots of string literals, even for important things like database path/name
• Lots of unused variables

I'm sure you were planning to remedy this in the future so let me know if you need any help.

[ripper234]

Sorry it's taken so long to review, I'll ping @zathras-crypto to take a look ... we should be more friendly to people submitting pull requests.

[ripper234]

I added this to the next milestone (due in 9 days).

Please note that version 0.4 of the codebase is coming soon, and has significant changes - @tradespoke it would be great if you could rebase the code off of that.

@zathras-crypto has promised to give this issue his full attention once smart property is done.

Also this would be eligible for Mastercoin Bug Hunt

[anderssonfilip]

@zathras-crypto please let me know what you think.

I wont waste anymore time on this if it doesn't get merged. I've explained that ui controls need to me moved for string expansion but would rather see that you do this, or I'll do it in another PR

Cheers

[zathras-crypto]

Hey there - thanks - I asked DexX to take a look and he advised against
merging in current form (I trust his advice). I'd like to collaborate with
you on localizing the client as soon as we get SP done. Be back to you in
a few days when the 0.4 version of the wallet is out.

Thanks!
Zathras

On Tue, Apr 22, 2014 at 11:01 AM, tradespoke [email protected]:

@zathras-crypto https://github.com/zathras-crypto please let me know
what you think.

I wont waste anymore time on this if it doesn't get merged. I've explained
that ui controls need to me moved for string expansion but would rather see
that you do this, or I'll do it in another PR

Cheers

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6#issuecomment-40993772
.

[dexX7]

I'm very, very sure all the work on the implementation and translation will be used at some point! :)

But as mentioned there are a few issues and it looks like the latest iteration does not solve all of them (e.g. the problem with localization and decimals) so this is not yet ready for the public.

Your criticism and all points listed above are valid and I think we should do a cleanup immediately after the SP release. Once that's done adopting the dynamic localization is probably easier and hopefully we can get rid of the workarounds then too.

[anderssonfilip]

@zathras I have done fixes after dexx pointed out problems in my request. I'd prefer if you merge this time, if I do it it may cause more bugs as was the case last time. I'm very willing to help solving the last bits but it needs to be incremental

[anderssonfilip]

have you had time to look at this yet?

[ripper234]

@tradespoke Masterchest is being deprecated in favor of Master Core.
I would like to appologize for your wasted effort with this pull request.

Masterchest was developed and maintained by @zathras-crypto alone by himself, and this is the reason he didn't get to look at pull requests.

MasterCore already has a core dev team of 2-3 people, and we are looking to beef up the team in the future as well.

[anderssonfilip]

Hi Ron,

Thanks for your message.

I tip to you is not to advertise bounties when you don't have the means to pay out
->https://trello.com/c/qPlB0nxw/51-1000-localization-of-at-least-one-client

It may give a bad reputation and speaking for myself have taken away my interest in supporting Mastercoin. I found out that zathras was the only developer on the project after this pull request and also that you were looking for extra developers. I was interested but waited to see how this bounty were handled.

I'm still accepting a bounty for this "completed" work!

[zathras-crypto]

Hey tradespoke,

Firstly please accept my apologies for not being able to give your pull request proper attention during MasterChest development - as Ron has noted it was just little old me and that bottleneck was far from ideal. We've now ceased development on MasterChest and I am contributing to our reference implementation; Master Core.

With regard to paying bounties - I can assure you the foundation does have the means to pay :) With that said, fiscal responsibility and budget diligence dictate that they only pay out on completed bounties. Please trust me when I say there is plenty of code I wrote that didn't end up getting used, and thus didn't qualify for bounties.

In this case, I believe the bounty was for completed localization of a client - DexX did a good analysis above and I don't believe we could call this complete (you yourself called this "first steps" :) - if I recall the terms of the bounty it had something along the lines of "Localization must be officially adopted").

Anyway, I just wanted to jump in here and make a quick post because I wouldn't like to see you or others discouraged from contributing, it's simply that if they paid for all the code towards incomplete bounties I think the foundation would be broke already haha.

Thanks again for your contribution, and I do hope you will stick around and see what we're doing with our Core platform - again as I note this 'single developer' bottleneck will not be an issue there, and you can do prior engagement before cutting code on exactly how to deliver a bounty (and thus how to ensure you get code merged).

All the best
Zathras

[dexX7]

Actually I feel that I need to add a few notes here: even though the code was not usable as-it-is, this was by no means due to non-willingness to finalize. This is especially relevant, because of my earlier statement which was "I'm very, very sure all the work on the implementation and translation will be used at some point!" as well the assurance to allocate time to evaluate the localization patch after v0.4.

[anderssonfilip]

Thanks guys for taking the time to reply. Obviously, it was "the first step" since I had no possible way to complete this without help from a developer on the inside, nor could I continue to to keep up with the master without introducing new bugs as dexx pointed out.

Anyway, I sincerely appreciate the clear communication and hope my comment above doesn't discourage other people from contributing!!

As usual, I answered too quickly yesterday and wish I'd left out the last sentence. I write code because I believe in open-source but since there are so many projects one need to be very selective where and how to contribute. Good luck guys, I hope I have time to dig into the reference implementation someday to really understand the details of your project.

[zathras-crypto]

@tradespoke thank you for your understanding, I obviously can't make any actual decisions but just wanted to feed back based on my experiences - if you ever want to talk to us about getting involved in the reference please don't hesitate to shout :)
@dexx agreed 100%, if we hadn't decided to deprecate Masterchest with 0.4 I completely agree we would have taken this forward further :)