Auto translate and new features

enagora commented 5 years ago

Hi, this is my first pull request but i think it is very usefull. First of all, thanks, this tool is great and saved a lot of time to me. I have cretaed a new branch called "auto-translate-and new-features" the most important thing is that now under the menu Tools is a new feature Translate empty values. This gets field translations from google and fill resx with different color for a later revision.

almost there are a few usability adds.

Unlocks resx files when Visual Studio is opened
Marks with LightYellow warning cells, like control.Name style names.
Added Tools > Translate to traslate with google from Spanish to selected languages
Marks with green forecolor translated field for possible revision
Use key combination Control + Shift +F2 to alternate betwenn upper, lower and first capital letter
Timer for continue retrying traslations every 5 minutes when google blocks.
Added short description to readme.md
Added stop "button" to toolstrip
Now Ctrl + S saves current file and Ctrl + Shift + S saves all files

I hope these changes were interesting for your proyect and apply them in master branch.

See you!

Klocman commented 5 years ago

Sounds good, thank you! I'll review the changes when I get some free time later.

enagora commented 5 years ago

Thanks!

I will response as soo as I can. I hope during this week.

Best regards

On Sun, 9 Jun 2019 at 13:28, Marcin Szeniak notifications@github.com wrote:

@Klocman requested changes on this pull request.

Sorry for the delay! I've read through all of the changes and while it's mostly good, I've found some issues. The biggest issue is that the google translate code is hard-coded to use the es language, which will not be very useful for majority of the users. I added more details in the comments.

Again, thank you for the PR! I hope you can fix these issues so I can merge it, because it would be very useful to have.

In src/Controls/ResourceGrid.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834059:

@@ -108,9 +110,10 @@ private void ApplyConditionalFormatting(DataGridViewRow r) r.DefaultCellStyle.ForeColor = dataGridView1.DefaultCellStyle.ForeColor; }

if (r == dataGridView1.Rows[RowCount - 1])

if (r == dataGridView1.Rows[RowCount - 1] && string.IsNullOrEmpty(r.Cells[colNameKey].Value?.ToString()))

Why is the new check necessary? RowCount - 1 are the comments and they shouldn't be highlighted.

In src/Controls/ResourceGrid.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834101:

@@ -119,12 +122,25 @@ private void ApplyConditionalFormatting(DataGridViewRow r) { ApplyConditionalCellFormatting(r.Cells[lng.LanguageId], SearchParams.TargetType.Text); } + +

if (!string.IsNullOrEmpty(r.Cells[colNameKey].Value?.ToString()) && r.Cells[colNameKey].Value.ToString().ToLower().Contains(".name"))

Where does the .name come from and what does it mean? It should be explained in the readme at least.

In src/ResourceOperations/ResourceHolder.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834155:

@@ -144,6 +144,7 @@ private void UpdateFile(string filename, string valueColumnId, bool skipNontrans { originalMetadatas.Add((string)metadataEnumerator.Key, metadataEnumerator.Value); }

reader.Close();

reader.Close(); is not necessary, it's already called by using (var reader...) when exiting the scope.

In src/ResourceOperations/ResourceHolder.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834169:

@@ -159,6 +160,7 @@ private void UpdateFile(string filename, string valueColumnId, bool skipNontrans if (!originalMetadatas.ContainsKey(key)) originalResources.Add(key, (ResXDataNode)dataEnumerator.Value); }

reader.Close();

Same as above, not necessary.

In src/ResourceOperations/ResourceHolder.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834195:

@@ -206,8 +208,32 @@ private void UpdateFile(string filename, string valueColumnId, bool skipNontrans }
         // Write the cached resources to the drive
using (var writer = new ResXResourceWriter(filename))

using (var writer = new ResXResourceWriter(filename))//$"{filename.Replace(".resx","")}_.resx"))

Make sure to remove commented out code or unnecessary comments before submitting a PR, please! :)

In src/ResourceOperations/ResourceHolder.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834267:
         {
//Retiramos el atributo de solo lectura, si existe

FileAttributes attrs = File.GetAttributes(filename);

if (attrs.HasFlag(FileAttributes.ReadOnly))

File.SetAttributes(filename, attrs & ~FileAttributes.ReadOnly & ~FileAttributes.Hidden);
Move the attribute change code after the file is finished writing and the file handle is closed, so after the using scope ends. Doing this while the file is opened in the writer might fail or work in an unexpected way.

In src/ResourceOperations/ResourceHolder.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834310:

@@ -228,6 +254,12 @@ private void UpdateFile(string filename, string valueColumnId, bool skipNontrans } }

+

private static FileAttributes RemoveAttribute(FileAttributes attributes, FileAttributes attributesToRemove)

Can be removed

In src/ResourceOperations/ResourceHolder.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834320:

@@ -351,7 +383,7 @@ public void Save() /// private static bool IsLocalizableString(string key, ResXDataNode dataNode) {

if (key.StartsWith(">>") || (key.StartsWith("$") && key != "$this.Text"))

if ((key.StartsWith(">>") && !key.ToLower().Contains(".caption")) || ((key.StartsWith("$") && key != "$this.Text")))

Where does the .caption come from and what does it mean?

In src/Windows/MainWindow.cs https://github.com/HakanL/resxtranslator/pull/37#discussion_r291834503:

@@ -626,5 +630,205 @@ private void exportAllResourcesToolStripMenuItem_Click(object sender, EventArgs } } } + +

///

/// Fill resx with translations from google

///

///

///

private void translateToolStripMenuItem_Click(object sender, EventArgs e)

{

LaunchAutoTranslate("es");

Hard-coded languages have limited usability for other people. It would be better to show a window with selection of column to be translated, and optionally the language of the column (so for example you can translate the fr column into other languages, even if it holds something other than French text. Or, more importantly, specify language for the NoLanguageCode column).

It would also be ideal to have a list of columns to google translate, so for example you can only translate fr column into the de column.

The google translate code should be extracted into a separate class, away from MainWindow as much as possible, so that in the future it is easier to add other translators.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HakanL/resxtranslator/pull/37?email_source=notifications&email_token=AFQEK7KFPE42EHAJE4MYFMLPZTSNJA5CNFSM4HOZ67O2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB27P2JY#pullrequestreview-247397671, or mute the thread https://github.com/notifications/unsubscribe-auth/AFQEK7IDW7PLVRBHU2WUBPDPZTSNJANCNFSM4HOZ67OQ .

--

[image: enagora] http://www.enagora.com/

Uraitz Olaizola Velasco Email: uolaizola@enagora.com Telf.: +34 658 988965 Web: http://www.enagora.com Dir.: Av Iparralde 43, Ficoba - Oficina 2, Irun (Gipuzkoa) - 20302 ¿En Ficoba, Donde? http://www.youtube.com/watch?v=wKCGxx3HukU

enagora commented 5 years ago

As I said when i start with this pull request, this is my first pull, I hope that now is all working and these changes done. Please notify me whatever is needed.

Thanks, i hope my changes will be welcome.

enagora commented 5 years ago

Hi! I have corrected the request but i don't know if you have received any notification. Thanks for you time and your project!

Klocman commented 5 years ago

Hello!

Sorry for the delayed reply, it was a mix of being busy and not getting the notifications. Changes look good, thank you for the great work! I only have two suggestions left before I merge it:

I suggest moving the remaining translation logic (WebBrowser, Timer) out of MainWindow and into a separate class. The class would ideally expose a single method to translate strings (first time it is called would initialize the translator). This is mainly to avoid too much unrelated code from cluttering the MainWindow class, and to make adding new translation sources easier in the future. To update status this translation method could have an Action as an argument, and call this action whenever it wanted to update status. Don't worry if you have trouble understanding what I want to do, I can refactor this myself later so this is optional.

It looks like this commit was a mistake https://github.com/HakanL/resxtranslator/pull/37/commits/d7eb05b3ea8cc525a40692286746db7296a30252, it rolled back some of the new commits made on the main repository. It looks like the commits affected are https://github.com/HakanL/resxtranslator/commit/87776d8dc00a32e2295a96a7d67fad636e2d0251 and newer. Can you undo the reverts, or restore the missing changes? Because of how you updated your branch, changes from these commits would get lost when this PR got merged.

Looking forward to your reply, this time I'll respond timely :)

enagora commented 5 years ago

Hi, I'll do these changes but first. Have you tried to load a resx with your main branch? When I ry to load for example, project's Localization.resx. Application crashes with a System.ArgumentException. Is for that I reversed these changes when I merged my branch.

Can you verify than your branch is working properly?

Thanks!

ManlyMarco commented 5 years ago

Yes, it's working correctly. I'm using the latest buld from master regularly. Please add an issue with the details of the exception.

Klocman commented 5 years ago

Hello!

Same here, I tested the master on all of my projects and everything seemed to work fine. It's probably caused by something unusual in your project, what's the stack trace of the exception?

enagora commented 5 years ago

I get this error:

at System.Data.DataRow.GetDataColumn(String columnName) at System.Data.DataRow.get_Item(String columnName) at ResxTranslator.ResourceOperations.ResourceHolder.RowContainsTranslation(DataRow row, String languageId) in C:\Github\HakanL\src\ResourceOperations\ResourceHolder.cs:line 415 at ResxTranslator.ResourceOperations.ResourceHolder.EvaluateRow(DataRow row, String[] languagesToCheck) in C:\Github\HakanL\src\ResourceOperations\ResourceHolder.cs:line 387 at ResxTranslator.ResourceOperations.ResourceHolder.EvaluateAllRows(String[] languagesToCheck) in C:\Github\HakanL\src\ResourceOperations\ResourceHolder.cs:line 618 at ResxTranslator.Windows.MainWindow.<>c__DisplayClass13_0.<set_CurrentResource>b__0(MainWindow _) in C:\Github\HakanL\src\Windows\MainWindow.cs:line 133 at ResxTranslator.Tools.Extensions.InvokeIfRequired[T](T c, Action1 action) in C:\Github\HakanL\src\Tools\Extensions.cs:line 17 at ResxTranslator.Windows.MainWindow.set_CurrentResource(ResourceHolder value) in C:\Github\HakanL\src\Windows\MainWindow.cs:line 118 at ResxTranslator.Windows.MainWindow.<.ctor>b__4_0(Object sender, ResourceOpenedEventArgs args) in C:\Github\HakanL\src\Windows\MainWindow.cs:line 40 at ResxTranslator.Controls.ResourceTreeView.OnResourceOpened(ResourceOpenedEventArgs e) in C:\Github\HakanL\src\Controls\ResourceTreeView.cs:line 62 at ResxTranslator.Controls.ResourceTreeView.SelectResourceFromTree() in C:\Github\HakanL\src\Controls\ResourceTreeView.cs:line 152 at ResxTranslator.Controls.ResourceTreeView.treeViewResx_AfterSelect(Object sender, TreeViewEventArgs e) in C:\Github\HakanL\src\Controls\ResourceTreeView.cs:line 169 at System.Windows.Forms.TreeView.OnAfterSelect(TreeViewEventArgs e) at System.Windows.Forms.TreeView.TvnSelected(NMTREEVIEW* nmtv) at System.Windows.Forms.TreeView.WmNotify(Message& m) at System.Windows.Forms.TreeView.WndProc(Message& m) at System.Windows.Forms.NativeWindow.DebuggableCallback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam) at System.Windows.Forms.UnsafeNativeMethods.SendMessage(HandleRef hWnd, Int32 msg, IntPtr wParam, IntPtr lParam) at System.Windows.Forms.Control.SendMessage(Int32 msg, IntPtr wparam, IntPtr lparam) at System.Windows.Forms.Control.ReflectMessageInternal(IntPtr hWnd, Message& m) at System.Windows.Forms.Control.WmNotify(Message& m) at System.Windows.Forms.Control.WndProc(Message& m) at System.Windows.Forms.UserControl.WndProc(Message& m) at System.Windows.Forms.NativeWindow.DebuggableCallback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam) at System.Windows.Forms.UnsafeNativeMethods.CallWindowProc(IntPtr wndProc, IntPtr hWnd, Int32 msg, IntPtr wParam, IntPtr lParam) at System.Windows.Forms.NativeWindow.DefWndProc(Message& m) at System.Windows.Forms.TreeView.WmMouseDown(Message& m, MouseButtons button, Int32 clicks) at System.Windows.Forms.TreeView.WndProc(Message& m) at System.Windows.Forms.NativeWindow.DebuggableCallback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam) at System.Windows.Forms.UnsafeNativeMethods.DispatchMessageW(MSG& msg) at System.Windows.Forms.Application.ComponentManager.System.Windows.Forms.UnsafeNativeMethods.IMsoComponentManager.FPushMessageLoop(IntPtr dwComponentID, Int32 reason, Int32 pvLoopData) at System.Windows.Forms.Application.ThreadContext.RunMessageLoopInner(Int32 reason, ApplicationContext context) at System.Windows.Forms.Application.ThreadContext.RunMessageLoop(Int32 reason, ApplicationContext context) at ResxTranslator.Program.Main() in C:\Github\HakanL\src\Program.cs:line 17`

Klocman commented 5 years ago

What's the exception type and/or message? I agree that it should be a separate issue.

HakanL / resxtranslator

Auto translate and new features #37

Again, thank you for the PR! I hope you can fix these issues so I can merge it, because it would be very useful to have.

Why is the new check necessary? RowCount - 1 are the comments and they shouldn't be highlighted.

Where does the .name come from and what does it mean? It should be explained in the readme at least.

reader.Close(); is not necessary, it's already called by using (var reader...) when exiting the scope.

Same as above, not necessary.

Make sure to remove commented out code or unnecessary comments before submitting a PR, please! :)

Move the attribute change code after the file is finished writing and the file handle is closed, so after the using scope ends. Doing this while the file is opened in the writer might fail or work in an unexpected way.

Can be removed

Where does the .caption come from and what does it mean?