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

Going back to the menu #153

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Going back to the menu #153

wants to merge 4 commits into from

Conversation

Y0lan
Copy link

@Y0lan Y0lan commented Apr 14, 2020

added a "go back to menu entry" to all the entry where it made sense to add it. Now you can configure all the app without relaunching it.

I also cleaned up some things that I can easily revert back if needed.

  1. New Exit entry into main menu
  2. Type menu into the proton username to go back to menu
  3. New option to go back to the menu added to each list
  4. (optional but thought it would make sense) since Plus and Visionary have the same functionality, I grouped them together.

…e to add it. Now you can configure all the app without relaunching it.
@Y0lan Y0lan mentioned this pull request Apr 14, 2020
@Rafficer
Copy link
Owner

(optional but thought it would make sense) since Plus and Visionary have the same functionality, I grouped them together.

Please remove this. This is done intentionally to avoid confusion of users ("I'm on visionary, why doesn't it support this plan?"). Not everyone knows that they are the same. As you've seen, it sets the same value anyway, it's just to not cause confusion.

@Y0lan
Copy link
Author

Y0lan commented Apr 14, 2020

Sorry, I should not have changed this. :)

@Rafficer
Copy link
Owner

These changes mess up protonvpn init:

image

It's also not possible anymore to choose visionary as plan, it just throws you back to the menu. Same if you choose it during initialization:

image

@Y0lan
Copy link
Author

Y0lan commented Apr 16, 2020

I did not take into account the init option, good catch ! 😅
I've tested it now, and it should work.

I added an 'Init' boolean argument to the 3 functions run during initialisation to display or not, and enable / disable the menu option.

I should have thought of this before my pull request. I hope it's ok now ?

@Rafficer
Copy link
Owner

Instead of adding a new boolean, wouldn't it make more sense to utilize write=True, which already exists?

@Y0lan
Copy link
Author

Y0lan commented Apr 18, 2020

Instead of adding a new boolean, wouldn't it make more sense to utilize write=True, which already exists?

Good question. I don't think so since the relation between write and init mode isn't direct. It could be a hack tough, but adding a boolean and gaining in readability is a good compromise I think. But it's your software, so if you want me to remove it no problem.

@Rafficer
Copy link
Owner

Rafficer commented Apr 18, 2020

I'm actually thinking about whether it would be better to rename it to init, as that's actually what matters.

It just doesn't make sense to have 2 booleans that are always the same in the 2 different cases.

Let me submit a PR to your branch later. :)

Copy link
Owner

@Rafficer Rafficer left a comment

Choose a reason for hiding this comment

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

@Y0lan please take a look at these changes and tell me what you think!

Also, please pull them and try them out. Try to break them to find bugs :P

protonvpn_cli/cli.py Outdated Show resolved Hide resolved
protonvpn_cli/cli.py Show resolved Hide resolved
protonvpn_cli/cli.py Show resolved Hide resolved
protonvpn_cli/cli.py Outdated Show resolved Hide resolved
protonvpn_cli/cli.py Outdated Show resolved Hide resolved
Comment on lines 438 to 446
configure_cli()
return
Copy link
Owner

Choose a reason for hiding this comment

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

I've replaced all "configure_cli()" calls with return, as to not make the process go deeper and deeper.

protonvpn_cli/cli.py Outdated Show resolved Hide resolved
Copy link
Author

@Y0lan Y0lan left a comment

Choose a reason for hiding this comment

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

Perfect, thanks !

@Y0lan
Copy link
Author

Y0lan commented Apr 18, 2020

I'll test it tomorrow !

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.

2 participants