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: Document setting offset in help #435

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

luke-jr
Copy link
Contributor

@luke-jr luke-jr commented Jul 8, 2021

No description provided.

@Et0h
Copy link
Contributor

Et0h commented Jul 23, 2021

I've not tested this yet, but this seems like a good improvement to me. Thanks for your contribution.

@daniel-123
Copy link
Contributor

Offset feature wasn't listed in there mostly because back in the day we thought it is confusing to use.

Thad said - I don't see a problem with adding it there, though in such case it should also be added to the man page in docs/syncplay.1.

I can add it there myself after merging if you aren't up for it in this PR.

@Et0h
Copy link
Contributor

Et0h commented Jul 25, 2021

Do people still use this feature, and if so are they actually finding it worth the hassle? I find it not confusing and a lot more hassle than just ensuring that everyone has the same file, so I can appreciate what daniel-123 is saying about the reasons why it was not listed previously. However, there might still be people with slow Internet connections perhaps who find the feature useful.

Options include:

  1. Keeping and documenting the feature, but adding some sort of warning in the documentation that its use is not recommended (i.e. that it is deprecated).
  2. Removing the feature.
  3. Keeping the feature, but making it a mostly undocumented to discourage use.

@daniel-123
Copy link
Contributor

Thinking about it a bit more:

  • Offset is present in GUI anyway. I'd assume vast majority of users use GUI, so given it's visible there it makes sense for it to be documented as console option.
  • Offset has gotten a bit more awkward with introduction of playlists. Right now it is set per instance of Syncplay running, but that doesn't translate well into watching multiple different files - not all of them might require offset so you need to manually keep track of it.

I don't see strong reasons to outright remove it (yet?), but marking it as depreciated along with documenting it sounds like best course of action for now.

@luke-jr given that you made the PR in first place, I assume you use the offset feature on at least somewhat regular basis. Could you share some feedback about how it works for you and how it's useful?

@luke-jr
Copy link
Contributor Author

luke-jr commented Jul 25, 2021

Yes, my group uses it all the time. Often we have files that have variation in pre-content company names and such. Everyone getting the same file means waiting on slow downloads, and people who want super-high-quality (eg, 80 GB 4K) having to fall back to lower quality so those of us with pitiful bandwidth or smaller screens (scaling down doesn't work well) can actually watch.

We don't, however, use the playlist feature ever. It doesn't seem very useful - even though we watch multiple things in sequence sometimes, we typically don't plan what to watch next until the first one is over.

@Et0h Et0h merged commit a593b85 into Syncplay:master Oct 27, 2021
Et0h added a commit that referenced this pull request Oct 27, 2021
Et0h added a commit that referenced this pull request Oct 27, 2021
* Create pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* Update pythonpackage.yml

* add libxcb manually

* Add missing libxcb-util to build environment

* Remove references to IRC (#430)

* Add reference to GitHub discussions

* Update issue templates

* Revert "Merge branch 'master' into master"

This reverts commit 173007e, reversing
changes made to 6105da8.

* Make Windows build 32-bit

* Support for LAV Filters Megamix (#457)

* Upver to 1.7.0 (r100)

* Refactor MPC-HC player path code (#453)

* Add LANG Parameter (#460)

If LANG parameter set, don't show language dialog.
Example usage: .\Syncplay-X.X.X-Setup.exe /S /LANG=1033

* Trusted Domains: Allow trusting a domain regardless of HTTP basic auth credentials (#437)

* Trusted Domains: don't consider HTTP basic auth credentials part of the domain name

* Trusted Domains: hide "add as trusted domain" menu item if entry does not contain domain

* Trusted Domains: strip HTTP basic auth credentials also when adding as trusted domain via context menu

* Allow .m3u/.m3u8 files to be played from network (#419)

* Fix room name case sensitivity UI issue (#403)

* Remove redundant help button from dialogs (#403)

* darkdetect: update vendor copy to 0.5.0

* macOS build: upgrade to Python 3.9 and PySide2 5.15.2

* Remove Encoding from .desktop files as it's depreciated now.

* Add Keywords entry to .desktop files.

* Console: Document setting offset in help (#435)

* Add deprecation notice for offset help (#435)

* Begin move from m3u/m3u8 to txt for playlist (#408)

Co-authored-by: Daniel Wróbel <[email protected]>
Co-authored-by: daniel-123 <[email protected]>
Co-authored-by: Assistant <[email protected]>
Co-authored-by: ImportTaste <[email protected]>
Co-authored-by: Ata Gülalan <[email protected]>
Co-authored-by: Tremolo4 <[email protected]>
Co-authored-by: Ridan Vandenbergh <[email protected]>
Co-authored-by: Alberto Sottile <[email protected]>
Co-authored-by: Luke Dashjr <[email protected]>
@luke-jr
Copy link
Contributor Author

luke-jr commented Aug 16, 2022

(Please don't deprecate the offset command! As mentioned, we rely on it)

@Et0h
Copy link
Contributor

Et0h commented Mar 30, 2023

(Please don't deprecate the offset command! As mentioned, we rely on it)

Your views have been noted.

Please be aware that within this context 'deprecate' does not mean remove, it means that we don't recommend using it and we don't promise to maintain support for it forever.

We wouldn't just remove support for it for no reason, but if we end up doing a refactor of the code to make way for a feature which cares about media position (such as if we ever allow for playback speed to be synced) we might decide to drop offset support to make it easier to implement that new feature.

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