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

Show file tree
Hide file tree
Changes from all 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
75 changes: 36 additions & 39 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 @@ -34,6 +34,7 @@
import org.jabref.gui.StateManager;
import org.jabref.gui.actions.ActionFactory;
import org.jabref.gui.actions.StandardActions;
import org.jabref.gui.frame.ExternalApplicationsPreferences;
import org.jabref.gui.help.HelpAction;
import org.jabref.gui.icon.IconTheme;
import org.jabref.gui.preferences.GuiPreferences;
Expand All @@ -47,6 +48,7 @@
import org.jabref.logic.help.HelpFile;
import org.jabref.logic.journals.JournalAbbreviationRepository;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.layout.LayoutFormatterPreferences;
import org.jabref.logic.openoffice.OpenOfficeFileSearch;
import org.jabref.logic.openoffice.OpenOfficePreferences;
import org.jabref.logic.openoffice.action.Update;
Expand Down Expand Up @@ -95,6 +97,8 @@ public class OpenOfficePanel {
private final VBox vbox = new VBox();

private final GuiPreferences preferences;
private final OpenOfficePreferences openOfficePreferences;
private final CitationKeyPatternPreferences citationKeyPatternPreferences;
private final StateManager stateManager;
private final ClipBoardManager clipBoardManager;
private final UndoManager undoManager;
Expand All @@ -103,14 +107,17 @@ public class OpenOfficePanel {
private final LibraryTabContainer tabContainer;
private final FileUpdateMonitor fileUpdateMonitor;
private final BibEntryTypesManager entryTypesManager;
private final OpenOfficePreferences openOfficePreferences;
private OOBibBase ooBase;
private OOStyle currentStyle;

private final SimpleObjectProperty<OOStyle> currentStyleProperty;

public OpenOfficePanel(LibraryTabContainer tabContainer,
GuiPreferences preferences,
OpenOfficePreferences openOfficePreferences,
ExternalApplicationsPreferences externalApplicationsPreferences,
LayoutFormatterPreferences layoutFormatterPreferences,
CitationKeyPatternPreferences citationKeyPatternPreferences,
JournalAbbreviationRepository abbreviationRepository,
UiTaskExecutor taskExecutor,
DialogService dialogService,
Expand All @@ -123,13 +130,14 @@ public OpenOfficePanel(LibraryTabContainer tabContainer,
this.fileUpdateMonitor = fileUpdateMonitor;
this.entryTypesManager = entryTypesManager;
this.preferences = preferences;
this.openOfficePreferences = openOfficePreferences;
this.citationKeyPatternPreferences = citationKeyPatternPreferences;
this.taskExecutor = taskExecutor;
this.dialogService = dialogService;
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 All @@ -143,7 +151,7 @@ public OpenOfficePanel(LibraryTabContainer tabContainer,
manualConnect.setTooltip(new Tooltip(Localization.lang("Manual connect")));
manualConnect.setMaxWidth(Double.MAX_VALUE);

help = factory.createIconButton(StandardActions.HELP, new HelpAction(HelpFile.OPENOFFICE_LIBREOFFICE, dialogService, preferences.getExternalApplicationsPreferences()));
help = factory.createIconButton(StandardActions.HELP, new HelpAction(HelpFile.OPENOFFICE_LIBREOFFICE, dialogService, externalApplicationsPreferences));
help.setMaxWidth(Double.MAX_VALUE);

selectDocument = new Button();
Expand All @@ -157,8 +165,8 @@ public OpenOfficePanel(LibraryTabContainer tabContainer,
update.setMaxWidth(Double.MAX_VALUE);

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

currentStyleProperty = new SimpleObjectProperty<>(currentStyle);
Expand Down Expand Up @@ -327,7 +335,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 +349,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 +383,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 @@ -394,30 +402,24 @@ private void connectManually() {
}

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;
boolean isConnectedToDocument = (ooBase != null && !ooBase.isDocumentConnectionMissing());
boolean hasStyle = (currentStyle != null);
boolean hasDatabase = !getBaseList().isEmpty();
boolean canCite = isConnectedToDocument && hasStyle && hasDatabase;
boolean canRefreshDocument = isConnectedToDocument && hasStyle;
boolean cslStyleSelected = currentStyle instanceof CitationStyle;

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

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

boolean canRefreshDocument = isConnectedToDocument && hasStyle;

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

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

Expand All @@ -427,7 +429,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 +453,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 @@ -502,23 +504,19 @@ private static CitationType citationTypeFromOptions(boolean withText, boolean in
private void pushEntries(CitationType citationType, boolean addPageInfo) {
final String errorDialogTitle = Localization.lang("Error pushing entries");

if (stateManager.getActiveDatabase().isEmpty()
|| (stateManager.getActiveDatabase().get().getDatabase() == null)) {
OOError.noDataBaseIsOpenForCiting()
.setTitle(errorDialogTitle)
.showErrorDialog(dialogService);
return;
}
final Optional<BibDatabaseContext> activeDatabase = stateManager.getActiveDatabase();

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.

if (activeDatabase.isEmpty() || (activeDatabase.get().getDatabase() == null)) {
OOError.noDataBaseIsOpenForCiting()
.setTitle(errorDialogTitle)
.showErrorDialog(dialogService);
return;
}

final BibDatabaseContext bibDatabaseContext = activeDatabase.get();

List<BibEntry> entries = stateManager.getSelectedEntries();

if (entries.isEmpty()) {
OOError.noEntriesSelectedForCitation()
.setTitle(errorDialogTitle)
Expand Down Expand Up @@ -553,12 +551,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 +599,11 @@ private boolean checkThatEntriesHaveKeys(List<BibEntry> entries) {
Optional<BibDatabaseContext> databaseContext = stateManager.getActiveDatabase();
if (citePressed && databaseContext.isPresent()) {
// Generate keys
CitationKeyPatternPreferences prefs = preferences.getCitationKeyPatternPreferences();
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
4 changes: 4 additions & 0 deletions src/main/java/org/jabref/gui/sidepane/SidePaneContentFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ public Node create(SidePaneType sidePaneType) {
case OPEN_OFFICE -> new OpenOfficePanel(
tabContainer,
preferences,
preferences.getOpenOfficePreferences(),
preferences.getExternalApplicationsPreferences(),
preferences.getLayoutFormatterPreferences(),
preferences.getCitationKeyPatternPreferences(),
abbreviationRepository,
(UiTaskExecutor) taskExecutor,
dialogService,
Expand Down
Loading