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(cli): desktop app now can be re-opened on Linux #3323

Closed

Conversation

Andrew15-5
Copy link
Contributor

@Andrew15-5 Andrew15-5 commented Dec 10, 2024

Fixes #3322.

I added some tracing logs just in case someone else also needs them, like I did when debugging this issue.

IDK why this is added and so soon in the build cycle, but perhaps this can be replaced by the approach from this PR, but someone needs to check if replacing works.

// Kill any running executables on Windows
if cfg!(windows) {
runner.kill_all();
}

@istudyatuni
Copy link

Another solution is just remove target file and then copy. It also seems simpler to implement

@Andrew15-5
Copy link
Contributor Author

Interesting.

pros:

  • only 2 cumulative operations in the code base, i.e., just need to add 1 operation to an existing one

cons:

  • not an atomic operation, unlike mv (in Unix)

It doesn't require additional space for the 3rd binary. Well actually, since some process is still using the file, this would mean that after rm & cp there will be 2 visual copies and 1 more copy which is used by a process.

So this one is debatable. It will still require 2 operations for the Linux platform. And the code wouldn't become tremendously smaller, so I think we should keep the atomic approach.

@jkelleyrtp
Copy link
Member

I implemented a similar but different system in #3608 to solve this issue not just for linux but for all platforms that have this restriction.

Will add your name to the contribution list so you get the badge :) thank you for the PR!

@jkelleyrtp jkelleyrtp closed this Jan 21, 2025
@Andrew15-5
Copy link
Contributor Author

Thanks. I've tested that and it does work. Though I saw that in your PR there is a place where you replace o key action with p instead of adding it, so I thought that maybe manual opening is removed altogether, but apparently not. Maybe you should look at it. Looks like a typo or a bug, or maybe I don't understand something.

@Andrew15-5 Andrew15-5 deleted the fix-desktop-open-linux branch January 21, 2025 22:07
@jkelleyrtp
Copy link
Member

jkelleyrtp commented Jan 21, 2025

Thanks. I've tested that and it does work. Though I saw that in your PR there is a place where you replace o key action info with p instead of adding it, so I thought that maybe manual opening is removed altogether, but apparently not. Maybe you should look at it. Looks like a typo or a bug, or maybe I don't understand something.

I moved the 'o' key to the / extra panel since we occasionally tell the user via a log they can use 'o' to re-open.

@Andrew15-5
Copy link
Contributor Author

Ohh, yeah, that makes sense.

@Andrew15-5
Copy link
Contributor Author

Will add your name to the contribution list so you get the badge :) thank you for the PR!

I ain't seeing no badge. ;-;

https://github.com/DioxusLabs/dioxus/releases/tag/v0.6.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Desktop app can't be automatically re-opened by the dx serve
3 participants