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

hide 7z/kindlegen consoles on Windows #656

Merged
merged 5 commits into from
Jan 2, 2024

Conversation

VampiroMedicado
Copy link
Contributor

@VampiroMedicado VampiroMedicado commented Dec 31, 2023

Hide the subprocess that runs kindlegen.exe when the program is performing the .mobi creation, to prevent the user from losing focus on what they are doing while a conversion is being done.

@axu2
Copy link
Collaborator

axu2 commented Dec 31, 2023

@VampiroMedicado D'oh! Sorry for not noticing this Windows-OS-only quirk.

This is probably due to me removing the stdin=PIPE in

- #633
- https://stackoverflow.com/questions/24455337/pyinstaller-on-windows-with-noconsole-simply-wont-work

You want to try putting it back? I notice a console window opens at EVERY point I call subprocess.run, including 7z, etc.

You can generate an exe with

python setup.py build_binary

@axu2 axu2 added the Windows Windows OS label Dec 31, 2023
@VampiroMedicado
Copy link
Contributor Author

VampiroMedicado commented Dec 31, 2023

@VampiroMedicado D'oh! Sorry for not noticing this Windows-only quirk.

This is probably due to me removing the stdin=PIPE in

You want to try putting it back? I notice a console window opens at EVERY point I call subprocess.run, including 7z, etc.

You can generate an exe with

python setup.py build_binary

Seems like the removal of Popen is intentional in PR #581.

What about using a utils function def run_silent_subprocess to handle this?

@axu2
Copy link
Collaborator

axu2 commented Dec 31, 2023

@VampiroMedicado

Subprocess.run and popen have the same args. Re-adding stdin=PIPE everywhere as an arg should work.

@axu2
Copy link
Collaborator

axu2 commented Jan 1, 2024

Actually, the proper way to do this is to use the

creationflags=CREATE_NO_WINDOW,

arg in subprocess.run

@VampiroMedicado
Copy link
Contributor Author

Actually, the proper way to do this is to use the

creationflags=CREATE_NO_WINDOW,

arg in subprocess.run

Yeah, it sounds better now.

I was checking the docs and found nothing about how stdin=PIPE might interact with Windows Shell, but I'm not near my computer to test it. 😅

I'll update the PR tomorrow.

@axu2
Copy link
Collaborator

axu2 commented Jan 1, 2024

I was checking the docs and found nothing about how stdin=PIPE might interact with Windows Shell, but I'm not near my computer to test it. 😅

I was basing this on the stackoverflow answer, but it seems to not be true.

Also, the CREATE_NO_WINDOW constant is Windows-OS only and throws an Import Error on mac.
https://docs.python.org/3/library/subprocess.html#windows-constants

ImportError: cannot import name 'CREATE_NO_WINDOW' from 'subprocess' (/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/subprocess.py)

What about using a utils function def run_silent_subprocess to handle this?

Sounds good, it just needs to check for Windows-OS or not.
It seems this issue stemmed from not using the unsafe shell=True...

@VampiroMedicado
Copy link
Contributor Author

VampiroMedicado commented Jan 1, 2024

I was checking the docs and found nothing about how stdin=PIPE might interact with Windows Shell, but I'm not near my computer to test it. 😅

I was basing this on the stackoverflow answer, but it seems to not be true.

Also, the CREATE_NO_WINDOW constant is Windows only and throws an Import Error on mac.
https://docs.python.org/3/library/subprocess.html#windows-constants

ImportError: cannot import name 'CREATE_NO_WINDOW' from 'subprocess' (/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/subprocess.py)

What about using a utils function def run_silent_subprocess to handle this?

Sounds good, it just needs to check for windows or not.
It seems this issue stemmed from not using the unsafe shell=True...

The current solution works right on Mac? I only have a Windows desktop.

@axu2
Copy link
Collaborator

axu2 commented Jan 1, 2024

Yes, Mac and Linux don't create extra console windows already. This is Windows OS only issue. Just check the os before importing.

@VampiroMedicado
Copy link
Contributor Author

Hi, @axu2 happy new year 🎉

I made some changes, utils.py has the helper function and replaced any ocurrence of subprocess.run with it.

Copy link
Collaborator

@axu2 axu2 left a comment

Choose a reason for hiding this comment

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

A few comments, I can confirm it runs on macOS. Thanks for the great work!

kindlecomicconverter/utils.py Outdated Show resolved Hide resolved
kindlecomicconverter/utils.py Outdated Show resolved Hide resolved
@VampiroMedicado
Copy link
Contributor Author

@axu2 changes done.

Tested on Windows without issues.

@axu2 axu2 self-assigned this Jan 1, 2024
Copy link
Collaborator

@axu2 axu2 left a comment

Choose a reason for hiding this comment

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

Looks great, I'll include these fixes in the next release!

Repository owner deleted a comment from VampiroMedicado Jan 1, 2024
@axu2 axu2 changed the title Add startupinfo to hide subprocess window hide subprocess consoles on Windows Jan 1, 2024
@axu2 axu2 merged commit e8502f0 into ciromattia:master Jan 2, 2024
@axu2 axu2 changed the title hide subprocess consoles on Windows hide 7z/kindlegen consoles on Windows Feb 6, 2024
@axu2
Copy link
Collaborator

axu2 commented Feb 8, 2024

I've decided to backport this fix to v5.6.5. V5.7.0 is in prerelease and gets fewer downloads than v5.6.5

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

Successfully merging this pull request may close these issues.

7zip window popup that steals focus
2 participants