DI Part A by calixtus · Pull Request #11342 · JabRef/jabref

calixtus · 2024-05-27T19:32:04Z

First step towards implementing proper dependency injection (DI)

Major changes

Migrates the old globals to the Injector map modelsAndServices
Removes Globals class
Removes DefaultInjector class
Refactored KeyBindingRepository to singleton
Replaced convertToString implementation with Guava string splitter and added tests
Collected logging intialization in one method
Refactored Launcher
Moved initialization stuff in JabRefGUI to a new method

Mandatory checks

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)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: 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.

# Conflicts: # src/main/java/org/jabref/gui/fieldeditors/OptionEditor.java # src/main/java/org/jabref/gui/util/component/TemporalAccessorPicker.java

koppor

Some small nitpick comments ^^

koppor · 2024-06-04T19:42:31Z

+        PreferencesService preferencesService = Injector.instantiateModelOrService(PreferencesService.class);
+        BibtexParser parser = new BibtexParser(preferencesService.getImportFormatPreferences());


Isn't it possible to Injector.instantiateModelOrService(ImportFormatPreferences)?

This would require to register the ImportFormatPreferences at launch time of jabref in the afterburner. Now we have lazy loading implemented. Would probably increase the launch time, as some of the preferences objects require some more construction time.

koppor · 2024-06-04T19:43:59Z

                        preferencesService.getXmpPreferences(),
                        preferencesService.getFilePreferences(),
                        preferencesService.getLibraryPreferences().getDefaultBibDatabaseMode(),


Isn't it possible to

Injector.instantiateModelOrService(XmpPreferences.class), Injector.instantiateModelOrService(FilePreferences.class), Injector.instantiateModelOrService(LibraryPreferences.class),

koppor · 2024-06-04T19:44:29Z

                        preferencesService.getLibraryPreferences().getDefaultBibDatabaseMode(),
-                        Globals.entryTypesManager,
+                        Injector.instantiateModelOrService(BibEntryTypesManager.class),
                        preferencesService.getFieldPreferences(),


Injector.instantiateModelOrService(FieldPreferences.class),

koppor · 2024-06-04T20:10:33Z

     * @param pr    The result of the BIB parse operation.
     */
-    void performAction(ParserResult pr, DialogService dialogService);
+    void performAction(ParserResult pr, DialogService dialogService, PreferencesService preferencesService);


Can't the injector be used "downstream"?

With a proper DI framework, yes. In the next iteration step

koppor · 2024-06-04T20:14:12Z

        fieldColumn.setCellValueFactory(cellData -> BindingsHelper.constantOf(cellData.getValue()));
        new ValueTableCellFactory<Field, Field>()
-                .withText(FieldsUtil::getNameWithType)
+                .withText(item -> FieldsUtil.getNameWithType(item, preferencesService, undoManager))


Shouldn't the Injector be used inside FieldsUtil to keep the method signature small?

Could be argued. For me the idea was the same with EntryEditor and the RelatedArticlesTab. As afterburner is not an di framework it just provides injection for a presenter in the PresenterFactory. Using the Injector class is not really the intended use and would not be really cleaner than using the Globals class. So I decided for constructor injection here.

koppor · 2024-06-04T20:14:40Z

+                    styleSource,
+                    outputFormat,
+                    stateManager.getActiveDatabase().get(),
+                    Injector.instantiateModelOrService(BibEntryTypesManager.class));


Can't the Injector be used inside CitationStyleGenerator?

Yes, could be, but using a proper DI framework is just the next iteration step.

koppor · 2024-06-04T20:37:11Z

-                pdfIndexer = PdfIndexerManager.getIndexer(Globals.stateManager.getActiveDatabase().get(), Globals.prefs.getFilePreferences());
+                StateManager stateManager = Injector.instantiateModelOrService(StateManager.class);
+                PreferencesService preferencesService = Injector.instantiateModelOrService(PreferencesService.class);
+                pdfIndexer = PdfIndexerManager.getIndexer(stateManager.getActiveDatabase().get(), preferencesService.getFilePreferences());


Use Injector inside PdfIndexerManger to get an instance of FilePrefererences instead of using the constructor?

Using contructor injection is always the cleanest solution for testing. About injection of smaller objects see above. Would require to abandon lazy loading of the preferences objects.

I asked at jvm-german for the first point (https://jvm-german.slack.com/archives/C4HLX61PY/p1717757611057629).

For the second point, I don't fully understand as I thought that DI really leads to lazy loading, because the injection happens at the latest possible time (if the Injector is called directly). Let's first solve the general discussion injection via constructor vs. injection at variables vs. injection using direct calls. And then, the other things will also get solves surely.

# Conflicts: # src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

# Conflicts: # src/main/java/org/jabref/gui/fieldeditors/optioneditors/OptionEditor.java

# Conflicts: # src/main/java/org/jabref/gui/entryeditor/EntryEditor.java # src/main/java/org/jabref/gui/entryeditor/RelatedArticlesTab.java # src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationRelationsTab.java

# Conflicts: # src/main/java/org/jabref/gui/entryeditor/EntryEditor.java # src/main/java/org/jabref/gui/entryeditor/RelatedArticlesTab.java # src/main/java/org/jabref/gui/fieldeditors/ISSNEditor.java # src/main/java/org/jabref/gui/fieldeditors/JournalEditor.java # src/main/java/org/jabref/gui/fieldeditors/OwnerEditor.java # src/main/java/org/jabref/gui/fieldeditors/PersonsEditor.java # src/main/java/org/jabref/gui/fieldeditors/SimpleEditor.java

koppor · 2024-06-17T20:54:48Z

~~The code locally checked out is not the same as treated by GitHub 😱~~

Update: We had some remote branch issues (same branch name on two upstreams).

github-actions · 2024-06-17T21:06:57Z

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

LoayGhreeb · 2024-06-18T04:12:22Z

@calixtus, saving preferences throws an exception:

ERROR: Uncaught exception occurred in Thread[#53,JavaFX Application Thread,5,main]: java.lang.IllegalStateException: Cannot instantiate view: interface org.jabref.gui.frame.UiMessageHandler
	at afterburner.fx/com.airhacks.afterburner.injection.Injector.lambda$getDefaultInstanceSupplier$6(Injector.java:237)
	at afterburner.fx/com.airhacks.afterburner.injection.Injector.instantiateModelOrService(Injector.java:111)
	at afterburner.fx/com.airhacks.afterburner.injection.Injector.instantiateModelOrService(Injector.java:104)
	at org.jabref@100.0.0/org.jabref.gui.preferences.general.GeneralTabViewModel.storeSettings(GeneralTabViewModel.java:265)
	at org.jabref@100.0.0/org.jabref.gui.preferences.AbstractPreferenceTabView.storeSettings(AbstractPreferenceTabView.java:34)
	at org.jabref@100.0.0/org.jabref.gui.preferences.PreferencesDialogViewModel.storeAllSettings(PreferencesDialogViewModel.java:183)
	at org.jabref@100.0.0/org.jabref.gui.preferences.PreferencesDialogView.savePreferencesAndCloseDialog(PreferencesDialogView.java:132)
	at org.jabref@100.0.0/org.jabref.gui.preferences.PreferencesDialogView.lambda$new$0(PreferencesDialogView.java:56)
	at org.jabref@100.0.0/org.jabref.gui.util.ControlHelper.lambda$setAction$0(ControlHelper.java:29)

calixtus · 2024-06-18T05:37:45Z

Thanks. Hotfix created.

calixtus and others added 30 commits May 23, 2024 00:59

Migrate BibEntryTypesManager

418abad

Fix deprecation

a72a5ab

Migrate UndoManager

fcc1b86

Remove BibEntryTypesManager from Globals

da9b991

Migrate PreferencesService

f174227

Migrate DialogService

e358d17

Migrate ThemeManager

7bae500

Migrate StateManager

6d34bae

Migrate TaskExecutor

3d58c7f

Migrate KeyBindingRepository WIP

513d577

Fix StringToList conversion

b1a7a68

Migrate KeyBindingRepository

f11d97c

Fix some tests

5530353

Checkstyle

37fa15b

Rewrite

a5674f4

Checkstyle

1a572c9

Fix tests

7ec1dbc

Migrate ProtectedTermsLoader

4b5b72e

Refactor Launcher and extract logging initialization

7d93569

Migrate JournalAbbreviationsRepository

555824c

Move initialization to initialize

fd17b7a

Migrate ClipBoardManager

efc6767

Migrate RemoteListenerServerManager

103a718

Migrate BuildInfo

8bac78a

Merge remote-tracking branch 'refs/remotes/upstream/main' into di

55a1345

# Conflicts: # src/main/java/org/jabref/gui/fieldeditors/OptionEditor.java # src/main/java/org/jabref/gui/util/component/TemporalAccessorPicker.java

Fix merge errors

9aa8cc4

Add some text on DI

5a085e1

Migrate FileUpdateMonitor and DirectoryMonitor, Remove Globals

5d55857

Remove DefaultInjector

7154f80

Merge remote-tracking branch 'koppor/di' into di

3f0a318

koppor reviewed Jun 4, 2024

View reviewed changes

calixtus added 6 commits June 5, 2024 00:03

Remove null check for DirectoryMonitor

70aaefa

Add javadoc to UiMessageHandler

6bc49ea

Split JabRefPreferencesTest

c074bc1

Merge remote-tracking branch 'refs/remotes/upstream/main' into di

1e8ea60

# Conflicts: # src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

Merge remote-tracking branch 'refs/remotes/upstream/di' into di

6f46e3e

Merge remote-tracking branch 'refs/remotes/upstream/main' into di

81f6e48

# Conflicts: # src/main/java/org/jabref/gui/fieldeditors/optioneditors/OptionEditor.java

koppor previously approved these changes Jun 17, 2024

View reviewed changes

calixtus dismissed koppor’s stale review via c39b382 June 17, 2024 20:29

koppor enabled auto-merge June 17, 2024 20:47

Merge branch 'main' into di

c4a17ea

koppor previously approved these changes Jun 17, 2024

View reviewed changes

koppor disabled auto-merge June 17, 2024 20:54

Add missing field

fb3241f

koppor dismissed their stale review via fb3241f June 17, 2024 20:58

koppor enabled auto-merge June 17, 2024 20:59

koppor approved these changes Jun 17, 2024

View reviewed changes

koppor added this pull request to the merge queue Jun 17, 2024

Merged via the queue into main with commit e6b5470 Jun 17, 2024

koppor deleted the di branch June 17, 2024 21:14

calixtus mentioned this pull request Jun 18, 2024

Hotfix preferences cannot be stored #11387

Merged

6 tasks

calixtus mentioned this pull request Jun 18, 2024

Remove telemetry #11389

Merged

6 tasks

calixtus mentioned this pull request Oct 14, 2024

Replace new instantiation of BuildInfo with calls to Injector #11954

Merged

6 tasks

		PreferencesService preferencesService = Injector.instantiateModelOrService(PreferencesService.class);
		BibtexParser parser = new BibtexParser(preferencesService.getImportFormatPreferences());

Uh oh!

Conversation

calixtus commented May 27, 2024 • edited by ThiloteE Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Mandatory checks

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor Jun 4, 2024 • edited by calixtus 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!

Uh oh!

koppor commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LoayGhreeb commented Jun 18, 2024

Uh oh!

calixtus commented Jun 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

calixtus commented May 27, 2024 •

edited by ThiloteE

Loading

koppor Jun 4, 2024 •

edited by calixtus

Loading

koppor commented Jun 17, 2024 •

edited

Loading

github-actions Bot commented Jun 17, 2024 •

edited

Loading