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

[Linux] Fix opening files in external apps #1954

Merged
merged 5 commits into from Jan 17, 2024

Conversation

Scrumplex
Copy link
Member

@Scrumplex Scrumplex commented Dec 14, 2023

Closes #1490

How to test

  1. Open a folder path
  2. Ensure file manager opens
  3. Open a file (edit a component in an instance)
  4. Ensure that a text editor opens

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@Scrumplex Scrumplex added this to the 8.1 milestone Dec 14, 2023
@Scrumplex Scrumplex added bug Something isn't working needs-testing PRs that should be tested a bit more before being merged labels Dec 14, 2023
Copy link
Member

@Trial97 Trial97 left a comment

Choose a reason for hiding this comment

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

Please consider removing IndirectOpen and isSandbox as they are not used anywhere with your current changes.
The code looks good. This still needs testing as I was never able to reproduce the actual issue.

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@Scrumplex Scrumplex added backport release-8.x Backport PR automatically changelog:fixed A PR that appears under "Fixed" in the changelog labels Dec 16, 2023
@TheKodeToad
Copy link
Member

Why the removal of IndirectOpen? What reason did it exist for - did it fix anything - and will removing it make more bugs present? Or was there something which stopped it working properly which can be changed?

@Scrumplex
Copy link
Member Author

Why the removal of IndirectOpen? What reason did it exist for - did it fix anything - and will removing it make more bugs present? Or was there something which stopped it working properly which can be changed?

I am not quite sure myself. But I wouldn't be surprised if MultiMC's issues stemmed from its ancient Qt version.

@develcooking
Copy link

Just in advance:
I'm not involved in the Prism Launcher, Multi MC or any of that. I also didn't read most of the code. I just want to open my folder from Prism. So I'm trying by searching and gifting possible answers for @TheKodeToad 's questions

What reason did it exist for - did it fix anything?

did it fix anything?

Yes think so. It fixed the self reported bug at issue 1389 LD_LIBRARY_PATH set for MultiMC leaks to other processes](MultiMC/Launcher#1389)

and will removing it make more bugs present? Or was there something which stopped it working properly which can be changed?

I didn't find any while testing it see last uploaded video at issue #1490

On my end @Scrumplex branch works

I hope this could help at leased a little bit

@TheKodeToad
Copy link
Member

Sorry! I probably could have figured this out if I put in more effort. I assumed Scrumplex knew though.

Copy link
Member

@TheKodeToad TheKodeToad left a comment

Choose a reason for hiding this comment

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

I am pretty sure this will reintroduce the bugs which the IndirectOpen impl was meant to fix :/

launcher/DesktopServices.h Outdated Show resolved Hide resolved
@Scrumplex
Copy link
Member Author

I am pretty sure this will reintroduce the bugs which the IndirectOpen impl was meant to fix :/

But no other Qt app I know uses IndirectOpen or something similar. This really seems like a legacy workaround that is not relevant nowadays

Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@TheKodeToad
Copy link
Member

I will approve if you fix the thing I mentioned - or you do not see it to be necessary!

@Scrumplex
Copy link
Member Author

I will approve if you fix the thing I mentioned - or you do not see it to be necessary!

I was working on it ^^

@shellheim
Copy link

I will approve if you fix the thing I mentioned - or you do not see it to be necessary!

It still doesn't work in flatpak though :(

@Scrumplex
Copy link
Member Author

It still doesn't work in flatpak though :(

Do you have any other Qt (5) apps in Flatpak to compare?

@shellheim
Copy link

There are many qt5 apps which can open files but I don't have any that needs to open folders.

@getchoo getchoo self-requested a review January 11, 2024 08:22
Signed-off-by: Sefa Eyeoglu <contact@scrumplex.net>
@Trial97
Copy link
Member

Trial97 commented Jan 15, 2024

Maybe related to: #993

@Scrumplex Scrumplex merged commit 0b3e91a into PrismLauncher:develop Jan 17, 2024
31 checks passed
@Scrumplex Scrumplex deleted the fix/open-paths-immediately branch January 17, 2024 14:18
Copy link
Contributor

Successfully created backport PR for release-8.x:

This was referenced Jan 17, 2024
@Scrumplex Scrumplex changed the title Open paths directly [Linux] Fix opening files in external apps Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-8.x Backport PR automatically bug Something isn't working changelog:fixed A PR that appears under "Fixed" in the changelog needs-testing PRs that should be tested a bit more before being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Open Instance folder" button does not work
5 participants