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

FileBrowser: "Open containing folder" automatically selects file in the OS's file manager #7700

Merged
merged 147 commits into from
Mar 2, 2025

Conversation

AW1534
Copy link
Contributor

@AW1534 AW1534 commented Feb 10, 2025

Introduces a new helper class FileRevealer to manage file selection across different platforms (Windows, macOS, Linux, and other *nix operating systems with xdg). Includes methods to open and select files or directories using the default file manager on the respective platform.

User-Facing Changes:

  • Replace "open containing folder" in the file context menu with "Show in [Explorer/Finder/file manager]" respectively for different Operating Systems. This not only opens the containing folder, but highlights the file if supported by the File Manager.
  • Adds "Open in [Explorer/Finder/file manager]" option to Folder context menu.

Other Changes:

  • Added FileRevealer.h and FileRevealer.cpp to define and implement the file selecting functionality.
  • Modified FileBrowser.cpp to integrate FileManagerServices for opening containing folders and directories.
  • FileRevealer::canSelect method to check if file selection is possible.
  • FileRevealer::select method to select files or directories using platform-specific commands.
  • Handling of different file managers on Linux by checking support for the --select option.

@AW1534

This comment was marked as outdated.

@AW1534 AW1534 marked this pull request as ready for review February 10, 2025 02:48
@AW1534 AW1534 marked this pull request as draft February 10, 2025 20:01
@AW1534 AW1534 marked this pull request as ready for review February 10, 2025 20:49
@sakertooth
Copy link
Contributor

Sticking to saying "Open containing folder" regardless of OS and opening it using the native file manager (if any) is probably the way to go IMO. It's a lot simpler that way at the cost of little.

Also, I'm confused on what this has to do with highlighting files in the FileBrowser? It seems like if anything, the goal is to highlight files in the file manager on the OS once "Open containing folder" is pressed (which IMO isn't a requirement if it means not having to deal with all the OS-specific file managers)

Additionally, I think we should be using something like QDesktopServices::openUrl and not worrying about all the OS-specific file managers and whatnot.

@AW1534
Copy link
Contributor Author

AW1534 commented Feb 14, 2025

Sticking to saying "Open containing folder" regardless of OS and opening it using the native file manager (if any) is probably the way to go IMO. It's a lot simpler that way at the cost of little.

Unless you have other concerns, the only downside is maintainability but that should've been fixed in the latest commit.

Also, I'm confused on what this has to do with highlighting files in the FileBrowser? It seems like if anything, the goal is to highlight files in the file manager on the OS once "Open containing folder" is pressed (which IMO isn't a requirement if it means not having to deal with all the OS-specific file managers)

You're right, this PR is badly titled.

Additionally, I think we should be using something like QDesktopServices::openUrl and not worrying about all the OS-specific file managers and whatnot.

I really do dislike using openUrl. It was really slow for me (like ~5 second wait time) when I was on linux, But almost instant with my PR.

@AW1534 AW1534 changed the title Highlight files in FileBrowser highlight files in the file manager on the OS Feb 14, 2025
@tresf
Copy link
Member

tresf commented Feb 14, 2025

Sticking to saying "Open containing folder" regardless of OS and opening it using the native file manager (if any) is probably the way to go IMO. It's a lot simpler that way at the cost of little.

@sakertooth Many applications have shims that do as the OP is suggesting. I've noticed that this trend has become an unofficial standard over the years for polished software. For example, my IDEs tend to do this same thing and when reading the source code, the techniques are often identical to the OP's. The larger the folder contents, the more useful this feature can become as the file manager will navigate directly to the file that was requested. This is a very well received feature IMO.

@tresf
Copy link
Member

tresf commented Feb 14, 2025

Additionally, I think we should be using something like QDesktopServices::openUrl and not worrying about all the OS-specific file managers and whatnot.

I really do dislike using openUrl. It was really slow for me (like ~5 second wait time) when I was on linux, But almost instant with my PR.

I haven't benchmarked openUrl, but I wholeheartedly agree. A pragmatic solution would leverage the underlying C++ APIs, but Linux lacks a consistently shared framework to leverage something like this and Windows + macOS have the support build into the CLI of their file managers, so this really is the simplest approach for 99% of users. For those 1% of users (e.g. Haiku) we'll need to simply fallback to Qt's inferior handling.

@tresf
Copy link
Member

tresf commented Feb 14, 2025

@sakertooth @AW1534 how much sense would it be to subclass QDesktopServices into our own implementation and take this logic right out of FileBrowser? This would allow us to eventually leverage the same logic in other/future areas of the codebase and help keep FileBrowser clean of our own implementation.

The code also reads more portable anyway since it uses non-LMMS precompiler definitions and focuses on standard ones (e.g. _WIN32, __APPLE__) which smells a lot like a general good utility class that other projects can copy/pasta.

Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

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

Unless you have other concerns, the only downside is maintainability but that should've been fixed in the latest commit.

Just took another peek at the code and it's a lot better. In particular, we aren't making so many references to platform-specific applications like Thunar, Nautilus, Dolphin, etc. The use of xdg-mime and xdg-open is a nice solution.

Code-wise, I think we can apply some formatting to the new changes, and I think some simplifications can be made. I can help out a bit if you will allow it.

@sakertooth
Copy link
Contributor

sakertooth commented Feb 14, 2025

@sakertooth @AW1534 how much sense would it be to subclass QDesktopServices into our own implementation and take this logic right out of FileBrowser? This would allow us to eventually leverage the same logic in other/future areas of the codebase and help keep FileBrowser clean of our own implementation.

The code also reads more portable anyway since it uses non-LMMS precompiler definitions and focuses on standard ones (e.g. _WIN32, APPLE) which smells a lot like a general good utility class that other projects can copy/pasta.

It seems like we merely fallback to using QDesktopServices, so there shouldn't be a need to extend from it. However, I do like the idea of putting all of the implementation specific code in one tidy place, which then FileBrowser can reference.

@AW1534
Copy link
Contributor Author

AW1534 commented Feb 14, 2025

@sakertooth @AW1534 how much sense would it be to subclass QDesktopServices into our own implementation and take this logic right out of FileBrowser? This would allow us to eventually leverage the same logic in other/future areas of the codebase and help keep FileBrowser clean of our own implementation.

The code also reads more portable anyway since it uses non-LMMS precompiler definitions and focuses on standard ones (e.g. _WIN32, __APPLE__) which smells a lot like a general good utility class that other projects can copy/pasta.

This is a great idea which i'm willing to do, but as @sakertooth said, it probably doesnt need to extend from QDesktopServices

Co-authored-by: Tres Finocchiaro <[email protected]>
@AW1534
Copy link
Contributor Author

AW1534 commented Feb 14, 2025

Code-wise, I think we can apply some formatting to the new changes, and I think some simplifications can be made. I can help out a bit if you will allow it.

absolutely, I'll apply any suggestions you give me

@bratpeki
Copy link
Member

Edit, nevermind, it's definitely broken.

xdg-mime query default inode/directory

In the meantime, I can test this on a local copy of Manjaro as well.

[peki@peki ~]$ xdg-mime query default inode/directory
org.xfce.Catfish.desktop

Yikes!

Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

Some style fixes.

When testing it again on Linux Mint with Nemo, using "show in file manager" on a file opens two Nemo windows - one is the directory the lmms executable is in and the other is the directory with the file in it. Using "Open in file manager" on a directory does not have this problem.

@bratpeki
Copy link
Member

Trying now. Still opens Catfish. 🥲

@bratpeki
Copy link
Member

I noticed this on the output:

FileRevealer: Default app for inode/directory: "catfish"

@tresf
Copy link
Member

tresf commented Feb 28, 2025

I noticed this on the output:

FileRevealer: Default app for inode/directory: "catfish"

Weird it should show exo-open. What's the value of echo $XDG_CURRENT_DESKTOP?

@bratpeki
Copy link
Member

2025-02-28-101004_420x87_scrot

Isn't that something! 😆

I'm currently in DWM, I'll switch over to the XFCE desktop and report back!

@bratpeki
Copy link
Member

On the XFCE DE, it displays "XFCE". This is an issue of my DWM instance not being set up properly, then! Sorry for the confusion! I'll try it now.

@bratpeki
Copy link
Member

Everything works great! 👍

Interestingly, LMMS samples and presets are loaded to /tmp/.mount_lmms-1EMMHng/usr/share/lmms/.... It's not related to this PR; just an observation! 🙂

@bratpeki
Copy link
Member

After running env XDG_CURRENT_DESKTOP=XFCE ./Downloads/lmms-1.3.0-alpha.1.914+pr7700.[...].AppImage on DWM, everything works well there as well!

For future documentation purposes, I've noted that this functionality depends on XDG_CURRENT_DESKTOP being properly set up.


What about this, though: In the case someone using XFCE uninstalls Thunar and installs, say, Dolphin or Nautilus, how will that affect this functionality?

@tresf
Copy link
Member

tresf commented Feb 28, 2025

2025-02-28-101004_420x87_scrot

Isn't that something! 😆

I'm currently in DWM, I'll switch over to the XFCE desktop and report back!

Can you try echo $XDG_CURRENT_SESSION next? That was @AW1534's first instinct and I didn't think it was needed.

@tresf
Copy link
Member

tresf commented Feb 28, 2025

Also, if xdg-open works correctly, we should be to as well.

@AW1534
Copy link
Contributor Author

AW1534 commented Feb 28, 2025

What about this, though: In the case someone using XFCE uninstalls Thunar and installs, say, Dolphin or Nautilus, how will that affect this functionality?

on XFCE It uses exo-open rather than trying to detect the file manager, which will open whatever the default file manager is.

So it should behave as expected and open whichever file manager the user has set as default

AW1534 and others added 6 commits February 28, 2025 17:20
@bratpeki
Copy link
Member

Tried playing around with this some more:

2025-02-28-223622_756x351_scrot

2025-02-28-223825_733x527_scrot

2025-02-28-223929_1458x692_scrot

Very odd! Will experiment some more 😁

@tresf tresf requested a review from messmerd February 28, 2025 22:41
Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

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

Works with Nemo now.

@tresf
Copy link
Member

tresf commented Mar 1, 2025

So the final hold-up is the peculiar behavior of @bratpeki's desktop with DWM installed.

On the XFCE DE, it displays "XFCE". This is an issue of my DWM instance not being set up properly, then! Sorry for the confusion! I'll try it now.

It's hard to know how the Desktop "should" be setup, but having an environment to reproduce would certainly help. I was always under the impression that the average Desktop was usually one of the major flavors, but this PR really has me rethinking that.

(Steps to get DWM installed on Manjaro, copied from Discord)

  1. Compile and install DWM
  2. Create a DESKTOP file and place it in the session folder, to tell XFCE there's a new session it can run on startup
  3. Log out
  4. Profit!
%> mimeopen .
Please choose a default application for files of type inode/directory

    1) Catfish File Search  (org.xfce.Catfish)
    2) Thunar File Manager  (thunar)
    3) Vifm (vifm)
    4) Other

^--- This is particularly worrisome to me for two reasons...

  1. So there's yet-another tool for mime associations 🤣
  2. Based on the output of this app mimeopen tool, somehow we're lucking out and doing what it expects?

But seriously what I think is a bit more unorthodox is the DWM doesn't even have a preferred file manager, so I feel like this is likely a configuration issue. The DWM official page offers recommended file managers, but does not list a default. DWM Q&A articles discussing setting the default file manager seem to dip back into this mess: #7700 (comment) (click to expand the table to see how complicated it can get).

So I think this is in good shape now unless we want to throw a few more DEs at it, but if so, I want to make sure that we don't go down another rabbit hole where the DE was misconfigured.

@AW1534
Copy link
Contributor Author

AW1534 commented Mar 2, 2025

So I think this is in good shape now unless we want to throw a few more DEs at it, but if so, I want to make sure that we don't go down another rabbit hole where the DE was misconfigured.

@tresf so does this mean we're ready to merge?

@tresf
Copy link
Member

tresf commented Mar 2, 2025

So I think this is in good shape now unless we want to throw a few more DEs at it, but if so, I want to make sure that we don't go down another rabbit hole where the DE was misconfigured.

@tresf so does this mean we're ready to merge?

Yes, I think so. Let's merge it and we can easily patch in other DEs as they're reported. The code is extremely scalable for adding more.

@tresf tresf merged commit 9159533 into LMMS:master Mar 2, 2025
10 checks passed
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.

7 participants