fix for 9699. Added bimap for language-lcid mapping by yenniejunvu · Pull Request #9729 · JabRef/jabref

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.

### Compulsory checks
- [x ] Change in `CHANGELOG.md` described in a way that is understandable for the average user (if applicable)
- [ ] Tests created for changes (if applicable)
- [ ] Manually tested changed features in running JabRef (always required)
- [ ] Screenshots added in PR description (for UI changes)
- [ x] [Checked developer's documentation](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
- [ x] [Checked documentation](https://docs.jabref.org/): Is the information available and up to date? If not, I created an issue at <https://github.com/JabRef/user-documentation/issues> or, even better, I submitted a pull request to the documentation repository.

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


    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

@@ -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

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

+        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...

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

 - 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

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

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

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

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


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

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

Uh oh!

Conversation

yenniejunvu commented Apr 2, 2023 • edited by Siedlerchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koppor left a comment • edited by Siedlerchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Apr 3, 2023

Uh oh!

yenniejunvu commented Apr 3, 2023

Uh oh!

yenniejunvu commented Apr 5, 2023

Uh oh!

koppor commented Apr 7, 2023

Uh oh!

Siedlerchr commented Apr 7, 2023

Uh oh!

Uh oh!

yenniejunvu commented Apr 9, 2023

Uh oh!

koppor commented Apr 10, 2023

Uh oh!

Siedlerchr commented Apr 10, 2023

Uh oh!

yenniejunvu commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yenniejunvu commented Apr 12, 2023

Uh oh!

Uh oh!

Siedlerchr commented Apr 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yenniejunvu commented Apr 2, 2023 •

edited by Siedlerchr

Loading

koppor left a comment •

edited by Siedlerchr

Loading

yenniejunvu commented Apr 10, 2023 •

edited

Loading