Fix menu bar gone on macOS #10967

Siedlerchr · 2024-03-03T22:08:50Z

was introduced in bec55f8

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.

Fixes #10966 was introduced in bec55f8

calixtus · 2024-03-03T22:34:16Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

@@ -2204,7 +2202,7 @@ public FilePreferences getFilePreferences() {
                // We choose the data directory, because a ".bak" file should survive cache cleanups
                getPath(BACKUP_DIRECTORY, OS.getNativeDesktop().getBackupDirectory()),
                getBoolean(CONFIRM_LINKED_FILE_DELETE),
-                getBoolean(TRASH_INSTEAD_OF_DELETE));
+                getBoolean(TRASH_INSTEAD_OF_DELETE, OS.getNativeDesktop().moveToTrashSupported()));


This is wrong. Can this be enabled in the preferences even though it is not supported?

Was already wrong before the chris change. 😉 isSupported is logic, not a preferences option.

jabref/src/main/java/org/jabref/gui/preferences/linkedfiles/LinkedFilesTab.java

Line 67 in cfabd9f

moveToTrash.setDisable(!OS.getNativeDesktop().moveToTrashSupported());

Nevertheless, the preference should not be true if it is not supported system-wide. I thought, in #10591 I implemented it as: "default = supported by OS" and then "disabled/enabled = supported by OS".

Maybe an additional line:

if (!OS.getNativeDesktop().moveToTrashSupported()) { moveToTrash.setValue(false) }

to ensure that it is both disabled and false if trash is not supported.

It was in defaults. but this broke the system menu bar in macOS. This is why i removed it in defaults and just added it as fallback value

Which part exactly broke the menu bar? The issue did not show any exception, so I cannot understand the root cause.

My suggestion would be just to make true the default, but it's greyed out in the preferences and where moveToTrash is used, there is an additional check, if this is supported. Check could go to JabRefDesktop#moveToTrash or sthg and then delete instead.

koppor · 2024-03-04T08:28:42Z

Failing tests are "only" architecture tests

org.jabref.architecture.MainArchitectureTest

  Test nativeDesktopIsRestricted FAILED

  java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes that do not have simple name 'JabRefDesktop' and do not have simple name 'Launcher' and do not have simple name 'DefaultDesktop' and do not have simple name 'OS' and do not have simple name 'Linux' and do not have simple name 'OSX' and do not have simple name 'Windows' and do not have simple name 'JabRefPreferences' and have name not matching '.*Test' should depend on classes that have simple name 'NativeDesktop'' was violated (5 times):
  Constructor <org.jabref.gui.preferences.externalfiletypes.EditExternalFileTypeEntryDialog.<init>(org.jabref.gui.preferences.externalfiletypes.ExternalFileTypeItemViewModel, java.lang.String, javafx.collections.ObservableList)> calls method <org.jabref.gui.desktop.os.NativeDesktop.getApplicationDirectory()> in (EditExternalFileTypeEntryDialog.java:37)
  Method <org.jabref.gui.linkedfile.DeleteFileAction.deleteFileHelper(org.jabref.model.database.BibDatabaseContext, org.jabref.model.entry.LinkedFile)> calls method <org.jabref.gui.desktop.os.NativeDesktop.moveToTrash(java.nio.file.Path)> in (DeleteFileAction.java:194)
  Method <org.jabref.gui.openoffice.DetectOpenOfficeInstallation.selectInstallationPath()> calls method <org.jabref.gui.desktop.os.NativeDesktop.getApplicationDirectory()> in (DetectOpenOfficeInstallation.java:37)
  Method <org.jabref.gui.preferences.linkedfiles.LinkedFilesTab.initialize()> calls method <org.jabref.gui.desktop.os.NativeDesktop.moveToTrashSupported()> in (LinkedFilesTab.java:67)
  Method <org.jabref.model.database.BibDatabaseContext.getFulltextIndexPath()> calls method <org.jabref.gui.desktop.os.NativeDesktop.getFulltextIndexBaseDirectory()> in (BibDatabaseContext.java:247)

koppor · 2024-03-04T08:29:42Z

Ah, I think org.jabref.gui.desktop.JabRefDesktop#moveToTrashSupported should be called. Not OS directly. We do have JabRefDesktop as bridge.

koppor

I don't know, how to solve this.

My thoughts:

Users should not be required to set any preference on first use.
JabRef should not delete data if it can do other way
Trashing files instead of deleting files is a way to prevent data loss.
Some operating systems do not support trashing
Thus, the default for trashing is the OS support for trashing.

Point 5 seemed to cause troubles with OSX. I don't know which troubles. @calixtus says, OS-depended defaults are not OK. I am curious about our ideas how to resolve points 1 to 4 above.

koppor · 2024-03-04T09:49:32Z

src/main/java/org/jabref/gui/desktop/JabRefDesktop.java

-    /**
-     * Moves the given file to the trash.
-     *
-     * @throws UnsupportedOperationException if the current platform does not support the {@link Desktop.Action#MOVE_TO_TRASH} action
-     * @see Desktop#moveToTrash(File)
-     */
-    public static void moveToTrash(Path path) {
-        NATIVE_DESKTOP.moveToTrash(path);
-    }
-
-    public static boolean moveToTrashSupported() {
-        return NATIVE_DESKTOP.moveToTrashSupported();
-    }
-
-    public static Path getApplicationDirectory() {
-        return NATIVE_DESKTOP.getApplicationDirectory();
-    }
-
-    public static Path getFulltextIndexBaseDirectory() {
-        return NATIVE_DESKTOP.getFulltextIndexBaseDirectory();
-    }
 }


Please keep the methods here. OS.getNativeDesktop() may only be used by certain classes. See org.jabref.architecture.MainArchitectureTest#nativeDesktopIsRestricted.

For me this is a code smell because we essentially have duplicate methods now. Before the mentioned PR we didn't have this architecture check

When introducing, I checked all calls to JabRef desktop and refactored out the implied ArchitectureCheck.

I like the FacadePattern :p

koppor · 2024-03-04T09:50:46Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

@@ -710,7 +709,6 @@ private JabRefPreferences() {
        defaults.put(WARN_BEFORE_OVERWRITING_KEY, Boolean.TRUE);
        defaults.put(CONFIRM_DELETE, Boolean.TRUE);
        defaults.put(CONFIRM_LINKED_FILE_DELETE, Boolean.TRUE);
-        defaults.put(TRASH_INSTEAD_OF_DELETE, JabRefDesktop.moveToTrashSupported());


Shouldn't there a true be put?

OK, what if the OS does not support move to trash?

This were my thoughts. Therefore, I made the default rely on the current OS's capabilities.

Exactly this line broke the menu bar! That's why I moved the check. I guess it has something to do with the initalization order of javafx <-> awt because we are here in a static context early at startup. That's why I moved the check down where the value is first read

koppor · 2024-03-04T09:51:28Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

@@ -2204,7 +2202,7 @@ public FilePreferences getFilePreferences() {
                // We choose the data directory, because a ".bak" file should survive cache cleanups
                getPath(BACKUP_DIRECTORY, OS.getNativeDesktop().getBackupDirectory()),
                getBoolean(CONFIRM_LINKED_FILE_DELETE),
-                getBoolean(TRASH_INSTEAD_OF_DELETE));
+                getBoolean(TRASH_INSTEAD_OF_DELETE, OS.getNativeDesktop().moveToTrashSupported()));


Which part exactly broke the menu bar? The issue did not show any exception, so I cannot understand the root cause.

github-actions · 2024-03-04T19:48:08Z

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

Fix menu bar gone on macOS

810b3b9

Fixes #10966 was introduced in bec55f8

Siedlerchr enabled auto-merge March 3, 2024 22:09

calixtus reviewed Mar 3, 2024

View reviewed changes

koppor requested changes Mar 4, 2024

View reviewed changes

koppor mentioned this pull request Mar 4, 2024

JabRef desktop to postpone logger creation #10975

Closed

6 tasks

Add comment

817817c

koppor force-pushed the fixOsxMenubar branch from cfabd9f to 817817c Compare March 4, 2024 19:37

koppor approved these changes Mar 4, 2024

View reviewed changes

Siedlerchr added this pull request to the merge queue Mar 4, 2024

Merged via the queue into main with commit f1c099c Mar 4, 2024
20 checks passed

Siedlerchr deleted the fixOsxMenubar branch March 4, 2024 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix menu bar gone on macOS #10967

Fix menu bar gone on macOS #10967

Siedlerchr commented Mar 3, 2024

calixtus Mar 3, 2024

calixtus Mar 3, 2024

Siedlerchr Mar 3, 2024

Siedlerchr Mar 3, 2024

koppor Mar 4, 2024

Siedlerchr Mar 4, 2024

koppor Mar 4, 2024

calixtus Mar 4, 2024

koppor commented Mar 4, 2024

koppor commented Mar 4, 2024

koppor left a comment

koppor Mar 4, 2024

Siedlerchr Mar 4, 2024

koppor Mar 4, 2024

koppor Mar 4, 2024

Siedlerchr Mar 4, 2024 •

edited

Loading

koppor Mar 4, 2024

github-actions bot commented Mar 4, 2024 •

edited

Loading

Fix menu bar gone on macOS #10967

Fix menu bar gone on macOS #10967

Conversation

Siedlerchr commented Mar 3, 2024

Mandatory checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Mar 4, 2024

koppor commented Mar 4, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 4, 2024 • edited Loading

Siedlerchr Mar 4, 2024 •

edited

Loading

github-actions bot commented Mar 4, 2024 •

edited

Loading