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.

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

Add io.github.martchus.syncthingtray #5255

Closed
wants to merge 12 commits into from

Conversation

qgymib
Copy link

@qgymib qgymib commented May 17, 2024

Please confirm your submission meets all the criteria

  • Please describe your application briefly.
  • I have read the App Requirements and App Maintenance pages.
  • My pull request follows the instructions at App Submission.
  • I have built and tested the submission locally.
  • I am using only the minimal set of permissions. (If not, please explain each non-standard permission.)
  • All assets referenced in the manifest are redistributable by any party. If not, the unredistributable parts are using an extra-data source type.
  • I am an author/developer/upstream contributor of the project. If not, I contacted upstream developers about submitting their software to Flathub. Link: Flatpak for SyncthingTray Martchus/syncthingtray#261 (comment)
  • The domain used for the application ID is controlled by the application developers either directly or through the code hosting (e.g. GitHub, GitLab, SourceForge, etc.). The application id guidelines are followed.
  • Any additional patches or files have been submitted to the upstream projects concerned. (If not, explain why.)

Description

Syncthing Tray provides a tray icon and further platform integrations for Syncthing. Checkout the website for an overview.

Submit Permission

Martchus/syncthingtray#261

Please also add upstream app author @Martchus as maintainer.

About filesystem access permissions

The syncthing is a file synchronization program, in most case user need it to sync their files between different machines,so access to /home should be necessary.

@hfiguiere hfiguiere added awaiting-changes Pull request waiting for changes from author blocked Pull requests that are blocked on something or won't be accepted in the currrent state labels May 17, 2024
@qgymib
Copy link
Author

qgymib commented May 17, 2024

@hfiguiere Could you please help to review again? Also due to the application id is changed, should I send another PR?

Martchus added a commit to Martchus/cpp-utilities that referenced this pull request May 17, 2024
@bbhtt bbhtt removed the blocked Pull requests that are blocked on something or won't be accepted in the currrent state label May 18, 2024
@bbhtt
Copy link
Contributor

bbhtt commented May 18, 2024

Also due to the application id is changed, should I send another PR?

Nope, please keep it in this PR.

@bbhtt bbhtt changed the title Add org.martchus.syncthingtray Add io.github.martchus.syncthingtray May 18, 2024
@Martchus
Copy link

You can consider cherry-picking https://github.com/Martchus/syncthingtray/commit/1ca2eecbf1bab96dc9fddd21b50cb95c451b3b9f.patch and adding -DNO_AUTOSTART_SETTINGS:BOOL=ON to the CMake arguments. This disables the autostart settings which are not working anyway (so users won't create issues about it being broken).

qgymib and others added 3 commits May 19, 2024 20:46
Co-authored-by: bbhtt <bbhtt.zn0i8@slmail.me>
Co-authored-by: bbhtt <bbhtt.zn0i8@slmail.me>
@qgymib
Copy link
Author

qgymib commented May 19, 2024

You can consider cherry-picking https://github.com/Martchus/syncthingtray/commit/1ca2eecbf1bab96dc9fddd21b50cb95c451b3b9f.patch and adding -DNO_AUTOSTART_SETTINGS:BOOL=ON to the CMake arguments. This disables the autostart settings which are not working anyway (so users won't create issues about it being broken).

Merged, but the autostart configuration in setup wizard still exist.

@flathubbot
Copy link

Build 121967 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/104911/io.github.martchus.syncthingtray.flatpakref

@Martchus
Copy link

Martchus commented May 19, 2024

When installing this via the command mentioned by the bot I also got org.gtk.Gtk3theme.Breeze pulled into the set of dependencies to be installed. I'd just like to note that this shouldn't be required from my upstream perspective.


EDIT: I haven't found anything critical when testing the Flatpak. Just a few remarks:

It seems to work quite well besides the positioning issues under Wayland which have nothing to do with the Flatpak. As I had Syncthing already running outside of the Flatpak I only tested the case of connecting to an already running instance. The wizard was not able to detect the Syncthing config file out of the box (which usually works) but it was possible to point it to the correct location of the config file and proceed via the wizard without manual configuration.

It doesn't contain the built-in web view but I guess that's a good thing (because shipping a whole web engine is quite excessive just for this purpose; I also don't do it for Windows builds). Unfortunately the option to open the web view in a Chromium-based browser in app-mode doesn't seem to work. It always fails claiming that it cannot open "msedge" (the last binary it tries before giving up). Maybe it requires an additional permission to run certain binaries installed outside of Flatpak (or maybe it would help to install Chromium as Flatpak?).

It also looks like opening a folder in the local file browser doesn't work. Considering opening Syncthing in the users normal web browser works (and that uses the same underlying mechanism), maybe also just a permission is missing?

@hfiguiere
Copy link
Contributor

When installing this via the command mentioned by the bot I also got org.gtk.Gtk3theme.Breeze pulled into the set of dependencies to be installed

Are you sure it wasn't just updated?

@Martchus
Copy link

I haven't even had Flatpak itself installed at all on that machine. (Although it could be that some leftovers were still installed from ages ago.)

The exact log of the installation looked like this:

$ flatpak install --user https://dl.flathub.org/build-repo/104911/io.github.martchus.syncthingtray.flatpakref

Note that the directories 

'/var/lib/flatpak/exports/share'
'/home/martchus/.local/share/flatpak/exports/share'

are not in the search path set by the XDG_DATA_DIRS environment variable, so
applications installed by Flatpak may not appear on your desktop until the
session is restarted.

The application io.github.martchus.syncthingtray depends on runtimes from:
  https://dl.flathub.org/repo/
Configure this as new remote 'flathub' [Y/n]: 
Required runtime for io.github.martchus.syncthingtray/x86_64/test (runtime/org.kde.Platform/x86_64/6.7) found in remote flathub
Möchten Sie es installieren? [Y/n]: 

io.github.martchus.syncthingtray Berechtigungen:
    ipc    network    fallback-x11    wayland    x11    dri    file access [1]   dbus access [2]

    [1] home, xdg-config/kdeglobals:ro
    [2] com.canonical.AppMenu.Registrar, org.freedesktop.Notifications, org.kde.KGlobalSettings, org.kde.StatusNotifierWatcher, org.kde.kconfig.notify


        KENNUNG                                                 Zweig                     Op              Gegenstelle                       Herunterladen
 1. [✓] org.freedesktop.Platform.GL.default                     23.08                     i               flathub                           164,3 MB / 164,6 MB
 2. [✓] org.freedesktop.Platform.GL.default                     23.08-extra               i               flathub                            18,5 MB / 164,6 MB
 3. [✓] org.freedesktop.Platform.GL.nvidia-550-78               1.4                       i               flathub                           307,7 MB / 307,8 MB
 4. [✓] org.freedesktop.Platform.openh264                       2.2.0                     i               flathub                           886,7 KB / 944,3 KB
 5. [✓] org.gtk.Gtk3theme.Breeze                                3.22                      i               flathub                           114,4 KB / 192,0 KB
 6. [✓] org.kde.Platform.Locale                                 6.7                       i               flathub                            24,4 MB / 380,7 MB
 7. [✓] org.kde.Platform                                        6.7                       i               flathub                           258,7 MB / 326,6 MB
 8. [✓] io.github.martchus.syncthingtray                        test                      i               syncthingtray-origin               12,9 MB / 12,9 MB

@Martchus
Copy link

Merged, but the autostart configuration in setup wizard still exist.

@qgymib You're right. I have just pushed https://github.com/Martchus/syncthingtray/commit/9dcd1c268b38e9392b13201b1890758bf78e1477.patch to cover this as well. I have already tested it so feel free to cherry-pick it as well.

@qgymib
Copy link
Author

qgymib commented May 19, 2024

I have just pushed https://github.com/Martchus/syncthingtray/commit/9dcd1c268b38e9392b13201b1890758bf78e1477.patch to cover this as well.

Verified and merged.

@qgymib
Copy link
Author

qgymib commented May 19, 2024

The wizard was not able to detect the Syncthing config file out of the box

That was by design, the $XDG_CONFIG_HOME is set to $HOME/.var/app/$FLATPAK_ID/config, see Footnotes in Sandbox Permissions.

Unfortunately the option to open the web view in a Chromium-based browser in app-mode doesn't seem to work.

It was also by design because flatpak isolate host file access. Is that works by find executable in $PATH? In that case we cannot give read access to every directory in $PATH because pre-defined file access rule is static, and also might be too much open. I perfer user manually override file access rules by Flatseal or console.

It also looks like opening a folder in the local file browser doesn't work.

I didn't find the place to open a folder, where is it?

@Martchus
Copy link

Ok, I guess that makes sense. A folder can be opened by clicking on the folder icon of an entry within the folders tab.

@qgymib
Copy link
Author

qgymib commented May 19, 2024

A folder can be opened by clicking on the folder icon of an entry within the folders tab.

Sorry but none of these icons are click-able (Tested on windows and ubuntu, with official releases). Am I made any mistake?

Screenshot-2024-05-19 235040


Edit:

Oh I totally miss understanding your description. Opening a file picker dialog in Syncthing Tray's Settings window, and in the file picker dialog open a folder using system file manager works for me. Can you describe the exactly reproducible steps?

@Martchus
Copy link

I meant the icons on the right side (next to the status) in the Syncthing Tray UI (not the web UI).

@bbhtt
Copy link
Contributor

bbhtt commented May 19, 2024

Breeze is being installed as it is likely the desktop theme you use. You can ignore that one.

OpenURI is supposed to work with Qt 6 in Flatpak there were some issues with Qt5 but, they were resolved by Qt6. So you probably have to report that upstream.

@qgymib
Copy link
Author

qgymib commented May 19, 2024

I meant the icons on the right side (next to the status) in the Syncthing Tray UI (not the web UI).

@Martchus. Yeah I found it and the folder does open normally. What is your system & Desktop environment & Display server ? If they are easy to install I probably can test them in virtual machines.

OpenURI is supposed to work with Qt 6 in Flatpak there were some issues with Qt5 but, they were resolved by Qt6. So you probably have to report that upstream.

@bbhtt. I can try to help to reproduce that. BTW, could you please trigger another test build since there is a patch appended?

@bbhtt
Copy link
Contributor

bbhtt commented May 19, 2024

Feel free to start builds when needed.

bot, build io.github.martchus.syncthingtray

@flathubbot
Copy link

Queued test build for io.github.martchus.syncthingtray.

@flathubbot
Copy link

Started test build 122011

@flathubbot
Copy link

Build 122011 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/104955/io.github.martchus.syncthingtray.flatpakref

@Martchus
Copy link

Martchus commented May 19, 2024

@Martchus. Yeah I found it and the folder does open normally. What is your system & Desktop environment & Display server ? If they are easy to install I probably can test them in virtual machines.

@qgymib It was the latest KDE Plasma 6 using Wayland via KWin. However, I think the problem was simply due to:

The filesystem access is limited to /home.

So probably not worth investigating considering it generally works.


Breeze is being installed as it is likely the desktop theme you use. You can ignore that one.

@bbhtt But org.gtk.Gtk3theme.Breeze sounds like that package it would contain a GTK3 theme - and this application is using Qt 6 and really cannot make use of GTK3 themes. (Except maybe if displaying GTK-based dialogs via the GTK3-platform theme. Well, maybe that actually is the reason for pulling in that package. However, I thought Flatpak would use some portal magic instead anyway.)

@qgymib
Copy link
Author

qgymib commented May 20, 2024

But org.gtk.Gtk3theme.Breeze sounds like that package it would contain a GTK3 theme - and this application is using Qt 6 and really cannot make use of GTK3 themes.

@Martchus I test on a fresh ubuntu VM and install kalk which also depends on KDE runtime. It turns out also installed a GTK theme org.gtk.Gtk3theme.Yaru, so I assume it just a common dependency which related to current desktop theme.


At this point I guess is it safe to say everything is ready so we can be merged ?

@Martchus
Copy link

From my side, yes.

@qgymib
Copy link
Author

qgymib commented May 21, 2024

@bbhtt @hfiguiere Is there anything else we can do?

@bbhtt bbhtt added ready Pull request ready for final review and merge and removed awaiting-changes Pull request waiting for changes from author labels May 21, 2024
@barthalion
Copy link
Member

bot, build io.github.martchus.syncthingtray

@flathubbot
Copy link

Queued test build for io.github.martchus.syncthingtray.

@flathubbot
Copy link

Started test build 122404

@flathubbot
Copy link

Build 122404 successful
To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/105345/io.github.martchus.syncthingtray.flatpakref

@barthalion
Copy link
Member

/merge

@flathubbot
Copy link

A repository for this submission has been created: https://github.com/flathub/io.github.martchus.syncthingtray

You will receive an invitation to be a collaborator which will grant you write access to the repository above. The invite can be also viewed here.

If you've never maintained an app on Flathub before, common questions are answered in the app maintenance guide. If you're the original developer (or an authorized party), verify your app next to let users know it's coming from you.

Thanks!

@flathubbot flathubbot closed this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Pull request ready for final review and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants