Some Globals.prefs injection in logic and model #1593

oscargus · 2016-07-16T13:03:36Z

Based on the discussion that Globals should not be accessed from logic and model.

lenhard · 2016-07-18T07:43:45Z

src/main/java/net/sf/jabref/JabRefPreferences.java

@@ -360,7 +360,7 @@
        activeFormatterCleanups.add(new FieldFormatterCleanup("title", new ProtectTermsFormatter()));
        activeFormatterCleanups.add(new FieldFormatterCleanup("title", new UnitsToLatexFormatter()));
        activeFormatterCleanups.add(new FieldFormatterCleanup("title", new LatexCleanupFormatter()));
-        activeFormatterCleanups.add(new FieldFormatterCleanup("title", new HtmlToLatexFormatter()));
+        activeFormatterCleanups.add(new FieldFormatterCleanup("title", new HtmlToLatexFormatter(false))); // TODO: Get the actual value USE_CONVERT_TO_EQUATION


Why is it not possible to use the actual value for USE_CONVERT_TO_EQUATION here? After all, this is the preferences class?

It cannot be accessed. prefs is non-static and Globals.prefs is not yet set (and this is executed before it can be set).

lenhard · 2016-07-18T08:00:45Z

Overall, one step in the right direction! However, there is one immediate issue that we need to consider: If we decouple a class completely from Globals and JabRefPreferences, for instance by just passing String values read from the preferences as parameters to the constructor, we might run into refreshment bugs. This means that if someone changes the preferences via the GUI, the change might have no effect on JabRef, since the value is not re-read every time it is needed. The obvious solution that comes to my mind to avoid this is the Windows approach: Always restart JabRef for preferences changes. What do @JabRef/developers think?

matthiasgeiger · 2016-07-18T08:15:56Z

The obvious solution that comes to my mind to avoid this is the Windows approach: Always restart JabRef for preferences changes.

😩 I don't like this. It already unnerves me that in some occassions a restart is needed.

I don't have the time (and I don't want to 😉) check this whole PR... but using params with the current settings, or recreating the object instances on pref changes is not possible?

lenhard · 2016-07-18T08:22:58Z

@matthiasgeiger At least I do not really know where every preference is needed. And I also do not know if it is possible to just recreate certain parts of JabRef on demand, without a restart of the program (because JabRef is just so tangled and that is what we are trying to improve here). @oscargus: Can you perhaps look at all occasions where you removed Globals and JabRefPreferences in this PR and try to guess if there might be problems when preferences are changed?

Could the event system be a way to make this work? (Think of a PreferencesChange event)

oscargus · 2016-07-18T10:31:30Z

It is definitely a problem for HtmlToLatexFormatter, but maybe we should remove that support anyway. It may be a problem for the AutoCompleter (not sure where the preferences are used there, if they are just passed on to LayOut, in which case it is used for formatting the File-formatters, so probably not an issue).

There are some way to alleviate this though:

In JournalAbbreviationLoader.getRepository() the JournalAbbreviationRepository is only generated once. Here one can probably check if the preferences (configuration object) have changed, and in that case generate a new JournalAbbreviationRepository. This would improve the behavior compared to the current one (before this PR).
In CustomExportList the JabRefPreferences are passed to every call. This is something I was considering fixing and only use the one from the constructor, but it is actually a feature here.

As it is said that this is an evolving process, what remains to be done? I think dropping the preferences from HtmlToLatexFormatter if nothing else. There are some clear cases where a specific preferences class and the generic one are both passed. This is something for later. Anything else?

lenhard · 2016-07-18T11:04:49Z

Though it is an evolving process, we should not introduce bugs in the meantime :-) In HTMLToLatexFormatter, you can remove the preferences configuration (which you already did).

As for the AutoCompleter, it should really still work and update as before the PR. So, if it doesn't at the moment you should implement an update of the preferences for the AutoCompleter, e.g. by recreating and resetting the object if changes in the preferences occur.

The whole rationale is: JabRef should behave in the same fashion after this PR as it did before (with the exception of HTMLToLatexFormatter). Architecture refactorings should not change functionality. It is quite hard for me to guarantee this, so it really depends on your intuition and testing.

If you find it too hard to implement a preferences update at a certain position, it would be better if you just reverted the dependency refactoring for that position. Then we can have a closer look at the specific dependency in another PR.

oscargus · 2016-07-18T11:19:12Z

Regarding AutoCompleter: it is currently (in the master branch) not updating when the preferences are changed. This PR actually makes it easier to update (although it is indeed behaving the same as before at the moment).

lenhard · 2016-07-18T11:26:51Z

Ok, then LGTM!

oscargus · 2016-07-18T11:37:17Z

To be more precise: JournalAbbreviationLoader.getRepository() used to get Globals.prefs on the first call. Now it is passed as an argument. Hence, any relevant changes in the preferences between starting (where e.g. AutoCompleter is initialized with the current Globals.prefs and later call getRepository()) and the first actual call to getRepository() (not sure when that is, it might be at start up as well and then there is no change) will be ignored now. However, the fundamental issue that any subsequent changes to the preferences after getRepository() is called first are ignored is still the same.

I think that the idea of a PreferenceChanged event is really the way to go. Although not obvious how to easily implement it at a "higher level", i.e. at the classes passing the preferences on rather than in the classes using the preferences.

tobiasdiez · 2016-07-18T12:25:57Z

src/main/java/net/sf/jabref/logic/exporter/BibtexDatabaseWriter.java

@@ -109,7 +113,10 @@ protected void writeString(BibtexString bibtexString, boolean isFirstString, int
                getWriter().write("{}");
            } else {
                try {
-                    String formatted = new LatexFieldFormatter().format(bibtexString.getContent(), LatexFieldFormatter.BIBTEX_STRING);
+                    String formatted = new LatexFieldFormatter(
+                            LatexFieldFormatterPreferences.fromPreferences(jabRefPreferences))


Could you include the LatexFieldFormatterPreferences in SavePreferences (and remove the explicit reference to JabRefPreferences from BibtexDatabaseWriter)

Yes, that would make sense. I just did something similar to AutoCompleterPreferences and JournalAbbreviationPreferences.

tobiasdiez · 2016-07-18T12:44:24Z

LGTM 👍 after having a quick look

oscargus · 2016-07-18T13:53:36Z

I should just note that I was incorrect about AutoCompleter/JournalAbbreviationLoader. It seems quite clever and I missed the point of having an update method. Everytime anything(?) related to the JournalAbbreviationLoader is changed in the preferences it is indeed updated.

simonharrer · 2016-07-19T12:19:05Z

Please rebase and merge.

* master: (268 commits) Added DateFormatter to LayoutEntry so that it actually works... (JabRef#1619) Implemented JabRef#1345 - cleanup ISSN (JabRef#1590) Always use https for help files (JabRef#1615) Resolves JabRef#1613 Use Jabref default icon for uninstaller for now (JabRef#1616) Added more fields and fixed some issues (JabRef#1617) Added LayoutFormatterPreferences (and related files) (JabRef#1608) [WIP] Create new fetcher infrastructure (JabRef#1594) Set user agent to fix 403 status error More fields added to FieldName (JabRef#1614) Added model.entry.FieldName that contains field name constants (JabRef#1602) Fixes imports Test CustomImporter (JabRef#1501) The field list gets the focus as soon as it is focused (JabRef#1541) When clicking on a tab, the first field now has the focus (JabRef#988) Add test in BibEntryWriterTest for type change Rewrite MedlineImporter with JAXB and add nbib fields (JabRef#1479) Some Globals.prefs injection in logic and model (JabRef#1593) Added filter to not show selected integrity checks (JabRef#1588) Replace getField with getFieldOptional in all of the tests and in som… (JabRef#1591) Move preferences (JabRef#1604) ... # Conflicts: # src/main/java/net/sf/jabref/importer/ImportMenuItem.java # src/main/java/net/sf/jabref/importer/OpenDatabaseAction.java

@comment

Move event (JabRef#1601) * Move event package to model Update dependencies: postgres 9.4.1208 -> 9.4.1209 and wiremock from 2.1.6 to 2.1.7 Added ISBN integrity checker (JabRef#1586) Added ISBN integrity checker * Extracted ISBN class Reenable errorprone (see http://errorprone.info/) Extend the OpenConsoleFeature (JabRef#1582) * Extend the OpenConsoleFeature by selection of custom terminal emulator. - Add radio selection to the AdvancedTab - Add new JabRefPreferences - Add file check and execution commands - Add localization keys * Fix localization key. * Move console selection to ExternalTab.java * Change localization entry. * Add command executor. * Fix placeholder replacement. * Fix codacy. * Update localization key. * Remove "Specify terminal emulator" option. Add GUI outputs. * Set default command for Windows. Fix localization entries. * Remove empty lines in language files. * Use lambda expressions insead of ActionListeners * Refactoring. * Update CHANGELOG.md. * Small refactorings. Move preferences (JabRef#1604) * Move preferences-related classes into separate package * Rename JabRefPreferencesFilterDialog -> PreferencesFilterDialog and move it to gui * Fix checkstyle warning Set user agent to fix 403 status error Replace getField with getFieldOptional in all of the tests and in som… (JabRef#1591) * Replace getField with getFieldOptional in all of the tests and in some more code * Some more conversions Added filter to not show selected integrity checks (JabRef#1588) * Added filter to not show selected integrity checks * Removed unused variable Some Globals.prefs injection in logic and model (JabRef#1593) * Some Globals.prefs injection in logic and model * Some more conversions and some fixes * More injections * Even more injections * Yes, even more injections * Indeed, even more injections * Probably the last injections for now * Removed unrequired dependency and fixed issue * Dropped support for selecting sub/super to equations * Added preference classes for LatexFieldFormatter and FieldContentParser * Removed some left over code * Added JournalAbbreviationPreferences * Encapsulated LatexFieldFormatterPreferences in SavePreferences * Changed getShortDescription to accept boolean argument * Removed Globals.prefs from tests and removed unused imports * Unused import * Unused import Rewrite MedlineImporter with JAXB and add nbib fields (JabRef#1479) Add test in BibEntryWriterTest for type change When clicking on a tab, the first field now has the focus (JabRef#988) * the first Field does now have focus when clicking on a tab in the entry editor * Make first field focused when selecting a tab in entry editor The field list gets the focus as soon as it is focused (JabRef#1541) Test CustomImporter (JabRef#1501) * Test CustomImporter Fixes imports Added model.entry.FieldName that contains field name constants (JabRef#1602) * Added model.entry.FieldName that contains field name constants * More constants * Renamed and added more constants * Some more fields and cleanups * Removed MedlineHandler left from merge conflicts More fields added to FieldName (JabRef#1614) * More fields added to FieldName * Some Medline fixes [WIP] Create new fetcher infrastructure (JabRef#1594) * Introduce new Fetcher interfaces * Refactor arXiv fulltext fetcher * Add query based arXiv fetcher * Reformat code * Add a few tests for the arxiv parser * Make new arXiv fetcher available * Fix small problems related to files * Fix tests * Rename interface methods * Add changelog entry * Mark old EntryFetcher interface as deprecated * Move fetcher to importer \ fetcher * Move HelpFile from gui.help to logic.help * Rename fetchers * Rename FulltextFinder * Optimize imports * Fix failing test * Ignore failing test Added LayoutFormatterPreferences (and related files) (JabRef#1608) * Added LayoutFormatterPreferences (and related files) * Rebased * Included JournalAbbreviationLoader in LayoutPreferences Added more fields and fixed some issues (JabRef#1617) Resolves JabRef#1613 Use Jabref default icon for uninstaller for now (JabRef#1616) Always use https for help files (JabRef#1615) Implemented JabRef#1345 - cleanup ISSN (JabRef#1590) * Implemented JabRef#1345 - cleanup ISSN * Fixed comments * Extracted ISSN class * Added tests for ISSN and ISBN Added DateFormatter to LayoutEntry so that it actually works... (JabRef#1619) Converted a few getField to getFieldOptional (JabRef#1625) * Converted a few getField to getFieldOptional Fixed JabRef#636 by using DOICheck and DOIStrip in export filters (JabRef#1624) Improved LaTeX to Unicode/HTML formatters to output more sensible values for unknown commands (JabRef#1622) Updated preview entries (JabRef#1606) * Updated preview entries, which return new entry Moved, removed, and used String constants (JabRef#1618) * Moved, removed, and used String constants * Some more fixes * Moved NEWLINE, made FILE_SEPARATOR public and used it * Moved NEWLINE and FILE_SEPARATOR to OS * Moved ENCODING_PREFIX and SIGNATURE * Corrected Globals in a few comments... * Apparently the localization tests find commented out text... More field names and a method (JabRef#1627) * Introduced FieldName in ArXiV * Some more field names * More field names Cleanup FindFile and asssociated tests (JabRef#1596) * Cleanup FindFile and rework it using Streams and nio methods- * Unignore test for trying on CI * Use explicit List and Set in findFiles and caller methods * Use Lazy Stream to find files changes should be tested manually Some enhancements and cleanups related to dates (JabRef#1575) * Some enhancements and cleanups related to dates * Fixed some time zone issues * Replaced SimpleDateFormat in ZipFileChooser and replaced arrays with Lists * Changed EasyDateFormat constructors * Fixed stupid mistake * Added CHANGELOG entry * Maybe tests are passing now? * Some server side print debugging... * As it should be * Tryng LocalDateTime * No time zone * Added test for Cookie * Fixed imports... * Added a third possible date format as it turns out that the server changed while developing this PR Builds are now stored via build-upload.jabref.org Consistent file name casing (and other localization improvements) (JabRef#1629) * AUX files * ZIP files * BIB files * JAR files * didn't * Couldn't what's * Consistent casing * AUX apparently is commonly used in French words... * Fixed the flawed quick-and-dirty find-and-replace failures define xjc input/ouput dir (subsequent builds will be faster) (JabRef#1628) Execute task only when input/output dir changed. Fixed a minor issue and refactored MergeEntries (JabRef#1634) * Fixed a minor issue and refactored MergeEntries * Fixed import * Added CHANGELOG entry Added LabelPatternPreferences (JabRef#1607) * Added LabelPatternPreferences * Removed static initializer More tests (JabRef#1635) * Added more tests for Cookie * Enabled some layout tests and added test for StringUtil.intValueOfWithNull * Updated a test * Split tests Updated Errorprone to 2.0.11 (JabRef#1636) * Updated Errorprone to 2.0.11 * Corrected test Keep @comment text in a bib file (JabRef#1638) * Kep @comment text in a bib file * Add test for @comment that contains regular entries Replaced some getField and fixed some bugs (JabRef#1631) * Replaced some getField and fixed some bugs * Fixed a few things * Added CHANGELOG entries * Improved equals implementation * Text book equals and hashCode Fixed JabRef#1639 (JabRef#1641) * Fixed JabRef#1639 * Removed old code Export OO/LO citations to new database (JabRef#1630) * Export OO/LO citations to new database * Fixed problem with duplicates * Added some comments * Fixed spelling in comment * Removed general Exception Unified some equals (JabRef#1640) * Unified some equals * Imported correct Objects... Fixed one more NPE which should have been fixed in JabRef#1631 (JabRef#1649) Finished method to hide visible fields and show hidden fields - Hide method done - Show method done - ToDo repaint hidden field - ToDo test class finished field repaint remove sysouts

oscargus added cleanup-ops status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers architecture labels Jul 16, 2016

oscargus mentioned this pull request Jul 17, 2016

Desired Architecture #1579

Closed

lenhard reviewed Jul 18, 2016
View reviewed changes

oscargus force-pushed the preferenceinjection branch from 21b8984 to 2ce40f8 Compare July 18, 2016 10:46

tobiasdiez reviewed Jul 18, 2016
View reviewed changes

oscargus force-pushed the preferenceinjection branch 3 times, most recently from 4776d71 to 88a48a2 Compare July 18, 2016 13:41

oscargus added 3 commits July 20, 2016 08:23

Some Globals.prefs injection in logic and model

fbe0290

Some more conversions and some fixes

3c4f11f

More injections

e33ba3e

oscargus added 11 commits July 20, 2016 08:26

Even more injections

c94ed22

Yes, even more injections

a7ef1e8

Indeed, even more injections

77b3c6f

Probably the last injections for now

cc3a821

Removed unrequired dependency and fixed issue

8fdd8b8

Dropped support for selecting sub/super to equations

2523592

Added preference classes for LatexFieldFormatter and FieldContentParser

431176b

Removed some left over code

b527f21

Added JournalAbbreviationPreferences

283dfcb

Encapsulated LatexFieldFormatterPreferences in SavePreferences

b73afb8

Changed getShortDescription to accept boolean argument

d03326a

oscargus force-pushed the preferenceinjection branch from 88a48a2 to d03326a Compare July 20, 2016 07:17

oscargus added 3 commits July 20, 2016 09:23

Removed Globals.prefs from tests and removed unused imports

9b75bee

Unused import

a92e658

Unused import

b2a437c

oscargus merged commit 90044ac into JabRef:master Jul 20, 2016

koppor mentioned this pull request Dec 22, 2019

Fix GUI tests #5781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some Globals.prefs injection in logic and model #1593

Some Globals.prefs injection in logic and model #1593

oscargus commented Jul 16, 2016

lenhard Jul 18, 2016

oscargus Jul 18, 2016

lenhard commented Jul 18, 2016

matthiasgeiger commented Jul 18, 2016

lenhard commented Jul 18, 2016 •

edited

Loading

oscargus commented Jul 18, 2016

lenhard commented Jul 18, 2016

oscargus commented Jul 18, 2016

lenhard commented Jul 18, 2016

oscargus commented Jul 18, 2016

tobiasdiez Jul 18, 2016 •

edited

Loading

oscargus Jul 18, 2016

tobiasdiez commented Jul 18, 2016

oscargus commented Jul 18, 2016

simonharrer commented Jul 19, 2016

Some Globals.prefs injection in logic and model #1593

Some Globals.prefs injection in logic and model #1593

Conversation

oscargus commented Jul 16, 2016

lenhard Jul 18, 2016

Choose a reason for hiding this comment

oscargus Jul 18, 2016

Choose a reason for hiding this comment

lenhard commented Jul 18, 2016

matthiasgeiger commented Jul 18, 2016

lenhard commented Jul 18, 2016 • edited Loading

oscargus commented Jul 18, 2016

lenhard commented Jul 18, 2016

oscargus commented Jul 18, 2016

lenhard commented Jul 18, 2016

oscargus commented Jul 18, 2016

tobiasdiez Jul 18, 2016 • edited Loading

Choose a reason for hiding this comment

oscargus Jul 18, 2016

Choose a reason for hiding this comment

tobiasdiez commented Jul 18, 2016

oscargus commented Jul 18, 2016

simonharrer commented Jul 19, 2016

lenhard commented Jul 18, 2016 •

edited

Loading

tobiasdiez Jul 18, 2016 •

edited

Loading