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

Fix implementation of move_to_trash() on Linux #44021

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

dakennedyd
Copy link
Contributor

Modifies move_to_trash() on Linux
Fixes #42840 OS move_to_trash() on Linux is not compliant with the Freedesktop specification

@akien-mga akien-mga changed the title Modifies move_to_trash() on Linux Fix implementation of move_to_trash() on Linux Dec 3, 2020
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Nice work! See my comments for details, otherwise the code looks fine. I haven't tested though.

Are there known use cases for gio trash and gvfs-trash not supported? I wonder if we still need the manual fallback case at all.

platform/linuxbsd/os_linuxbsd.cpp Show resolved Hide resolved
platform/linuxbsd/os_linuxbsd.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/os_linuxbsd.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/os_linuxbsd.cpp Show resolved Hide resolved
platform/linuxbsd/os_linuxbsd.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/os_linuxbsd.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/os_linuxbsd.cpp Outdated Show resolved Hide resolved
@dakennedyd
Copy link
Contributor Author

The gio/gvfs-trash functions seem to be part of the GNOME/GTK stack. I don't know if this commands work at all
on non GNOME systems, plus is nice to have a fallback.

@pouleyKetchoupp
Copy link
Contributor

I've made some tests on Ubuntu/Gnome and it works fine with the 3 methods.

The only difference is in the manual fallback, the .trashinfo files use %2F for / characters (due to how Godot's http_escape works) while gio and gvfs-trash use /. It doesn't seem to be a problem though, the file information is displayed correctly in the trash.

@akien-mga
Copy link
Member

akien-mga commented Dec 4, 2020

Are there known use cases for gio trash and gvfs-trash not supported? I wonder if we still need the manual fallback case at all.

The gio/gvfs-trash functions seem to be part of the GNOME/GTK stack. I don't know if this commands work at all
on non GNOME systems, plus is nice to have a fallback.

I do have gio here on KDE Plasma, which is part of glib2.0, which is a common dependency for a lot of software regardless of the DE, so that's a fairly safe option.

That being said, KDE does have its own "io" client kioclient5:

  kioclient5 move 'src' 'dest'
            # Moves the URL 'src' to 'dest'.
            #   'src' may be a list of URLs.

            #   'dest' may be "trash:/" to move the files
            #   to the trash.

            #   the short version kioclient5 mv
            #   is also available.

Tested locally with:

kioclient5 move icon.png trash:/

And it seems to work fine.

I think KDE4 had kioclient (without 5), but I don't think we need to bother with KDE4 support anymore.
That being said the same holds true for gvfs-trash which seems to be deprecated now.

@akien-mga
Copy link
Member

Could you add support for kioclient5 as suggested above? Then it should be good to merge.

I'd suggest this order:

  • gio (glib)
  • kioclient5 (KDE)
  • gfvs-trash (Debian/Ubuntu-specific, deprecated)

Fixes godotengine#42840 OS move_to_trash() on Linux is not compliant with the Freedesktop specification
@dakennedyd
Copy link
Contributor Author

dakennedyd commented Dec 7, 2020

I did the changes you requested.

Sorry for the delay.

@akien-mga akien-mga merged commit efc2104 into godotengine:master Dec 7, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OS move_to_trash() on Linux is not compliant with the Freedesktop specification
5 participants