RTFchars fix by mairdl · Pull Request #2097 · JabRef/jabref

mairdl · 2016-09-29T14:50:01Z

RTFcharsTest had some ignored test. So I looked into it and made the necessery changes. I added some of the tested but missing special characters. I also modified the expected and actual vaulues where needed (because to my understanding of this topic, some values were wrong). Since I am not 100% sure if everything is correct, comments are appreciated.

Also I added "translator" which takes the unicode and transforms it back into its root letter.
E.g. é - > \\u233e

My final change is that I removed duplicate keys inside the RTFCharMap.

Change in CHANGELOG.md described
Tests work for changes
internal qs
double check mappings

mairdl · 2016-09-29T15:10:47Z

looks like i need some more testscases for my changes

boceckts

Only minor comments from my side.

boceckts · 2016-10-01T12:06:00Z

+    }
 }
+//if( <= c && c <= )
+//        return "";


Remove out commented code.

boceckts · 2016-10-01T12:15:49Z

        return new StringInt(format(res), part.length());
    }
+
+    private String transformSpecialCharacter(long c) {


Please add a javadoc comment.

boceckts · 2016-10-01T12:19:37Z

+            return "ss";
+        if(161 ==c)
+            return "!";
+        return "?";


I don't like the multiple return statements and no brackets. Please use a variable which gets returned once in the end of the method and use if else with brackets.

the way it is implemented now, has a better performance 👍

You can leave the return statements but please add the brackets.

Siedlerchr · 2016-10-01T17:49:34Z

What about a switch Statement with case?
Am 01.10.2016 2:20 nachm. schrieb "Tobias Boceck" <notifications@github.com

:

@boceckts commented on this pull request.

Only minor comments from my side.

In src/main/java/net/sf/jabref/logic/layout/format/RTFChars.java
#2097 (review):

}
+//if( <= c && c <= )
+// return "";

Remove out commented code.

In src/main/java/net/sf/jabref/logic/layout/format/RTFChars.java
#2097 (review):

@@ -199,4 +199,100 @@ private StringInt getPart(String text, int i, boolean commandNestedInBraces) {
// the wrong "}" at the end is removed by "format(res)"
return new StringInt(format(res), part.length());
}
+

private String transformSpecialCharacter(long c) {

Please add a javadoc comment.

In src/main/java/net/sf/jabref/logic/layout/format/RTFChars.java
#2097 (review):
       return "z";
   if(198 == c)
       return "AE";
   if(230 == c)
       return "ae";
   if(338 == c)
       return "OE";
   if(339 == c)
       return "oe";
   if(222 == c)
       return "TH";
   if(223 == c)
       return "ss";
   if(161 ==c)
       return "!";
   return "?";
I don't like the multiple return statements and no brackets. Please use a
variable which gets returned once in the end of the method and use if else
with brackets.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2097 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AATi5O3n-NXgNbOgUcLHNA6IpTMdkdKCks5qvlASgaJpZM4KKFcO
.

mairdl · 2016-10-01T21:56:05Z

I also thought about switch case, but i came to the conclusion it gets way to big 🎅

tobiasdiez

I can't really judge the correctness of your changes (I have no experience with the RTF stuff), thus only a few general comments about the code.

tobiasdiez · 2016-10-01T23:17:47Z

+            return "ss";
+        if(161 ==c)
+            return "!";
+        return "?";


You can leave the return statements but please add the brackets.

tobiasdiez · 2016-10-01T23:18:15Z

+            return "!";
+        return "?";
+    }
+}


Don't remove newlines at end.

I'll add the brackets and the newline.

tobiasdiez · 2016-10-02T10:45:11Z

-        put("?i", "\\'ed");
-        put("?o", "\\'f3");
-        put("?u", "\\'fa");
+        put("'a", "\\'e1");


Could you please explain the reason for this change (? -> ' )

Yes sure, later in the RTFCharMap you would find "'a" assigned with unicode, so I thought why not just map it to the RTF value if it already exists.

tobiasdiez · 2016-10-02T10:46:58Z

        // Use UNICODE characters for RTF-Chars which can not be found in the
        // standard codepage

-        put("`A", "\\u192A"); // "Agrave"


Why did you removed some of these mappings?

Some keys got 2 values assigned, so I removed the double assignement. If a key got an RTF value I removed the unicode value.

tobiasdiez · 2016-10-02T10:47:52Z

+        Assert.assertEquals("R\\u233eflexions sur le timing de la quantit\\u233e {\\u230ae} should be \\u230ae",
+                formatter.format("Réflexions sur le timing de la quantité {\\ae} should be æ"));
+
+        Assert.assertEquals("h\\'e1ll{\\u339oe}", formatter.format("h\\'all{\\oe}"));


Please only one assert per test method.

Some test methods inside this test class have lots of assertions for single characters, can I leave it like that? I will split the longer ones like you recommend.

tobiasdiez · 2016-10-02T10:47:55Z

+        Assert.assertEquals("R\\u233eflexions sur le timing de la quantit\\u233e {\\u230ae} should be \\u230ae",
+                formatter.format("Réflexions sur le timing de la quantité {\\ae} should be æ"));
+
+        Assert.assertEquals("h\\'e1ll{\\u339oe}", formatter.format("h\\'all{\\oe}"));


Please only one assert per test method.

lenhard · 2016-10-03T12:56:42Z

Code-wise this looks fine for me. Though I'll admit that I have not looked up the mapping table and would trust you on this.

I have one more question regarding code style that is not your fault, but could you improve it anyway? I do not like that RtfCharMap extends HashMap<String><String>, which is also completely unecessary. Through this, a malicious user could compromise the structure of the table itself. It would be better if RtfCharMap just used an internal HashMap. All you would need to offer to the outside is a get methods, because nothing more is used atm. If you add an internal private put method, you can also avoid rewriting the code.

mairdl · 2016-10-03T18:33:41Z

I looked the mappings up and the ones in JabRef seemed to be fine, so I used them. I will double check my code at the end to be 100% sure the mappings are fine. Also I will improve the code quality like @lenhard mentioned.

# Conflicts: # CHANGELOG.md

lenhard · 2016-10-04T12:57:41Z

This looks done, or is there something left? @mairdl If so, please merge master into your branch and we are ready to go.

mairdl · 2016-10-04T13:49:55Z

I'm on it.

# Conflicts: # CHANGELOG.md

mairdl · 2016-10-05T09:03:23Z

So can it be merged now?

lenhard · 2016-10-05T10:00:29Z

Yes, thanks for your contribution!

* upstream/master: (102 commits) Removed unused test code (#2140) Fix main table bug when creating a duplicate (#2135) Remove explicit author and add SPDX-License-Identifier Remove GPL from README.md and CONTRIBUTING.md fix preview update (#2125) Remove some UnicodeToLatex uses (#2132) Fix mixup in french/farsi localization FetcherException should extend JabRefException Fix exception when opening preference dialog (#2127) Unify ParserException and ParseException (#2124) Small refactoring in Importer package (#2053) Implement Datepicker "none"-button (#2122) revert change from 816d30c Change title/tooltip of source panel in biblatex mode (#2120) Refactoring: completey typed metadata and add detailed travis output (#2112) RTFchars fix (#2097) Fix NPE in Medline fetcher on missing ISSN (#2113) Ctrl-s parsing error message (#2114) Fix bad web search error messages (#2034) parse error freeze (#2106) ... # Conflicts: # src/main/java/net/sf/jabref/collab/FileUpdateMonitor.java # src/main/java/net/sf/jabref/gui/externalfiles/DownloadExternalFile.java # src/main/java/net/sf/jabref/gui/externalfiles/DroppedFileHandler.java # src/main/java/net/sf/jabref/gui/externalfiles/MoveFileAction.java # src/main/java/net/sf/jabref/logic/cleanup/RenamePdfCleanup.java # src/main/java/net/sf/jabref/logic/exporter/FileSaveSession.java # src/main/java/net/sf/jabref/logic/util/io/FileUtil.java # src/main/java/net/sf/jabref/preferences/JabRefPreferences.java

@ignore

* Fix RTFChars * Formatting, correcting wrong expected values * add missing keys and clean up * Improve formatting, fix minor issue * Rework tests to fit, remove @ignore * Remove duplicate keys * Make test fit new keys, add new test for RTFChars * Add additional complicated sentence * add a javadoc, remove unneeded lines * Add brackets to if statements * Split asserts up * use internal HashMap, remove extends HashMap * remove unneeded values * Add changes to CHANGELOG.md

mairdl added 5 commits September 29, 2016 16:37

Fix RTFChars

9b25d4e

Formatting, correcting wrong expected values

f057649

add missing keys and clean up

482eb36

Improve formatting, fix minor issue

35c7d71

Rework tests to fit, remove @ignore

687eca9

tschechlovdev added the stupro label Sep 29, 2016

mairdl added 3 commits September 30, 2016 14:31

Remove duplicate keys

89a38b4

Make test fit new keys, add new test for RTFChars

6b07774

Add additional complicated sentence

a11ce35

mairdl force-pushed the RTFCharsFix branch from 6f9014e to a11ce35 Compare September 30, 2016 13:16

boceckts added the stupro-ready-for-internal-review label Oct 1, 2016

boceckts reviewed Oct 1, 2016

View reviewed changes

boceckts removed the stupro-ready-for-internal-review label Oct 1, 2016

add a javadoc, remove unneeded lines

bc1bd01

tobiasdiez reviewed Oct 2, 2016

View reviewed changes

mairdl added 2 commits October 2, 2016 16:14

Add brackets to if statements

8b65738

Split asserts up

b08a1c7

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 2, 2016

boceckts changed the title ~~[WIP] RTFchars fix~~ RTFchars fix Oct 2, 2016

mairdl added 5 commits October 3, 2016 23:42

use internal HashMap, remove extends HashMap

f730e64

Merge remote-tracking branch 'upstream/master' into RTFCharsFix

0051c0d

remove unneeded values

148986f

Add changes to CHANGELOG.md

acddb26

4.10.16 Merge remote-tracking branch 'upstream/master' into RTFCharsFix

17e2348

# Conflicts: # CHANGELOG.md

4.10.16 Merge remote-tracking branch 'upstream/master' into RTFCharsFix

408e7ae

# Conflicts: # CHANGELOG.md

lenhard merged commit be390e4 into JabRef:master Oct 5, 2016

mairdl deleted the RTFCharsFix branch October 11, 2016 12:41

Uh oh!

Conversation

mairdl commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mairdl commented Sep 29, 2016

Uh oh!

boceckts left a comment

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!

boceckts Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mairdl Oct 1, 2016 • edited 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!

Siedlerchr commented Oct 1, 2016

Only minor comments from my side.

Remove out commented code.

Please add a javadoc comment.

Uh oh!

mairdl commented Oct 1, 2016

Uh oh!

tobiasdiez left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

lenhard commented Oct 3, 2016

Uh oh!

mairdl commented Oct 3, 2016

Uh oh!

lenhard commented Oct 4, 2016

Uh oh!

mairdl commented Oct 4, 2016

Uh oh!

mairdl commented Oct 5, 2016

Uh oh!

lenhard commented Oct 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mairdl commented Sep 29, 2016 •

edited

Loading

boceckts Oct 1, 2016 •

edited

Loading

mairdl Oct 1, 2016 •

edited

Loading