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

Added Italian language support for the LessMSI GUI app #205

Merged
merged 23 commits into from
Oct 5, 2024
Merged

Added Italian language support for the LessMSI GUI app #205

merged 23 commits into from
Oct 5, 2024

Conversation

mega5800
Copy link
Contributor

@mega5800 mega5800 commented Aug 25, 2024

👍🎉 First off, thanks for taking the time to contribute! 🎉👍

Please fill out the following checklist:

If you need any help at all, feel free to submit the PR and @-mention activescott and I'll be happy to assist!

Hello @activescott.

I took care of this ticket and added Italian text support for the LessMSI GUI app.

Here are some screenshots of my work:
image

image

@bovirus, thank you for your help with Italian translations.

Thank you.

@mega5800
Copy link
Contributor Author

@activescott can you please help me with the raised Codacy error:
image

The partial keywork is required here, as this is a Form class:
image

@mega5800
Copy link
Contributor Author

@activescott any updates?

Thank you.

@bovirus
Copy link

bovirus commented Aug 28, 2024

@mega5800

How can help you with the code?

Could be a modifier (use of &) issue?

@mega5800
Copy link
Contributor Author

Thank you for your help suggestion @bovirus.

The issue is with the Codacy test rather than with the code itself.
I am waiting for response from Scott about this issue.

@activescott
Copy link
Owner

@mega5800 I ignored the codacy issue. Can you also try removing "partial" in that one place and see if it compiles. I can't remember if C# allows it to be omitted in that case or not - but that is what Codacy/Sonar# is implying. I don't have my windows dev box handy right now, but I'll investigate this more closely here soon.

@mega5800
Copy link
Contributor Author

@activescott I tried removing the partial keyword but got this error
image

Is there an option of ignoring this Codacy issue while merging this pull request to the main repo?
This pull request only introduces GUI changes, without logic changes, and this is only a minor Codacy issue.

Thank you.

Copy link
Owner

@activescott activescott left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but a couple minor changes need made and a test for CI to make sure that the builds are always building properly translated apps.

A quick though on tests: Maybe after the build is done, some of the CLI could be run to confirm it returned translated text? Just a an idea.

src/LessMsi.Gui/AboutBox.cs Show resolved Hide resolved
src/LessMsi.Gui/MainForm.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/MainForm.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/MainForm.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/MainForm.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/MainFormPresenter.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/MainFormPresenter.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/Resources/Languages/Strings.it.Designer.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/Windows.Forms/SearchPanel.Designer.cs Outdated Show resolved Hide resolved
src/LessMsi.Gui/Resources/Languages/Strings.Designer.cs Outdated Show resolved Hide resolved
@mega5800
Copy link
Contributor Author

@activescott thank you for your comments.
I took care of most of them and left a few comments of my own.
I'd love to get some extra feedback from you.

Thank you.

@mega5800 mega5800 requested a review from activescott August 28, 2024 22:31
@mega5800
Copy link
Contributor Author

mega5800 commented Aug 29, 2024

Hello @activescott.

Following your advice, I added some tests for GUI in different languages (English and Italian).
These tests run successfully on my machine, but once I push them to the app veyor server the Italian test fail.

I believe my locale changing logic isn’t working properly on the app veyor server.
What can we do in such case?

Thank you.

@mega5800
Copy link
Contributor Author

@activescott any updates?

Thank you.

@activescott
Copy link
Owner

I'll look at the test case. I see the code but I'll do some debugging myself too

@mega5800
Copy link
Contributor Author

mega5800 commented Sep 4, 2024

@activescott, as you requested I reverted the changes I made to the AboutBox logic and updated the pull request.

I believe the only thing left is to sort the issue with LessMsi.Tests.GUITests.CheckUIStrings.
@bovirus, I believe that once all the tests are executed successfully, Scott can publish a new version.

If you will find any issues with the translations at that point, I will gladly assist you.

@mega5800 mega5800 requested a review from activescott September 4, 2024 21:49
@mega5800
Copy link
Contributor Author

@activescott any updates?

Thank you.

@mega5800
Copy link
Contributor Author

Hello @activescott.

I'd be grateful if you could check the testing I’ve been having.
This is the last bit in this pull request, and I’d like to start working on other tickets.

Thank you.

@activescott
Copy link
Owner

I am looking. Sorry for the delay

@activescott
Copy link
Owner

If you run it locally using the build.bat, test.bat scripts locally it also fails. Those scripts are really the only reliable way to build and test the project as Visual Studio has various "magic" that isn't always obvious. In this case the magic is that it is running commans to build the resources, and those aren't included in the build and the deployed files. I'll take a stab at adding that and push it.

@mega5800
Copy link
Contributor Author

@activescott, thank you for examining this issue.

I would like to mention that the only test that currently fails is the locale test, where I am changing the locale and testing the texts displayed in different buttons across the GUI app.
As you can see from the screenshot below, I am failing the Italian checks:
image

Perhaps, I can't change the locale on the AppVeyor server.

@bovirus
Copy link

bovirus commented Sep 29, 2024

@mega5800

Italian should be "Seleziona &tutto"

@mega5800
Copy link
Contributor Author

@bovirus
I matched the format in which the original English text is shown, where each word is in capital case.

@bovirus
Copy link

bovirus commented Sep 29, 2024

@mega5800

In Italian the grammtical rule is different.
Only first letter of a paragraph (except own name) could be in uppercase.

And "is "Seleziona tutto" and not "Selziona tutto".

@activescott
Copy link
Owner

I just pushed 62138c1 which fixed it locally when runnig build.bat and test.bat. Lets just double-check it passes in CI...

@activescott
Copy link
Owner

Didn't work. I'll leave some notes here in case I don't figure this out here shortly before I have to take off for a bit:

https://ci.appveyor.com/project/activescott/lessmsi/builds/50698208#L250 shows that it did attempt to generate the it resources sattelite assembly.

https://ci.appveyor.com/project/activescott/lessmsi/builds/50698208#L266 shows it was generated and copied to the LessMsi.Gui\bin\Release\it\lessmsi-gui.resources.dll folder.

VS also copied it into the tests directory: https://ci.appveyor.com/project/activescott/lessmsi/builds/50698208#L311

The msbuild script did NOT copy it to the deploy directory which I would expect to have seen two lines after https://ci.appveyor.com/project/activescott/lessmsi/builds/50698208#L437

🤔

@activescott
Copy link
Owner

Well the good news (?) is that it fails locally now for me :) it succeeded earlier though so apparently I had some msbuild syntax that worked and broke it lol

@activescott
Copy link
Owner

i just tried another simpler approach in msbuild. Hope this works! 🤞

@activescott
Copy link
Owner

activescott commented Sep 29, 2024

looks like the it directory isn't in the zip though. So couple things left here that I know of:

  • ensure the \it*.resources files are included inthe zip (there is only one for now)
  • ensure that the nupkg install also extracts the it subdir and the resources file in it

@mega5800
Copy link
Contributor Author

mega5800 commented Sep 29, 2024

@activescott, thank you for your help.
These points you mentioned, do you want me to take care of them?

If so, I'd be grateful if you could provide some information, since I am not familiar with their content.
BTW, I noticed that we are able to pass all the tests successfully, including the locale test, so you definitely fixed the testing issue I had, thank you.

@bovirus, I believe we are getting closer to releasing a version with the Italian captions.
I'd like to remind that all the Italian captions were provided by you in the ticket thread, and I will be glad to help you amending any Italian captions once this version is released.

@activescott
Copy link
Owner

This is good for me now. Here is how you can test this for future reference:

CI always publishes artifacts for every build. So for the most recent build on this PR at the time I'm writing this comment they are at https://ci.appveyor.com/project/activescott/lessmsi/builds/50698795/artifacts

  • You can download the zip file there and check inside of it and see the file \it\lessmsi-gui.resources.dll.
  • You can download the nuspec file there (or build it locally) and then install it with choco locally with the commandchoco install lessmsi --version="0.0.0" --source="C:\src\lessmsi\src\.deploy\chocolateypackage" (in this case I used version 0.0.0 build locally). Then it will print out where it installs it and in that directory I can see the it subdirectory with the expected resources file there.

@activescott
Copy link
Owner

@mega5800 Please confirm all tasks are complete here and that you want to merge it and I'll merge it.

@mega5800
Copy link
Contributor Author

Hello @activescott.
I believe we can finally merge this pull request.
All the changes you've made yesterday are clear to me.

Thank you for your help.

@mega5800
Copy link
Contributor Author

mega5800 commented Oct 1, 2024

@activescott, can you please merge the pull request?
Or do you want me to do anything else?

Thank you.

@activescott activescott merged commit 18356d0 into activescott:master Oct 5, 2024
2 checks passed
@activescott
Copy link
Owner

Merged. Sorry for the delay. I was/am traveling for work.

@activescott
Copy link
Owner

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants