fix for 9699. Added bimap for language-lcid mapping #9729

yenniejunvu · 2023-04-02T20:40:17Z

Added a private static bimap for supporting more lcids/languages when exporting to MS Office 2007.
Added simple unit tests, but not sure if those are necessary.

Tasks

Give feedback

indented tasks (i.e. nested tasklist; support for this is coming!)
an empty task (i.e. - [ ] on a line by itself)
any empty new lines (before/after the tasklist or in between tasks)
duplicate issue/pr links
a draft task exceeds 512 characters

Please check the tasklists documentation for more information. 🔗 Feedback Discussion

Thank you for participating in the Private Beta ❤️

Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Screenshots added in PR description (for UI changes)
Options

koppor

Good initial shot.

MSBibConverter.java also should be adapted.

Please add test for MsBibConverter using Ctrl+Shift+T

Press then Enter

Then create a test case with a BibEntry.

See IEEETest.java for a more complete example.

koppor · 2023-04-03T14:39:30Z

src/main/java/org/jabref/logic/msbib/MSBibMapping.java

@@ -24,6 +24,45 @@ public class MSBibMapping {

    private static final HashBiMap<Field, String> BIBLATEX_TO_MS_BIB = HashBiMap.create();

+    private static final HashBiMap<String, Integer> LANG_TO_LCID = HashBiMap.create();


Do you really need HashBiMap here? Isn't BiMap enough?

See https://github.com/HugoMatilla/Effective-JAVA-Summary#52-refer-to-objects-by-their-interface for a longer explanation

Doesnt the same logic apply for BIBLATEX_TO_MS_BIB as well?

I think so.

koppor · 2023-04-03T14:39:55Z

src/main/java/org/jabref/logic/msbib/MSBibMapping.java

@@ -131,8 +170,8 @@ public static MSBibEntryType getMSBibEntryType(EntryType bibtexType) {
     */
    public static int getLCID(String language) {
        // TODO: add language to LCID mapping


This TODO can be removed.

Also fix the checkstyle issue!

koppor · 2023-04-03T14:51:56Z

See also that the tests start to fail: Please check the tests:

yenniejunvu · 2023-04-03T15:14:06Z

My bad about the unit tests. I'll take a look and make some fixes. Thanks for the quick feedback.

yenniejunvu · 2023-04-05T01:19:15Z

@return Returns 0 for English

In regards to the unit tests failing,
I saw this comment above the getLCID function while trying to fix the issue, and noticed that if I set the LCID to '0' in the BiMap for english, the tests pass fine. However, according to documentation,
"Word sets LCID to 0 to mean the value should be determined at runtime by the application." (https://learn.microsoft.com/en-us/openspecs/office_standards/ms-oe376/6c085406-a698-4e12-9d4d-c3b0ee3dbc4a)

Not quite sure what to do here.

Additionally, since @Alexandra-Stath was assigned to this problem first, you do not need to review this PR until she submits a PR as well or says it is okay. I just do not want this to get merged, since she was technically assigned it first as a course project!

koppor · 2023-04-07T14:22:54Z

Without thinking much, my question would be, why can't 0 be kept?`

Are all these null pointer exceptions really because of this?

Siedlerchr · 2023-04-07T20:05:12Z

I think the tests are failing because the xml files for the tests probably contain 0 for the language. And there is no map entry with 0 inside. So either add an entry for 0 or do something like getOrDefault

src/test/java/org/jabref/logic/msbib/MsBibMappingTest.java

yenniejunvu · 2023-04-09T19:22:44Z

@Siedlerchr I'm not sure if that's the case, because the previous implementation of getLCID() returned the hexidecimal value of 1033, which is the LCID for american english.

koppor · 2023-04-10T11:21:35Z

@Siedlerchr I'm not sure if that's the case, because the previous implementation of getLCID() returned the hexidecimal value of 1033, which is the LCID for american english.

This is good!

Without thinking deeper about the consequences, just fallback to 1033 if there is no mapping in the map?

(And please adapt the wrong comment @return Returns 0 for English if not already done so)

Siedlerchr · 2023-04-10T16:06:30Z

Don't guess, check the files! https://github.com/JabRef/jabref/tree/6f9eaafce606e61426565f5316e8a0b6f68bf227/src/test/resources/org/jabref/logic/exporter
and https://github.com/JabRef/jabref/tree/6f9eaafce606e61426565f5316e8a0b6f68bf227/src/test/resources/org/jabref/logic/importer/fileformat

…into fix-for-9699

yenniejunvu · 2023-04-10T16:15:45Z

I added MSBibConverterTest but I am not sure if that is what you guys are expecting.
Additionally, I added some error handling. If there is an invalid language/lcid passed, it all defaults to english.

(my last commit had some missing files, which is why the unit tests were failing :,( )

Siedlerchr · 2023-04-10T16:32:14Z

src/main/java/org/jabref/logic/msbib/MSBibMapping.java

     */
    public static int getLCID(String language) {
+        if (!LANG_TO_LCID.containsKey(language)) {
+            return 1033;
+        }
        return LANG_TO_LCID.get(language);


You can use getOrDefault

yenniejunvu · 2023-04-12T20:17:22Z

Not sure why that one test is now suddenly failing, it works fine on my local machine...

src/main/java/org/jabref/logic/msbib/MSBibMapping.java

Siedlerchr · 2023-04-12T20:46:21Z

The test sometimes fails randomly on CI. Don't worry. If you think your PR is ready then you can mark it as ready for review. In my opinion it looks good now

Add import as well

koppor · 2023-04-15T21:16:28Z

CHANGELOG.md

@@ -381,7 +381,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We moved the union/intersection view button in the group sidepane to the left of the other controls. [#8202](https://github.com/JabRef/jabref/pull/8202)
 - We improved the Drag and Drop behavior in the "Customize Entry Types" Dialog [#6338](https://github.com/JabRef/jabref/issues/6338)
 - When determining the URL of an ArXiV eprint, the URL now points to the version [#8149](https://github.com/JabRef/jabref/pull/8149)
- We Included all standard fields with citation key when exporting to Old OpenOffice/LibreOffice Calc Format [#8176](https://github.com/JabRef/jabref/pull/8176)
+- We Included all standard fields with citation key when exporting to Old OpenOffice/LibreExpoa Calc Format [#8176](https://github.com/JabRef/jabref/pull/8176)


@Siedlerchr What is "LibreExpoa"? Is this change in CHANGELOG.md really intended?

I was editing this on the github, this was not intended

Fixed it directly in main at 461e915

fix for 9699. Added bimap for language-lcid mapping

7c344f4

koppor requested changes Apr 3, 2023

View reviewed changes

Siedlerchr added the status: changes required Pull requests that are not yet complete label Apr 3, 2023

ThiloteE added component: export-or-save component: microsoft-word-integration labels Apr 4, 2023

removed comments

7fa2a14

koppor removed the status: changes required Pull requests that are not yet complete label Apr 5, 2023

koppor marked this pull request as draft April 5, 2023 05:28

Merge branch 'main' into fix-for-9699

ffa8e9d

koppor reviewed Apr 8, 2023

View reviewed changes

src/test/java/org/jabref/logic/msbib/MsBibMappingTest.java Show resolved Hide resolved

yennie02 added 2 commits April 10, 2023 11:12

unit test fixes/invalid lang handling + added unit tests

67b18d7

Merge branch 'fix-for-9699' of https://github.com/yenniejunvu/jabref …

b31489f

…into fix-for-9699

added missing files

671a4f7

Siedlerchr reviewed Apr 10, 2023

View reviewed changes

use getOrDefault

c79eb5b

Siedlerchr reviewed Apr 12, 2023

View reviewed changes

src/main/java/org/jabref/logic/msbib/MSBibMapping.java Show resolved Hide resolved

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 13, 2023

Siedlerchr previously approved these changes Apr 13, 2023

View reviewed changes

comment added

d8d73c0

yenniejunvu dismissed Siedlerchr’s stale review via d8d73c0 April 13, 2023 18:28

yenniejunvu marked this pull request as ready for review April 13, 2023 18:28

Update CHANGELOG.md

d9f464d

Add import as well

Siedlerchr merged commit 62e3d3f into JabRef:main Apr 15, 2023

koppor reviewed Apr 15, 2023

View reviewed changes

koppor mentioned this pull request Mar 17, 2024

Keep enclosing braces of authors #11034

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for 9699. Added bimap for language-lcid mapping #9729

fix for 9699. Added bimap for language-lcid mapping #9729

yenniejunvu commented Apr 2, 2023 •

edited by Siedlerchr

Loading

Tasks

koppor left a comment •

edited by Siedlerchr

Loading

koppor Apr 3, 2023

yenniejunvu Apr 5, 2023

koppor Apr 7, 2023

koppor Apr 3, 2023

koppor commented Apr 3, 2023

yenniejunvu commented Apr 3, 2023

yenniejunvu commented Apr 5, 2023

koppor commented Apr 7, 2023

Siedlerchr commented Apr 7, 2023

yenniejunvu commented Apr 9, 2023

koppor commented Apr 10, 2023

Siedlerchr commented Apr 10, 2023

yenniejunvu commented Apr 10, 2023 •

edited

Loading

Siedlerchr Apr 10, 2023

yenniejunvu commented Apr 12, 2023

Siedlerchr commented Apr 12, 2023

koppor Apr 15, 2023

Siedlerchr Apr 15, 2023

koppor Apr 15, 2023

		@@ -24,6 +24,45 @@ public class MSBibMapping {

		private static final HashBiMap<Field, String> BIBLATEX_TO_MS_BIB = HashBiMap.create();

		private static final HashBiMap<String, Integer> LANG_TO_LCID = HashBiMap.create();

fix for 9699. Added bimap for language-lcid mapping #9729

fix for 9699. Added bimap for language-lcid mapping #9729

Conversation

yenniejunvu commented Apr 2, 2023 • edited by Siedlerchr Loading

Tasks

koppor left a comment • edited by Siedlerchr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Apr 3, 2023

yenniejunvu commented Apr 3, 2023

yenniejunvu commented Apr 5, 2023

koppor commented Apr 7, 2023

Siedlerchr commented Apr 7, 2023

yenniejunvu commented Apr 9, 2023

koppor commented Apr 10, 2023

Siedlerchr commented Apr 10, 2023

yenniejunvu commented Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

yenniejunvu commented Apr 12, 2023

Siedlerchr commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yenniejunvu commented Apr 2, 2023 •

edited by Siedlerchr

Loading

koppor left a comment •

edited by Siedlerchr

Loading

yenniejunvu commented Apr 10, 2023 •

edited

Loading