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

Reworked Bandwidth Scheduler #9203

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

Conversation

FurkanKambay
Copy link

@FurkanKambay FurkanKambay commented Jul 16, 2018

This implements the advanced bandwidth scheduler described in #8804 but without the fancy grid UI.

Explanation & features implemented

  • Completely reworked the existing BandwidthScheduler (and moved it into a subdirectory). Introduced new data classes ScheduleDay and ScheduleEntry.
  • Global/alternative/scheduled limit prioritization is as follows:
    1. The Paused state at current time (from the new scheduler, explained below),
    2. Alternative limits if enabled,
    3. Scheduler limits at current time,
    4. Fallback to global limits.
  • The scheduler has a new Pause option that pauses or resumes the native lt::session. This is currently being prioritized more than the alternative limits but that can be changed.
  • The schedule data is stored in a JSON file alongside the regular settings. The file is updated only when the user clicks Apply/OK in settings after making changes to the scheduler.
  • Both the JSON data and the user input are validated properly. Should still be beta tested though.
  • When running for the first time, the old scheduler settings are "imported" (converted) to the new scheduler format and the old scheduler settings are removed. (see the importLegacyScheduler method)
  • The main feature is the editable "Scheduler" table added to the Speed tab in the Options page. Users can navigate between Monday-Sunday and add/remove/edit any time-span to the scheduler using the context menu. The input fields for adding a new entry are: start time, end time, download speed, upload speed, and pause.
  • Schedule tables are inline editable and have copy-paste support, 'clear all', and 'copy to other days' functionality for better UX. All of these are in the context menu.
  • WebUI is on par with native UI, except for editing entries. (User can remove an entry and add a new one to do the same thing anyway)
  • The session paused state is displayed as [Paused] on the status bar.

Screenshots

Native UI WebUI
Screenshot of scheduler options, native UI context menu Screenshot of scheduler options, Web UI context menu
Screenshot of scheduler options, native UI add dialog Screenshot of scheduler options, Web UI add dialog

src/base/scheduler/day.h Outdated Show resolved Hide resolved
@FurkanKambay

This comment has been minimized.

@FurkanKambay

This comment has been minimized.

@FurkanKambay

This comment was marked as outdated.

@FurkanKambay

This comment was marked as off-topic.

@sledgehammer999
Copy link
Member

Unless I am mistaken github allows other people to push to your branch once you have opened a PR based on it. So your branch can be the feature branch you're asking for.

@FurkanKambay

This comment was marked as outdated.

@FurkanKambay FurkanKambay changed the title [WIP] Advanced scheduler [WIP] Advanced scheduler - help needed Jul 26, 2018
@FurkanKambay

This comment has been minimized.

@FurkanKambay

This comment has been minimized.

@sledgehammer999
Copy link
Member

@FurkanKambay I quickly skimmed your posts. I didn't read any of the code. Here are my thoughts and suggestions. If something is interfering with what you have already decided/implemented disregard it.

I have given thought to this issue a long time ago, but I lack the time and motivation to actually code it.
Personally I like the way the scheduler is implemented in the program foobar2000. IMO, it is simpler in terms of UI design but as powerful(or even more) as other implementations.
In case you aren't familiar with it, I post 2 screenshots AND explain it.
Screenshots:
foobar1
foobar2

Basically we let the user define a specific Date-Time(timestamp) and what happens(action) when that Date-Time comes. So the UI just represents a list of timestamps and their associated actions. In the 1st iteration of the implementation of the scheduler the only allowed action is to enable/disable alternative speed limits.
The next iterations can broaden the available actions and even allow groups of actions per timestamp. Ideas for actions: pause/start session, specific DL, UL limits (making alternative limits useless), running external program
Also the timestamp should be flexible like it is on foobar2000: Either a specific timestamp or something like "On Tuesdays and Mondays at 12:00 PM do this action". aka per day actions.
In this type of scheduler the user is responsible to set a new timestamp that reverses the action of a previous timestamp. This type of scheduler doesn't deal in timestamp ranges.

A few words on how to approach this PR.
There are 3 distinct places where coding is done.

  1. The core
  2. The GUI
  3. The WebUI

The core
IMO, no matter what UI representation of the scheduler you choose the core should care about one thing: "At timestamp X do this". BandwidthScheduler should be reworked to do the following:

  1. Hold the list of timestamp,actions
  2. Interpret the list in such a way that it knows what's the next/earliest event based on currrent time
  3. Run a timer regularly. On timeout check current time and compare to the next in line event. If it is time for the event, instruct the Session (and related classes) on the actions required (eg set X DL speed). Drop the event from the list if it isn't recurring. Also don't forget to sanitize the list in case the user has changed the clock between 2 timeouts and some events have gone stale.

GUI and WebUI
Once core is implemented the interface implementation is almost trivial.

The above is just in abstract. When actually coding it you might need to make modifications.

I hope I didn't give you headaches.

@Liggliluff
Copy link

Does it read which weekday should be first, and the time and date formats from the system?

@sledgehammer999
Copy link
Member

Does it read which weekday should be first, and the time and date formats from the system?

Why does this matter?

@Liggliluff
Copy link

Because you are supposed to support the user's format.

I, as a European, don't want to use the American format just like how an American doesn't want to use the European format. (If they want to use a different format, they can change it in the regional settings).

@sledgehammer999
Copy link
Member

The UI representation is independent of the internal one. The UI should display the formats in the current locale anyway.

@Jarpetnee
Copy link

Can we expect this on an upcoming beta release? I am looking forward to being able to pause torrents during the day instead of the pseudo-pause of 1kbps and still connected to the swarm. This has a good use case for Sonarr and Radarr users that need constant client uptime (for torrent adding) but want scheduled upload/download downtime. Otherwise I would just close the client during the day.

@FurkanKambay
Copy link
Author

@glassez

I'd also like an update. I know the PR is a bit large so let me know if there's anything you need to know. I manually tested the app with the latest appveyor build on a clean Windows 11, and it works just fine.

There are only minor visual bugs that have already been discussed here, and as I said, I have no time or motivation to fix them, but also they're small enough to at least make it into the beta in my opinion. (ps, I'll squash my commits once it's ready for merging)

@FurkanKambay
Copy link
Author

Is there anything I can do right now? I will be mostly unavailable until the end of the year. If there are any concerns or bugs, I might be able to address them this month.

@thalieht
Copy link
Contributor

Maybe this has been discussed before but... being unable to set e.g. 10 PM -2 AM (next day) is not user friendly.

i would remove "advanced" from "Enable advanced scheduler". I mean there is no basic scheduler so...

@FurkanKambay
Copy link
Author

@thalieht

I removed 'advanced' this time, sorry I forgot.

being unable to set e.g. 10 PM -2 AM (next day) is not user friendly

I could see someone expecting it to work like that, but I think I don't have the time or motivation to implement it now.

@mushis
Copy link

mushis commented Aug 22, 2023

Can we expect this on an upcoming beta release? I am looking forward to being able to pause torrents during the day instead of the pseudo-pause of 1kbps and still connected to the swarm. This has a good use case for Sonarr and Radarr users that need constant client uptime (for torrent adding) but want scheduled upload/download downtime. Otherwise I would just close the client during the day.

This is exactly what I am looking for. Any news?

@xavier2k6 xavier2k6 removed the Stale label Sep 24, 2023
@FurkanKambay FurkanKambay changed the title Advanced scheduler Reworked Bandwidth Scheduler Sep 26, 2023
@FurkanKambay
Copy link
Author

FurkanKambay commented Nov 2, 2023

@glassez @FranciscoPombal @xavier2k6

Do you have a guess as to when this can be reviewed? Or could I get some preliminary feedback based on the information and screenshots I provided in the OP? I will probably have time to work on this from January.

Some questions:

  • The old scheduler settings are deleted after being converted to the new system; is that OK or should the user have the option to stay on the old system? I'm not sure how much work bringing back the old scheduler would be.
  • Is the location for importing the old settings fine or do I have to move it to upgrade.cpp?
  • The red/green validation doesn't look good as we've discussed here, but is that a blocking issue for the PR?
  • Does it seem realistic to merge this into a beta branch in 2024?

Also, some testing would be very much appreciated from anyone who'd like to see this feature.

todo for me
  • review app structure for scheduler and session
  • split ui state caching from scheduler
  • split file io from scheduler
  • add entry UI is confusing, the times should be at the top, and the labels for dl/ul should be below the times.
  • before importing old settings, back up the old settings, and warn the user.
  • (maybe?) import old settings in upgrade.cpp instead
  • (maybe?) "clear this time range from other days" action

@luzpaz
Copy link
Contributor

luzpaz commented Nov 3, 2023

Can the PR be rebased? then we can get testers to test it

@glassez
Copy link
Member

glassez commented Nov 3, 2023

Do you have a guess as to when this can be reviewed? Or could I get some preliminary feedback based on the information and screenshots I provided in the OP? I will probably have time to work on this from January.

I honestly tried to review it, as promised earlier. I've started this several times, trying to come in from different ends. But unfortunately it was not successful. In the end, I came to the conclusion that it does not make sense to review the entire PR, making comments in each specific place, but first you need to eliminate its fundamental shortcomings, which are blocking.
I'm not talking about its UI, you've discussed it a lot with other people, so I guess it looks quite acceptable now, so I don't care about it.
I am mainly concerned about its implementation. And there I see two main drawbacks:

  1. The first is the ambiguous position of the Bandwidth Scheduler in the structure of application components. It must either be a standalone component created and managed by the Application, and communicate with other components (e.g. BitTorrent Session) using their public interface (e.g. when it needs to change some rate session->setDownloadSpeed(val) etc.), or an internal part of the Session itself, then it must be created and managed by the Session and communicate with it privately, and with external components through the public interface of the Session (e.g. session->doSomethingRelatedToScheduler()), or provide its own public interface through the Session interface (e.g. session->bandwidthScheduler()->doSomething()), but not mixing different ways.
  2. The second drawback is "obliging" the caller side (the code that uses it) to be responsible for some logic, which in fact should be its implementation detail, adding these private things to its interface. (I mean saving the configuration to a file.) The interface should remain simple and reflect the subject area of the implemented model. Otherwise, it makes the entire surrounding code ugly, obscure and poorly maintainable. Within the scope of the subject area, the Scheduler client is supposed to only add, modify (optionally) and delete scheduler entries. It should not care about any service aspects of the Scheduler's functioning, such as saving configuration data, etc. If it is designed according to the above, then it will be simple, understandable, flexible and extensible. If buffered/caching interaction is required in some use case, for example, to provide the GUI editor with deferred applying of the changes made and the possibility of their cancellation, then this can be implemented as a separate layer (representation model) that would interact with the user (more precisely, with GUI components of the editor), caching all changes until they need to be applied, and then pass them to the Scheduler itself. You can take "Torrent files watcher" component and related stuff as a reference of how this could be done. Note that even in this case the client code (representation model) has nothing to do with internals of used component, it just add/edit/remove entries as usual.

I hope the above was at least somewhat understandable.
I have some more questions (not so serious, perhaps), but it's pointless to ask them now until the above problems are fixed.

P.S. Such long comments are uncharacteristic for me. I prefer to avoid discussions in which I need to read and (especially) to write such comments. I hope that further discussion of this topic will not require such comments.

@FurkanKambay
Copy link
Author

@glassez Thanks for the comprehensive explanation. I understand the problems; I'll start working on those probably in January.

@luzpaz There are a bunch of conflicts to go through so rebasing will take time but the build artifacts can be downloaded from the CI runs right now. However, testing can wait now since I have more work to do.

@AndreEriksenNLTG
Copy link

Still hoping too see this merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Implement new feature/subsystem PR waiting author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet