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

gui_qt: Add open config folder action #981

Merged
merged 3 commits into from
Nov 18, 2018

Conversation

nsmarkop
Copy link
Contributor

@nsmarkop nsmarkop commented Jul 8, 2018

Fixes #697.

I added an entry for "Open config folder" to the File menu that opens up the configuration folder.

image

The icon used is from Icons8 Flat Color Icons.

Perhaps the oddest part here is the use of webbrowser.open to open the folder. os.startfile is Windows exclusive, so I searched around for cross platform ways to open folders and files and ended up on that standard library function accomplishing the desired behavior despite the name. Alternatively something like this could be implemented, but I figured if webbrowser.open works it's simpler to rely on it:

import subprocess, os
if sys.platform.startswith('darwin'):
    subprocess.call(('open', filepath))
elif os.name == 'nt':
    os.startfile(filepath)
elif os.name == 'posix':
    subprocess.call(('xdg-open', filepath))

I've only verified behavior on Windows, so Mac / Linux testing would be nice.

Copy link
Member

@benoit-pierre benoit-pierre left a comment

Choose a reason for hiding this comment

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

Seems to work fine on Linux.

@benoit-pierre
Copy link
Member

@morinted: can you check it works on macOS?

@TheaMorin
Copy link
Member

Yes -- will check in 36 hours when I have my Mac again.

@TheaMorin
Copy link
Member

On initial go, this doesn't do anything on macOS. I'll try the PyQT update and see if that makes a difference as I'm on the current macOS version (10.14)

@TheaMorin
Copy link
Member

After updating to the requirements in #1023, this still doesn't work :( might have to go with the OS-switch after all.

@nsmarkop
Copy link
Contributor Author

Darn. Since it doesn't seem to work for you on Mac, I updated the logic to be platform-specific instead. I only tested on Windows so I'll need verification again for Linux / Mac on this approach.

@TheaMorin
Copy link
Member

Works for me on Mac

@@ -238,6 +243,15 @@ def on_disable_output(self):
def on_configure(self):
self._configure()

def on_open_config_folder(self):
# webbrowser.open does not work on Mac so we need OS-specific logic
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove the mention, as testing it again on Linux with my configuration, I see that it always open a directory listing in my browser, which is clearly not the best behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in the latest commit.

if sys.platform.startswith('win'):
os.startfile(CONFIG_DIR)
elif sys.platform.startswith('linux'):
subprocess.run(['xdg-open', CONFIG_DIR])
Copy link
Member

Choose a reason for hiding this comment

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

For Python 3.4 compatibility:

Suggested change
subprocess.run(['xdg-open', CONFIG_DIR])
subprocess.call(['xdg-open', CONFIG_DIR])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this in the latest commit.

@benoit-pierre
Copy link
Member

Works fine on Linux.

@benoit-pierre
Copy link
Member

Looks like the Circle CI build is not run on the result of the merge, but directly the PR branch, unlike AppVeyor / Travis...

@benoit-pierre benoit-pierre merged commit bd1a2d0 into openstenoproject:master Nov 18, 2018
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.

Open the config folder from the GUI
3 participants