Add weeks to schedule options #551
Conversation
|
I'm all for having the every hour/day/week dropdown, but setting the starting time of Cron seems overkill. |
|
Its the only way I could think of for converting a QDateTime to a crontrigger. Edit2: Peewee supports storing datetimefields, will change to that |
|
Don't merge this yet, going to add #263. |
|
Because it clutters the UI/uses mental resources/adds more code to maintain while a majority of users doesn't care about it. |
|
What type of trigger doesn't actually matter. In the end I'm using a run_date=datetime.datetime, eliminating the need for a trigger. This UI is less cluttered by removing the daily and hourly options. Edit: I realize you're talking about the QDateTimeEdit |
|
Initially, my UI concept was:
This would require switching between all the time periods, along with UI switching, requiring more code and maintenance. If for some reason we wanted to add or remove an option, it would be a lot more work this way than with the way I have in the PR. Alternatively, it could be done with setDisplayFormat and enums (still requires switching code). Also it would be confusing since the line length would shrink and grow as sections are removed/added, shifting text to the right of it around. Edit: A search of "Backup scheduler" on google images reveals many UIs a lot more complex than this. |
|
Basically what I'm saying is that this is the simplest it can get while supporting hours/days/weeks/months Edit: As well as a persistent backup time store, needed for knowing if a backup was missed. |
|
Tests segfault on here, but on my own branch they work https://github.com/samuel-w/vorta-1/runs/1029008552?check_suite_focus=true. Weird. |
The dropdown option is great, but can we remove the starting at calendar field? Reasons explained above. |
This was my reasoning behind the calendar field. TLDR: Calendar is the simplest way to do it in code and UI Removing it and autosetting the time would not work either, since it removes functionality that was there before (run backup daily at specific time). Its a thing that is set infrequently anyways. In case your talking about the calendar dropdown itself and not the QDateTimeEdit, its easier to visualize dates using a calendar than typing. |
|
The drop-down and number field are great. Every X hours/days/weeks. I just question the usefulness of setting the start date. Won't it become meaningless after a while? User needs control of it? |
|
The start date is automatically bumped every time a scheduled backup is run, so it's always correct. |
|
Don't we show the next backup run further down already? What's the purpose of the starting at field exactly? In your screenshot they show the same date and time. |
|
The starting at field allows configuring the date and time, replicating the previous functionality of being able to set the hour interval for hourly backups and the time of day for daily backups. It adds the ability to set the day / hour for weekly backups and the day / hour for monthly backups. Removing it would create a feature regression. |
|
I see. Now I get it. You removed the Daily option below.
Looking at the code, I think this PR is doing too many things at once with a very long and risky DB migration. The PRs you linked suggest two things:
I think the current form handles the hourly and daily options quite nicely. For weekly we have 2 options:
Personally, I like option 2. Next, the missed backup issue. I see you set a very large misfire-grace time for APScheduler. Have you verified that this will only do one missed backup and not 5, if 5 hourly backups were missed? If yes, this one setting would suffice. I wouldn't run missed backups on startup. There is also the suggestion to prevent backups on battery power. If we can detect the battery state for both OS, this would be an interesting option too. |
|
All the migration code is doing is converting the old time options into the new time options. It's long because it precisely converts the options, rather than resetting user scheduling. It should be safe as long as the user doesn't kill Vorta from the task manager while its upgrading the DB, which takes very fast unless someone has 100 profiles. On the weekly thing, I don't think there is a need to add a day of the week selector, since if the weekly option is selected, it will run every n weeks on the same day of the week. Additionally, there aren't any requests to add running daily backups on certain days of the week. The misfire_grace_time does need a coalese option added to it. As for the battery power, this PR already does too much, I'll add the battery power in later https://github.com/samuel-w/vorta-1/tree/BatteryLimit |
|
What other changes should I make, I already:
For the earlier changes, see my comment above. |
|
I would love to see this merged soon! This will fix a lot of issues I have been having with my laptop backups. |
|
Needs more discussion. The current PR would loose backing up at a fixed time and the "starting at" isn't needed. Suggestions welcome (no code needed). |
|
Backing up at a fixed time is still there. Just set it to something like 8pm and daily and it will run every day at 8pm. Same thing with weekly, set it to 8pm Monday and it will run every week at 8pm on Monday. Same thing with hourly, set it to every 3 hours starting at 8pm and it will run at 8pm,11pm,2am... The super long conversion code is so that the daily and hourly settings are converted correctly. As stated earlier, starting at is the only simple way of doing this, at least according to other backup. Scheduling is a set and forget thing, so I'd rather have marginally more complex UI and simpler code than the reverse. |
|
We could always have four lines https://images.app.goo.gl/9ZNYWmZAyWyMdmhv9 That would remove the starting at that you don't like. |
|
Looks very complex to be honest. Maybe we just keep it as-is, if there is no better solution. Just the migration code is massive. |
|
The migration code has to be massive, otherwise we lose data. |
|
|
|
Wow. Looks very nice, but also complex. We could remove old migration code after 6 months or so. |
|
Does the migration code really matter though? Its a run once thing. Also, its not good to assume that everyone will update after 6 months. I don't want to break any existing installs. |
|
So should we go with the current setup or the "starting at". I prefer the current setup just because I have sunk 3ish hours into it, and it avoids having an explicit datetime selector. It also keeps the similarity to the current UI so the change is less jarring. |
|
Hmm, should I keep the lambda function connections, or connect all boxes to the scheduler apply and read all the data there? First one is cleaner since it reads only the changed data, second one is easier to read and understand. |
|
This removes the minutes past the hour setting for hourly backups. Going to fix that. |
|
Done. The model updating code is less of a mess now. |
|
So do we go with the current setup or the 'starting at' setup? |
|
For me it is most important that a backup is done directly at startup when scheduled backups have been missed. I have some users that log in only occasionally. If I set the backup frequency to once per day with the present system, they may never get backed up. So I have to set it to 1h, just to catch them when they are there. To me that is the most important aspect of this thread. |
|
OK, its back as a feature. Just waiting for it to be reviewed. Edit: Had to remove it as multiple backups cannot run at a time. |
|
Yeah, sorry. A bit slow since it's a bigger PR. But better take some time to do it right. |
|
I'll try to make missed backups a notification, since its easier to do. |
|
Woohoo! 7 months old! 3 weeks until this PR's 8 month birthday. |
|
Yeah, waiting long for merges suck... Don't forget though that @m3nu is maintaining Vorta pretty much on their own currently 😊 |
|
Yeah, I understand. Its just arguing that the datetime selector needs to be there to reduce code length, removing the datetime selector which increases code length, and then being shocked about the code length that I warned about is very discouraging. |
|
😢 |
|
No need to close this, @samuel-w. This will be used, but more work is needed. I imagine the next scheduler iteration to:
|
|
A pity that this PR has been closed without implementation. |
This is another fairly large change, but it shouldn't break any existing setups.
This implements a persistent backup time storage, making #263 easier to implement.
The pretty_date localizes only the day of the week (Monday) but not the HH:MM am/pm
Fixes #487
Semifixes #263 by adding notification and extending gracetime so it runs missed backups if Vorta is not closed.
Edit: Fixes #343 (Last one hopefully)
This has become an enormous PR.
Discussions #804 #812