Skip to content

Feature/flatpak update portal #117

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.

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

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
8 changes: 7 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,10 @@

<!-- runtime dependencies -->

<api.version>1.6.0</api.version>
<api.version>1.7.0-SNAPSHOT</api.version>
<secret-service.version>2.0.1-alpha</secret-service.version>
<kdewallet.version>1.4.0</kdewallet.version>
<flatpakupdateportal.version>1.0.0</flatpakupdateportal.version>
<slf4j.version>2.0.17</slf4j.version>
<appindicator.version>1.4.2</appindicator.version>

Expand Down Expand Up @@ -88,6 +89,11 @@
<artifactId>libappindicator-gtk3-java-minimal</artifactId>
<version>${appindicator.version}</version>
</dependency>
<dependency>
<groupId>org.purejava</groupId>
<artifactId>flatpak-update-portal</artifactId>
<version>${flatpakupdateportal.version}</version>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/module-info.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,23 @@
import org.cryptomator.integrations.quickaccess.QuickAccessService;
import org.cryptomator.integrations.revealpath.RevealPathService;
import org.cryptomator.integrations.tray.TrayMenuController;
import org.cryptomator.integrations.update.UpdateService;
import org.cryptomator.linux.autostart.FreedesktopAutoStartService;
import org.cryptomator.linux.keychain.KDEWalletKeychainAccess;
import org.cryptomator.linux.keychain.GnomeKeyringKeychainAccess;
import org.cryptomator.linux.keychain.KDEWalletKeychainAccess;
import org.cryptomator.linux.quickaccess.DolphinPlaces;
import org.cryptomator.linux.quickaccess.NautilusBookmarks;
import org.cryptomator.linux.revealpath.DBusSendRevealPathService;
import org.cryptomator.linux.tray.AppindicatorTrayMenuController;
import org.cryptomator.linux.update.FlatpakUpdater;

module org.cryptomator.integrations.linux {
requires org.cryptomator.integrations.api;
requires org.slf4j;
requires org.freedesktop.dbus;
requires org.purejava.appindicator;
requires org.purejava.kwallet;
requires org.purejava.portal;
requires de.swiesend.secretservice;
requires java.xml;

Expand All @@ -25,8 +28,10 @@
provides RevealPathService with DBusSendRevealPathService;
provides TrayMenuController with AppindicatorTrayMenuController;
provides QuickAccessService with NautilusBookmarks, DolphinPlaces;
provides UpdateService with FlatpakUpdater;

opens org.cryptomator.linux.tray to org.cryptomator.integrations.api;
opens org.cryptomator.linux.quickaccess to org.cryptomator.integrations.api;
opens org.cryptomator.linux.autostart to org.cryptomator.integrations.api;
opens org.cryptomator.linux.update to org.cryptomator.integrations.api;
}
140 changes: 140 additions & 0 deletions src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
package org.cryptomator.linux.update;

import org.cryptomator.integrations.common.CheckAvailability;
import org.cryptomator.integrations.common.DisplayName;
import org.cryptomator.integrations.common.OperatingSystem;
import org.cryptomator.integrations.common.Priority;
import org.cryptomator.integrations.update.Progress;
import org.cryptomator.integrations.update.ProgressListener;
import org.cryptomator.integrations.update.UpdateFailedException;
import org.cryptomator.integrations.update.UpdateService;
import org.freedesktop.dbus.FileDescriptor;
import org.freedesktop.dbus.exceptions.DBusException;
import org.freedesktop.dbus.types.UInt32;
import org.freedesktop.dbus.types.Variant;
import org.purejava.portal.Flatpak;
import org.purejava.portal.FlatpakSpawnFlag;
import org.purejava.portal.UpdatePortal;
import org.purejava.portal.Util;
import org.purejava.portal.rest.UpdateCheckerTask;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.concurrent.CopyOnWriteArrayList;

@Priority(1000)
@CheckAvailability
@DisplayName("Update via Flatpak update")
@OperatingSystem(OperatingSystem.Value.LINUX)
public class FlatpakUpdater implements UpdateService, AutoCloseable {

private static final Logger LOG = LoggerFactory.getLogger(FlatpakUpdater.class);
private static final String APP_NAME = "org.cryptomator.Cryptomator";

private final List<ProgressListener> progressListeners = new CopyOnWriteArrayList<>();

private final UpdatePortal portal;
private Flatpak.UpdateMonitor updateMonitor;

public FlatpakUpdater() {
this.portal = new UpdatePortal();
portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
}
Comment on lines +41 to +44
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove unnecessary monitor creation in constructor.

The constructor creates an update monitor but doesn't check or use the result. This monitor should be created lazily when first needed via getUpdateMonitor().

 public FlatpakUpdater() {
 	this.portal = new UpdatePortal();
-	portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public FlatpakUpdater() {
this.portal = new UpdatePortal();
portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
}
public FlatpakUpdater() {
this.portal = new UpdatePortal();
}
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java between lines
42 and 45, remove the call to
portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY) from the constructor
since the update monitor is created but never used there. Instead, implement
lazy initialization of the update monitor inside the getUpdateMonitor() method
so that the monitor is only created when first accessed.


@Override
public boolean isSupported() {
return portal.isAvailable();
}

@Override
public UpdateCheckerTask getLatestReleaseChecker() {
portal.setUpdateCheckerTaskFor(APP_NAME);
return portal.getUpdateCheckerTaskFor(APP_NAME);
}

@Override
public void triggerUpdate() throws UpdateFailedException {
var monitor = getUpdateMonitor();
portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid hardcoding the display parameter.

The hardcoded "x11:0" display parameter may not work correctly in Wayland environments or other display configurations.

Consider obtaining the display parameter dynamically:

 @Override
 public void triggerUpdate() throws UpdateFailedException {
     var monitor = getUpdateMonitor();
-    portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
+    String display = System.getenv("DISPLAY");
+    if (display == null || display.isEmpty()) {
+        display = System.getenv("WAYLAND_DISPLAY");
+        if (display != null && !display.isEmpty()) {
+            display = "wayland:" + display;
+        } else {
+            display = "x11:0"; // fallback
+        }
+    }
+    portal.updateApp(display, monitor, UpdatePortal.OPTIONS_DUMMY);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void triggerUpdate() throws UpdateFailedException {
var monitor = getUpdateMonitor();
portal.updateApp("x11:0", monitor, UpdatePortal.OPTIONS_DUMMY);
}
@Override
public void triggerUpdate() throws UpdateFailedException {
var monitor = getUpdateMonitor();
String display = System.getenv("DISPLAY");
if (display == null || display.isEmpty()) {
display = System.getenv("WAYLAND_DISPLAY");
if (display != null && !display.isEmpty()) {
display = "wayland:" + display;
} else {
display = "x11:0"; // fallback
}
}
portal.updateApp(display, monitor, UpdatePortal.OPTIONS_DUMMY);
}
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
70 to 73, the display parameter "x11:0" is hardcoded in the portal.updateApp
call, which can cause issues in Wayland or other display environments. Modify
the code to dynamically obtain the display parameter from the environment or
system properties instead of using a fixed string. Replace the hardcoded "x11:0"
with a variable that retrieves the current display setting at runtime.


@Override
public long spawnApp() {
var cwdPath = Util.stringToByteList(System.getProperty("user.dir"));
List<List<Byte>> argv = List.of(
Util.stringToByteList(APP_NAME));
Map<UInt32, FileDescriptor> fds = Collections.emptyMap();
Map<String, String> envs = Map.of();
UInt32 flags = new UInt32(FlatpakSpawnFlag.LATEST_VERSION.getValue());
Map<String, Variant<?>> options = UpdatePortal.OPTIONS_DUMMY;

return spawnApp(cwdPath, argv, fds, envs, flags, options).longValue();
}

@Override
public boolean doesRequireElevatedPermissions() {
return false;
}

@Override
public void close() throws Exception {
try {
if (null != updateMonitor) {
portal.cancelUpdateMonitor(updateMonitor);
}
portal.close();
} catch (Exception e) {
LOG.error(e.toString(), e.getCause());
}
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve exception handling in close method.

The current implementation catches a generic Exception and the logging could be more informative.

 @Override
 public void close() throws Exception {
     try {
         if (null != updateMonitor) {
             portal.cancelUpdateMonitor(updateMonitor);
         }
         portal.close();
-    } catch (Exception e) {
-        LOG.error(e.toString(), e.getCause());
+    } catch (DBusException e) {
+        LOG.error("Failed to close DBus connection properly", e);
+        throw e;
+    } catch (Exception e) {
+        LOG.error("Unexpected error during resource cleanup", e);
+        throw e;
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Override
public void close() throws Exception {
try {
if (null != updateMonitor) {
portal.cancelUpdateMonitor(updateMonitor);
}
portal.close();
} catch (Exception e) {
LOG.error(e.toString(), e.getCause());
}
}
@Override
public void close() throws Exception {
try {
if (null != updateMonitor) {
portal.cancelUpdateMonitor(updateMonitor);
}
portal.close();
} catch (DBusException e) {
LOG.error("Failed to close DBus connection properly", e);
throw e;
} catch (Exception e) {
LOG.error("Unexpected error during resource cleanup", e);
throw e;
}
}
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/linux/update/FlatpakUpdater.java around lines
98 to 108, improve the exception handling in the close method by catching more
specific exceptions if possible, and enhance the logging to provide a clearer
and more informative message. Instead of logging e.toString() and e.getCause()
separately, log the full exception with its stack trace to capture complete
context for debugging.


private synchronized Flatpak.UpdateMonitor getUpdateMonitor() {
if (updateMonitor == null) {
var updateMonitorPath = portal.CreateUpdateMonitor(UpdatePortal.OPTIONS_DUMMY);
if (updateMonitorPath != null) {
LOG.debug("UpdateMonitor successful created at {}", updateMonitorPath);
updateMonitor = portal.getUpdateMonitor(updateMonitorPath.toString());
try {
portal.getDBusConnection().addSigHandler(Flatpak.UpdateMonitor.Progress.class, signal -> {
notifyOnUpdateProceeds(signal);
});
} catch (DBusException e) {
LOG.error(e.toString(), e.getCause());
}
} else {
LOG.error("Failed to create UpdateMonitor on DBus");
}
}
return updateMonitor;
}

@Override
public void addProgressListener(ProgressListener listener) {
progressListeners.add(listener);
}

@Override
public void removeProgressListener(ProgressListener listener) {
progressListeners.remove(listener);
}

private void notifyOnUpdateProceeds(Flatpak.UpdateMonitor.Progress signal) {
long status = ((UInt32) signal.info.get("status").getValue()).longValue();
long progress = 0;
Variant<?> progressVariant = signal.info.get("progress");
if (null != progressVariant) {
progress = ((UInt32) progressVariant.getValue()).longValue();
}
Progress p = new Progress(status, progress);
for (ProgressListener listener : progressListeners) {
listener.onProgress(p);
}
}

private UInt32 spawnApp(List<Byte> cwdPath, List<List<Byte>> argv, Map<UInt32, FileDescriptor> fds, Map<String, String> envs, UInt32 flags, Map<String, Variant<?>> options) {
return portal.Spawn(cwdPath, argv, fds, envs, flags, options);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.cryptomator.linux.update.FlatpakUpdater