-
Notifications
You must be signed in to change notification settings - Fork 389
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
Enhancement to add the document language to DOCX based on the report locale (#1620) #1627
Enhancement to add the document language to DOCX based on the report locale (#1620) #1627
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR seems to contain an change in one file (see my comments). Can you explain or remove this deletion block?
Personally I think the default en-US should be changed to if possible, but that's a matter of taste, so since you wrote the code, you may decide yourself.
And one question regarding using reportContext.getLocale()
as the source. It seems to work, but at first glance I couldn't find where the rptdesign's locale property is used to initialize the reportContext's locale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the two dozens of deleted lines here. They seem unrelated to the language change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deleted code is clean-up of old commented code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I'd prefer (only if MS Word accepts it) if it was possible to not set a language, e.g. skip the whole w:lang
element, instead of using the old en-US
default (which is a minor bug IMO).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will not change the structure over all because we can get effects if we remove the "w:lang" over all.
In my understanding this tag is a default tag of the styles of Word. Therefore I will not remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that the locale
property inside the rptdesign is already used to initialize reportContext.getLocale()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "locale"-property is a new one and will be initialized in the "public void initialize()"-method.
This was a local variable before and I use this one for the language. So I avoid special handling if the local is null and use the same behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understood that already. My question is from the perspective that we said we want to use the locale property from the rptdesign file as the source to set the language inside the DOCX file.
And in the PR I see that we use the locale property from reportContext
as the source instead.
So there's kind of a missing link from "rptdesign.report.locale" to "reportContext.locale", which I assume is existing somewhere in the existing source (outside of the PR). but I couldn't find it. I only want to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, well, this is a good question. On of my test cases was to enter a local on report side "test-locale" & "auto"
and I got "test-locale" and "auto" internally which will be switched to "en" because these are no valid locale-values.
I tested also the option of "blank" locale and the result was the fallback "en" again.
My test environment is MS Windows 11 "de". So the system-locale wasn't used on my side.
But I cannot say directly where the transfer will be done on code level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Is your "OK" the approval for that change then I would merge the change. |
Yes, pls merge. |
Enhancement to add the document language to DOCX based on the report locale (#1620)