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

Use InquirerPy for user input #512

Merged
merged 4 commits into from
Sep 29, 2021
Merged

Use InquirerPy for user input #512

merged 4 commits into from
Sep 29, 2021

Conversation

sirloinofbeef
Copy link
Contributor

@sirloinofbeef sirloinofbeef commented Sep 27, 2021

Simplified user input using InquirerPy:

  • Server selection (selectable list)
    image

  • Managed user Y/N (made redundant by below)

  • Managed user selection (selectable list, including default main username)
    image

@glensc
Copy link
Collaborator

glensc commented Sep 27, 2021

I don't see the point in adding another dependency.

the password wasn't shown already with the current implementation.

@sirloinofbeef
Copy link
Contributor Author

sirloinofbeef commented Sep 27, 2021

Imho having * appear makes input easier / more intuitive.
The main bonus though comes with the selection input for servers / users.
Selective input makes it impossible for users to enter typos or incorrect data.
The managed user Y/N and selection inputs are also streamlined into a single input.

If this is not a direction wanted for the script though, that's fine, and / or I can look into coding something which doesn't require the dependency.

@glensc
Copy link
Collaborator

glensc commented Sep 27, 2021

having * appear only for windows users, unix users know that password input never shows anything. and you should add screenshots

@glensc
Copy link
Collaborator

glensc commented Sep 27, 2021

We already have rich library in dependencies:

@sirloinofbeef
Copy link
Contributor Author

Plex password input (Displays '*' for Windows only)
image

Plex server selection (use up + down arrows to select and press enter)
image

Plex user selection (use up + down arrows to select and press enter)
image

@sirloinofbeef
Copy link
Contributor Author

We already have rich library in dependencies:

Afaik rich is for RTF and doesn't offer the selection input which this change is making.

@glensc
Copy link
Collaborator

glensc commented Sep 28, 2021

a4d0bbd: you misunderstood me, it's a mindset of windows users that password must be shown as stars. other users consider that that's leaking password info if you show how long is your password while you're typing and someone peeks over your shoulders. I never said you should make such changewindows only. what I said, that rather using yet another framework as dependency, use existing ones. we have click, we have rich already as deps.

it's my opinion (other maintainers might think otherwise). also, I won't support any windows specific code at all. ever. python is platform-independent and should stay that way here.

@glensc
Copy link
Collaborator

glensc commented Sep 28, 2021

Screenshots should go to PR body

@glensc
Copy link
Collaborator

glensc commented Sep 28, 2021

Server selection: you lost information of server details that were printed before

@sirloinofbeef
Copy link
Contributor Author

sirloinofbeef commented Sep 28, 2021

Screenshots should go to PR body

Fixed

Server selection: you lost information of server details that were printed before

No, that information is still there. The selectable list appears directly after it as per the current server selection input.

that's leaking password info

Completely understand. The idea of the password * is to make things more user friendly for a susbset of users (Windows users). If this presents too much of a concern then I'm happy to remove it, particularly if it distracts from the main purpose of this push, selectable server / user input options rather than typed (which are more susceptible to user error e.g. typos). I only tacked on the password input alteration as the dependency happened to support it on top of the selectable input options.

I won't support any windows specific code

That's fair.

@glensc
Copy link
Collaborator

glensc commented Sep 28, 2021

  1. please remove platform specific code. use same style for all platforms
  2. change "main user" to show actual login of main account. maybe add dash separator between it and managed users?
  3. your screenshot shows only server names, doesn't show server IP, URL, last seen, server version
- ONEPLUS A5000: [Last seen: 2021-09-27 21:54:39, Server: Plex for Android (Mobile)/8.23.1.28053 on ONEPLUS A5000: Android/10]
    http://192.168.0.30:32500
- Vulpes: [Last seen: 2021-09-28 08:26:17, Server: Plex Media Server/1.22.3.4392-d7c624def on Docker Container (LinuxServer.io): Linux/4.14.134-boot2docker]
    https://192-168-0-xxx.plex.direct:32400
    https://192-168-0-xxx.plex.direct:32400

@glensc
Copy link
Collaborator

glensc commented Sep 28, 2021

default password via commandline/env is lost: default=password

@sirloinofbeef
Copy link
Contributor Author

  1. please remove platform specific code. use same style for all platforms

Done

  1. change "main user" to show actual login of main account. maybe add dash separator between it and managed users?

I'll lookup how to do this and implement.

  1. your screenshot shows only server names, doesn't show server IP, URL, last seen, server version

Screenshot updated to show positioning in flow.

@sirloinofbeef
Copy link
Contributor Author

sirloinofbeef commented Sep 28, 2021

default password via commandline/env is lost: default=password

Double checked and it IS there as per the original code.

@glensc
Copy link
Collaborator

glensc commented Sep 28, 2021

default password via commandline/env is lost: default=password

Double checked and it IS there as per the original code.

yes, it's back once you removed platform-specific code.

@glensc glensc force-pushed the patch-4 branch 2 times, most recently from e61f2fc to 6373202 Compare September 29, 2021 06:05
sirloinofbeef and others added 3 commits September 29, 2021 09:05
Co-authored-by: Elan Ruusamäe <[email protected]>
Server input now uses inquirer to make selection easier.

Managed user Y/N and input combined into single inquirer selection with MAIN USER as default.

Co-authored-by: Elan Ruusamäe <[email protected]>
The main Plex username is passed from the original user input and displayed at the top of the user list as the default.
Also sorted server list for easier navigation.

A more elegant solution to passing the main Plex username.

Co-authored-by: Elan Ruusamäe <[email protected]>
@glensc glensc changed the title InquirerPy user input Use InquirerPy for user input Sep 29, 2021
@glensc glensc added the tested label Sep 29, 2021
@glensc glensc merged commit 278bb27 into Taxel:main Sep 29, 2021
Repository owner locked as resolved and limited conversation to collaborators Jan 11, 2022
@glensc
Copy link
Collaborator

glensc commented Jan 30, 2024

The "rich" package discussion on the feature request:

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

Successfully merging this pull request may close these issues.

2 participants