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

Console improvements #327

Merged
merged 9 commits into from
Sep 13, 2020
Merged

Console improvements #327

merged 9 commits into from
Sep 13, 2020

Conversation

odrling
Copy link
Contributor

@odrling odrling commented Jun 21, 2020

Hi,

I saw #319 and hacked my way through to get it to work (I'm expecting a few things will look dirty to you).

Various improvements to the console UI, most basic operations to the playlist can be done from the console and media directories are loaded with --no-gui (currently you need to load the files manually even when the media directories are set correctly). I might try to add a batch edit too later (open the playlist in an editor, maybe using the $EDITOR environment variable).

The name of the commands could use some improvements too, I've gotten used to using q (add file to the end of playlist), ql (show the play list) and qs (select a file in the playlist).

I've been using this for a few weeks and it seems to be working well FWIW.

@Et0h
Copy link
Contributor

Et0h commented Aug 6, 2020

Cheers @odrling. Good to see good use being made of what @csandras05 has contributed. Sorry for my delay in reviewing this PR.

I've tested it and it seems to work fine for me in console mode, but in GUI mode if you run from the chat box (or mpv) /q [path] it won't always work. I think that this is because you need to do the same changes #319 does for addFileToPlaylist in gui.py:

    def addFileToPlaylist(self, filePath, index=-1):
        if not isURL(filePath):
            self.removePlaylistNote()
            filename = os.path.basename(filePath)
            if self.noPlaylistDuplicates(filename):
                if self.playlist == -1 or index == -1:
                    self.playlist.addItem(filename)
                else:
                    self.playlist.insertItem(index, filename)
                self._syncplayClient.fileSwitch.notifyUserIfFileNotInMediaDirectory(filename, filePath)
        else:
            self.removePlaylistNote()
            if self.noPlaylistDuplicates(filePath):
                if self.playlist == -1 or index == -1:
                    self.playlist.addItem(filePath)
                else:
                    self.playlist.insertItem(index, filePath)

Some things to consider:

  1. When listing/specifying playlist index it is zero-based. From an ease-of-coding point of view this makes sense, but in terms of usability some people might prefer for it to be translated into a 1-based system. Thoughts?
  2. For consistency, one possibility is for everything to do with the playlist to start with a 'q': qa to add, qd to delete, qs to select and ql to list.
  3. At present there is no way to enable/disable shared playlist mode at runtime from within console mode. Is there a strong use case for this or not?

@odrling
Copy link
Contributor Author

odrling commented Aug 7, 2020

Just glad to see that this PR may not sleep here forever ;)

Indeed I've not tested the commands in GUI mode (and probably started with the earlier commit of @csandras05 from #316).

When listing/specifying playlist index it is zero-based. From an ease-of-coding point of view this makes sense, but in terms of usability some people might prefer for it to be translated into a 1-based system. Thoughts?

Both are fine for me and I understand why some would prefer to start the index at 1, so I'll change that.

For consistency, one possibility is for everything to do with the playlist to start with a 'q': qa to add, qd to delete, qs to select and ql to list.

I do like this, it makes the commands feel more like a set of command (you type q and you know that you will be editing the playlist). I do find it a bit odd to start the commands with q (it meant 'queue' initially) which now feels a bit arbitrary but I also would not want to start the command with p which would make it too easy to pause the playback for everyone with an unfortunate typo.

At present there is no way to enable/disable shared playlist mode at runtime from within console mode. Is there a strong use case for this or not?

I've never toggled this mode. Though now that I remember that it exists I could have used it too at some point instead of trying to work around the fact that some people would have different playlists. I might add it, I'm guessing this should not be hard to add. I could see this as a qsh command.

@odrling
Copy link
Contributor Author

odrling commented Aug 7, 2020

I also think I should add a check for duplicate files in the playlist, it seems to be in the method addFileToPlaylist in gui.py too. I did add a duplicate once and it was a bit confusing to see the playlist be automatically updated by someone else in the room to remove the duplicate. A simple "You can't add a file that is already in the playlist" in the console would make it easier to understand.

@Et0h
Copy link
Contributor

Et0h commented Aug 8, 2020

I do find it a bit odd to start the commands with q

I agree that it's a bit weird, but as p and l are already taken I think queue might be the most memorable because the playlsit is a queuing system.

I've never toggled this mode...I might add it, I'm guessing this should not be hard to add. I could see this as a qsh command.

I personally just have playlists on all the time, so I don't toggle it either. I am perfectly happy a decision not to add this feature in the name of not wanting to over-load the list of playlist commands. We can expect anyone who uses console to manually change the setting if they want the behaviour to change in the same way that they have to manually change the list of media directories. Those who want everything to be easy can just use the GUI.

The main times when people might want to disable shared playlists is if they are wanting to play a 720p version of a file while they are in the middle of downloading the 1080p version, or if they are wanting to play a local version of a file which has been added from a URL such as YouTube. However, Syncplay doesn't have to fully support every edge case because to do so adds to the complexity of the software.

If we were to add an enable/disable toggle I'm not sure what qsh means. Maybe qt for toggle, or qon and `qoff" to enable/disable (although that would then mean two new command line switches).

I also think I should add a check for duplicate files in the playlist

You can either do that, or see if you can make it so that both GUI and console use the same check.

@Et0h
Copy link
Contributor

Et0h commented Sep 13, 2020

Messages output via the print function do not show up in the GUI console notifications window. As such, the new print functions in consoleUI.py need to be changed to self.showErrorMessage or self.showMessage as appropriate.

@Et0h Et0h merged commit b09fb90 into Syncplay:master Sep 13, 2020
albertosottile pushed a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
…and Syncplay#319

* add videos to playlist from chat

* add urls to playlist

* add files in media directory to playlist

* add commands to show the playlist and select an index

* add command to delete files from the playlist

* show selected index in playlist

* fix adding files with queue command in GUI mode

* start indexing the playlist at 1

or at least that's what it would look like to the user

* start all commands related to playlist with `q`

Co-authored-by: kiscs <csandras05@gmail.com>
albertosottile pushed a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
albertosottile pushed a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
albertosottile pushed a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
…and Syncplay#319

* add videos to playlist from chat

* add urls to playlist

* add files in media directory to playlist

* add commands to show the playlist and select an index

* add command to delete files from the playlist

* show selected index in playlist

* fix adding files with queue command in GUI mode

* start indexing the playlist at 1

or at least that's what it would look like to the user

* start all commands related to playlist with `q`

Co-authored-by: kiscs <csandras05@gmail.com>
albertosottile pushed a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
albertosottile pushed a commit to albertosottile/syncplay that referenced this pull request Sep 30, 2020
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.

3 participants