Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OO Maintenance: Better checks, clean up dead code, refactoring #11779

Merged
merged 15 commits into from
Sep 23, 2024
Merged

OO Maintenance: Better checks, clean up dead code, refactoring #11779

Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
150603c
Clean up dead code
subhramit Sep 16, 2024
d21a455
Use typed exception handling
subhramit Sep 16, 2024
041bf02
Rename prefs variable
subhramit Sep 16, 2024
3dd04a4
Merge branch 'main' into oo-maintenance
subhramit Sep 16, 2024
f759087
Minimize usage of preferences
subhramit Sep 16, 2024
985c9a8
Merge branch 'main' into oo-maintenance
subhramit Sep 16, 2024
cb1bcc8
Merge branch 'main' into oo-maintenance
subhramit Sep 18, 2024
073d550
Merge branch 'main' into oo-maintenance
subhramit Sep 21, 2024
7958e7d
Enhanced pre-condition checks
subhramit Sep 22, 2024
68233dd
Merge condition
subhramit Sep 22, 2024
6594375
Reorder flags
subhramit Sep 22, 2024
a913ebe
Merge branch 'main' into oo-maintenance
subhramit Sep 22, 2024
6131662
Revert check: entry selection presence
subhramit Sep 22, 2024
2e77921
Fix indent
subhramit Sep 22, 2024
9e76718
Merge branch 'main' into oo-maintenance
subhramit Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 22 additions & 36 deletions src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ public OpenOfficePanel(LibraryTabContainer tabContainer,
this.stateManager = stateManager;
this.clipBoardManager = clipBoardManager;
this.undoManager = undoManager;
this.currentStyle = preferences.getOpenOfficePreferences().getCurrentStyle();
this.openOfficePreferences = preferences.getOpenOfficePreferences();
this.currentStyle = openOfficePreferences.getCurrentStyle();

ActionFactory factory = new ActionFactory();

Expand Down Expand Up @@ -157,7 +157,7 @@ public OpenOfficePanel(LibraryTabContainer tabContainer,
update.setMaxWidth(Double.MAX_VALUE);

loader = new StyleLoader(
preferences.getOpenOfficePreferences(),
openOfficePreferences,
preferences.getLayoutFormatterPreferences(),
abbreviationRepository);

Expand Down Expand Up @@ -327,7 +327,7 @@ private void exportEntries() {

private List<BibDatabase> getBaseList() {
List<BibDatabase> databases = new ArrayList<>();
if (preferences.getOpenOfficePreferences().getUseAllDatabases()) {
if (openOfficePreferences.getUseAllDatabases()) {
for (BibDatabaseContext database : stateManager.getOpenDatabases()) {
databases.add(database.getDatabase());
}
Expand All @@ -341,7 +341,7 @@ private List<BibDatabase> getBaseList() {
}

private void connectAutomatically() {
DetectOpenOfficeInstallation officeInstallation = new DetectOpenOfficeInstallation(preferences.getOpenOfficePreferences(), dialogService);
DetectOpenOfficeInstallation officeInstallation = new DetectOpenOfficeInstallation(openOfficePreferences, dialogService);

if (officeInstallation.isExecutablePathDefined()) {
connect();
Expand Down Expand Up @@ -375,7 +375,7 @@ private void connectManually() {
var fileDialogConfiguration = new DirectoryDialogConfiguration.Builder().withInitialDirectory(System.getProperty("user.home")).build();
Optional<Path> selectedPath = dialogService.showDirectorySelectionDialog(fileDialogConfiguration);

DetectOpenOfficeInstallation officeInstallation = new DetectOpenOfficeInstallation(preferences.getOpenOfficePreferences(), dialogService);
DetectOpenOfficeInstallation officeInstallation = new DetectOpenOfficeInstallation(openOfficePreferences, dialogService);

if (selectedPath.isPresent()) {
BackgroundTask.wrap(() -> officeInstallation.setOpenOfficePreferences(selectedPath.get()))
Expand All @@ -397,28 +397,20 @@ private void updateButtonAvailability() {
boolean isConnected = ooBase != null;
boolean isConnectedToDocument = isConnected && !ooBase.isDocumentConnectionMissing();

// For these, we need to watch something
boolean hasStyle = true; // (style != null);
boolean hasDatabase = true; // !getBaseList().isEmpty();
Comment on lines -401 to -402
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two comments (non-null check / isEmpty) are typical assert statements.

Copy link
Collaborator Author

@subhramit subhramit Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay wait, don't merge yet, I'll see if I can put those conditions/assertions (replacing style->currentStyle) instead of complete removal. I don't know if there were reasons these were commented and true was hardcoded.

boolean hasSelectedBibEntry = true;

selectDocument.setDisable(!isConnected);
pushEntries.setDisable(!(isConnectedToDocument && hasStyle && hasDatabase));
pushEntries.setDisable(!isConnectedToDocument);

boolean canCite = isConnectedToDocument && hasStyle && hasSelectedBibEntry;
boolean cslStyleSelected = preferences.getOpenOfficePreferences().getCurrentStyle() instanceof CitationStyle;
pushEntriesInt.setDisable(!canCite);
pushEntriesEmpty.setDisable(!canCite);
pushEntriesAdvanced.setDisable(!canCite || cslStyleSelected);
boolean cslStyleSelected = openOfficePreferences.getCurrentStyle() instanceof CitationStyle;
pushEntriesInt.setDisable(!isConnectedToDocument);
pushEntriesEmpty.setDisable(!isConnectedToDocument);
pushEntriesAdvanced.setDisable(!isConnectedToDocument || cslStyleSelected);

boolean canRefreshDocument = isConnectedToDocument && hasStyle;
update.setDisable(!isConnectedToDocument);
merge.setDisable(!isConnectedToDocument || cslStyleSelected);
unmerge.setDisable(!isConnectedToDocument || cslStyleSelected);
manageCitations.setDisable(!isConnectedToDocument || cslStyleSelected);

update.setDisable(!canRefreshDocument);
merge.setDisable(!canRefreshDocument || cslStyleSelected);
unmerge.setDisable(!canRefreshDocument || cslStyleSelected);
manageCitations.setDisable(!canRefreshDocument || cslStyleSelected);

exportCitations.setDisable(!(isConnectedToDocument && hasDatabase) || cslStyleSelected);
exportCitations.setDisable(!isConnectedToDocument || cslStyleSelected);
}

private void connect() {
Expand All @@ -427,7 +419,7 @@ private void connect() {
protected OOBibBase call() throws BootstrapException, CreationException {
updateProgress(ProgressBar.INDETERMINATE_PROGRESS, ProgressBar.INDETERMINATE_PROGRESS);

Path path = Path.of(preferences.getOpenOfficePreferences().getExecutablePath());
Path path = Path.of(openOfficePreferences.getExecutablePath());
return createBibBase(path);
}
};
Expand All @@ -451,13 +443,13 @@ protected OOBibBase call() throws BootstrapException, CreationException {
LOGGER.error("autodetect failed", ex);
switch (ex) {
case UnsatisfiedLinkError unsatisfiedLinkError -> {
LOGGER.warn("Could not connect to running OpenOffice/LibreOffice", ex);
LOGGER.warn("Could not connect to running OpenOffice/LibreOffice", unsatisfiedLinkError);

dialogService.showErrorDialogAndWait(Localization.lang("Unable to connect. One possible reason is that JabRef "
+ "and OpenOffice/LibreOffice are not both running in either 32 bit mode or 64 bit mode."));
}
case IOException ioException -> {
LOGGER.warn("Could not connect to running OpenOffice/LibreOffice", ex);
LOGGER.warn("Could not connect to running OpenOffice/LibreOffice", ioException);

dialogService.showErrorDialogAndWait(Localization.lang("Could not connect to running OpenOffice/LibreOffice."),
Localization.lang("Could not connect to running OpenOffice/LibreOffice.")
Expand Down Expand Up @@ -511,12 +503,6 @@ private void pushEntries(CitationType citationType, boolean addPageInfo) {
}

final BibDatabaseContext bibDatabaseContext = stateManager.getActiveDatabase().get();
if (bibDatabaseContext == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future: It is ok to add assert statements.

Example:

assert bibDatabaseContext  != null

OK, one needs to add -ea somehow: https://stackoverflow.com/a/18168305/873282 -- I would have bet that I saw assertion errors when developing...

The long discussion at https://stackoverflow.com/q/2758224/873282 says: do something else, but I like assertions:

  1. More semantic meaning than removed code
  2. More semantic meaning than code comments
  3. Runs in debug mode only (in contrast to Objects.requireNonNull
  4. Does not clutter the code with @NonNull annotations, which need a separate framework for checking - (https://github.com/kelloggm/checkerframework-gradle-plugin)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, will follow that.
Reason for removal here was that bibDatabaseContext could never be null here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. At the time of the code. But what if there is a future refactoring. One could argue that the whole research of pre- and post conditions is useless, because the author was sure at when writing the code 🤣🤣

This kind of code (or JSpecify annitations are useful for working on the code at a later point of time. To avoid regressions.

Side track: Another tool for avoiding regressions is OpenFastTrace - see https://devdocs.jabref.org/requirements/ for details ^^.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is to ensure the precondition, it is better to revert this change and use the dedicated error dialog instead of using an internal assertion. Will be updated in latest commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks for empty database and databasecontext merged in (updated) lines 509-515.

OOError.noDataBaseIsOpenForCiting()
.setTitle(errorDialogTitle)
.showErrorDialog(dialogService);
return;
}

List<BibEntry> entries = stateManager.getSelectedEntries();
if (entries.isEmpty()) {
Expand Down Expand Up @@ -553,12 +539,12 @@ private void pushEntries(CitationType citationType, boolean addPageInfo) {
}

Optional<Update.SyncOptions> syncOptions =
preferences.getOpenOfficePreferences().getSyncWhenCiting()
openOfficePreferences.getSyncWhenCiting()
? Optional.of(new Update.SyncOptions(getBaseList()))
: Optional.empty();

// Sync options are non-null only when "Automatically sync bibliography when inserting citations" is enabled
if (syncOptions.isPresent() && preferences.getOpenOfficePreferences().getSyncWhenCiting()) {
if (syncOptions.isPresent() && openOfficePreferences.getSyncWhenCiting()) {
syncOptions.get().setUpdateBibliography(true);
}
ooBase.guiActionInsertEntry(entries,
Expand Down Expand Up @@ -601,12 +587,12 @@ private boolean checkThatEntriesHaveKeys(List<BibEntry> entries) {
Optional<BibDatabaseContext> databaseContext = stateManager.getActiveDatabase();
if (citePressed && databaseContext.isPresent()) {
// Generate keys
CitationKeyPatternPreferences prefs = preferences.getCitationKeyPatternPreferences();
CitationKeyPatternPreferences citationKeyPatternPreferences = preferences.getCitationKeyPatternPreferences();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, you can have these passed in the constructor - and get rid of preferences parameter alltogheter? ^^

Copy link
Collaborator Author

@subhramit subhramit Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be the ideal level of abstraction, but preferences would be needed here:

image

Reason for moving openOfficePreferences in the constructor initialization block was that it is the core preference used all over the class (22 usages), whereas the rest of the preferences (ExternalApplicationsPreferences, LayoutFormatterPreferences, CitationKeyPatternPreferences) are used just used once in within their respective scopes (three times all together in total), thus declared locally.

Copy link
Collaborator Author

@subhramit subhramit Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koppor I tried to clean it up a bit further by passing all the preference subtypes via constructor. Although I couldn't get rid of preferences, I could minimize its usage to this one place (above comment). Check commit f759087. If this is not useful, we can revert the commit.

Advantage (imo) - more inversion of control (gets rid of all getters).
Ping: @calixtus

NamedCompound undoCompound = new NamedCompound(Localization.lang("Cite"));
for (BibEntry entry : entries) {
if (entry.getCitationKey().isEmpty()) {
// Generate key
new CitationKeyGenerator(databaseContext.get(), prefs)
new CitationKeyGenerator(databaseContext.get(), citationKeyPatternPreferences)
.generateAndSetKey(entry)
.ifPresent(change -> undoCompound.addEdit(new UndoableKeyChange(change)));
}
Expand Down
Loading