Fix location bundle with fast access by halirutan · Pull Request #3327 · JabRef/jabref

halirutan · 2017-10-20T06:09:44Z

This PR addresses the issue I mentioned under Localization memory. To provide a reasonable implementation and not only a bugfix I had to change the layout. These things bothered me the most:

LocalizationBundle and Localization were calling each other. We needed Localization to create the keys in the bundle and on the other hand, the LocalizationBundle was only used in the Localization class
Localization seemed like a quick fix to provide messages as bundle to Java FX
On each call to any function in these classes, the unescaping of messages was done again. This, in particular, was the reason why showed up as hot spot in the profiler

To unify the implementation, I based the complete Localization framework on two instances (menus and messages) of LocalizationBundle and made this an internal class as it should not be used without it. The bundles are (re)created when setLanguage is called and contain the unescaped keys and messages for the specified language as a fast hash table.

This enables two things: (1) We can pass the LocalizationBundle instance directly to Java FX with getMessages and since we only pass the reference to the hash table, it will be instantaneous. (2) Every call to lang or menuTitle is a simple lookup in the hash table and a replacement of possible parameters in the string.

The public API for Localization, meaning lang, menuTitle, getMessages and setLanguage did not change and works as expected. During profiling/debugging I noticed, however, that there is a flaw in the JabRef startup. The language is set after the initialization of the preferences. At least one preference setting uses a localized string in a static field. Since when uninitialized, the Localization reverts back to "en", these strings will never be in the user language (see this issue). Furthermore, due to this access, the Localization framework will always be initialized several times. Once in the init of the preferences and once further down in main.

This can only be fixed by pushing setLanguage to the initialization of the preferences which I did. One additional minor thing was that one message key was used escaped and had to be unescaped. Other than that, this implementation worked out of the box and passed all existing localization tests.

I commented my code heavily and would be glad if someone would review it. Don't forget that I'm looking only about 5 days on the code base and it is very likely I have overlooked something.

…ed, that all accesses to it are only lookups in hashmaps Made all access of Location-messages a fast hash table lookup. Fixed one message in JabRefFrame that used an escaped key instead of the unescaped version.

* Fix NPE in MainTable * Fix build

Siedlerchr · 2017-10-20T08:48:50Z

+    public static String lang(String key, String... params) {
+        if (localizedMessages == null) {
+            // I'm logging this because it should never happen
+            LOGGER.error("Messages are not initialized in " + Localization.class);


Yozu can omit the class part when writing a log message, as the Logger is already instantiated for the current class and therefore knows its name.

Siedlerchr · 2017-10-20T08:54:09Z

+     */
+    private static Hashtable<String, String> createLookupTable(ResourceBundle baseBundle){
+        final ArrayList<String> baseKeys = Collections.list(baseBundle.getKeys());
+        Hashtable<String, String> lookup = new Hashtable<>(baseKeys.size());


HashTable is deprecated, better use HashMap.
And with java 8 streams and the Collection methods you an even make this code a lot nicer:
https://www.mkyong.com/java8/java-8-convert-list-to-map/

Siedlerchr · 2017-10-20T08:54:45Z

-    protected static String translate(ResourceBundle resBundle, String idForErrorMessage, String key, String... params) {
-        Objects.requireNonNull(resBundle);
+    private static String lookup(LocalizationBundle bundle, String idForErrorMessage, String key, String... params) {
+        if (key == null) {


Better use the Objects.requireNonNull here. It will automatically throw an NPE

Siedlerchr · 2017-10-20T08:56:10Z

+    private static class LocalizationBundle extends ResourceBundle {
+
+        // I'm using a Hashtable since it allows to get the keys directly as an Enumeration
+        private Hashtable<String, String> lookup;


As mentioned above, use as HashMap

Siedlerchr

First of all thanks for the analysis and for providing a fix!
I added some minor code wise suggestions, but in general looks good

lenhard

I have added a few minor comments on top of those of @Siedlerchr, but overall the code looks good.

I have also played around with a running JabRef, changing the localization etc., in this branch and did not notice errors.

I think your refactoring of the localization classes is fine, as well as moving the initialization into the preferences. So, when the few minor things are fixed, this PR is good to go.

lenhard · 2017-10-20T10:04:29Z

+        ResourceBundle menuTitles = ResourceBundle.getBundle(MENU_RESOURCE_PREFIX, locale, new EncodingControl(StandardCharsets.UTF_8));
+        // Just for the case something really stupid happened.
+        if (messages == null || menuTitles == null) {
+            LOGGER.error("Could not load language translation files in " + Localization.class);


Same remark regarding the class files as by @Siedlerchr above

lenhard · 2017-10-20T10:05:04Z

+        // Just for the case something really stupid happened.
+        if (messages == null || menuTitles == null) {
+            LOGGER.error("Could not load language translation files in " + Localization.class);
+            throw new NullPointerException();


I think throwing an IllegalStateException would be better here. Just for the more descriptive exception type.

I used

Objects.requireNonNull(messages, "Could not load " + RESOURCE_PREFIX + " resource."); Objects.requireNonNull(menuTitles, "Could not load " + MENU_RESOURCE_PREFIX + " resource.");

which throws NPE, but gives a description.

lenhard · 2017-10-20T10:06:08Z

-        if (menuTitles == null) {
-            setLanguage("en");
+        public final Object handleGetObject(String key) {
+            if (key == null) {


As above, rather use Objects.requireNonNull here.

General code cleanup

halirutan · 2017-10-20T10:29:16Z

I fixed the issues. Is there a nicer way to create a HashMap from the Collectors.toMap than simply wrapping it around?

Siedlerchr · 2017-10-20T11:25:35Z

Unfortunately there is no simpler solution for the Map thing.
Looks good to me, but you have still some checkstyle issues, here is a template? for Intellij:
https://github.com/JabRef/jabref/tree/master/config

Replacement of last null test with Objects.requireNonNull

lenhard · 2017-10-20T17:52:32Z

Since the checkstyle problems are fixed, I'll merge this now.

Thanks a lot for your contribution!

* upstream/master: Change integrity message for names depending on database mode (#3330) Fix location bundle with fast access (#3327) Small code cleanup Fix "path not found" exception # Conflicts: # src/main/java/org/jabref/logic/l10n/Localization.java

* upstream/master: Initializing EntryEditor Tabs on focus (#3331) Fix #3292: annotations are now automatically refreshed (#3325) Change integrity message for names depending on database mode (#3330) Fix location bundle with fast access (#3327) Small code cleanup Fix "path not found" exception Small fix in drag and drop handler of linked files (#3328) # Conflicts: # src/main/java/org/jabref/gui/DefaultInjector.java

* upstream/master: (31 commits) Source tab entry duplication (#3360) Use CITE_COMMENT not only for external latex editors but also for cop… (#3351) Updating with new translations (#3348) Upgrade error-prone (#3350) Jabref_pt_BR partially updated (#3349) Used late initialization for context menus (#3340) Fix NPE when calling with bib file as cmd argument (#3343) update mockito-core from 2.10.0 -> 2.11.0 (#3338) Remove underscore in Localized messages (#3336) Localisation: French: new entries translated (#3337) Refactoring: Lazy init of all editor tabs (#3333) Initializing EntryEditor Tabs on focus (#3331) Fix #3292: annotations are now automatically refreshed (#3325) Change integrity message for names depending on database mode (#3330) Fix location bundle with fast access (#3327) Small code cleanup Fix "path not found" exception Small fix in drag and drop handler of linked files (#3328) Fix NPE in MainTable (#3318) Increase relative size of abstract field in editor (#3320) ...

halirutan and others added 2 commits October 20, 2017 07:13

Fix NPE in MainTable (JabRef#3318)

ea60d26

* Fix NPE in MainTable * Fix build

Siedlerchr reviewed Oct 20, 2017

View reviewed changes

Siedlerchr requested changes Oct 20, 2017

View reviewed changes

lenhard suggested changes Oct 20, 2017

View reviewed changes

halirutan added 2 commits October 20, 2017 12:21

Use of HashMap instead of Hashtable

fa0f17b

General code cleanup

Forgot to replace one null test

f7a98ef

Siedlerchr approved these changes Oct 20, 2017

View reviewed changes

Reordering of import statements and fields

fdbf564

Replacement of last null test with Objects.requireNonNull

lenhard approved these changes Oct 20, 2017

View reviewed changes

lenhard merged commit 1df1612 into JabRef:master Oct 20, 2017

lenhard mentioned this pull request Oct 20, 2017

GUI button label is not correctly translated #3326

Closed

halirutan deleted the fix-LocationBundleWithFastAccess branch October 24, 2017 01:07

Uh oh!

Conversation

halirutan commented Oct 20, 2017 • 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!

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!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

lenhard 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!

halirutan commented Oct 20, 2017

Uh oh!

Siedlerchr commented Oct 20, 2017

Uh oh!

lenhard commented Oct 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

halirutan commented Oct 20, 2017 •

edited

Loading