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

luci-mod-system: give crontab (aka scheduled tasks) some helpers #7495

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

systemcrash
Copy link
Contributor

@systemcrash systemcrash commented Dec 22, 2024

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Tested on: 23.05.5, Firefox ✅
  • ( Preferred ) Screenshot or mp4 of changes:
  • Description: (describe the changes proposed in this PR)

Screenshot 2024-12-23 at 15 51 44

Any input from @dannil @stokito @Ramon00 @hnyman @rdmitry0911 ?

Gave crontab a small makeover. It's raw and rudimentary but it is already an improvement over editing the raw crontab.

Good if others can take it for a spin or suggest improvements. ( inter-row and element styling )

I do not recommend putting any safeguards in for the command field, since we were already editing the crontab directly.

@stokito
Copy link
Contributor

stokito commented Dec 22, 2024

Interesting how many people and how use the cron. The UI improvements also increase a size but it looks like that's not a problem anymore.
From the screenshot I can tell that:

  1. The original editor needs to be preserved so if if someone just copy-paste from instructions.
  2. The command field needs to be wider.
  3. We may use the same visual language of a table/sections

As for inspiration we may check how this implemented in other web admin panels like cPanel and Webmin https://webmin.com/docs/modules/scheduled-cron-jobs/

@systemcrash
Copy link
Contributor Author

Interesting how many people and how use the cron. The UI improvements also increase a size but it looks like that's not a problem anymore. From the screenshot I can tell that:

Not many users? Possibly because it remains cryptic.

1. The original editor needs to be preserved so if if someone just copy-paste from instructions.

Fair point. I thought retaining both original window would be possible. Requires two save buttons and routines.

2. The command field needs to be wider.

Fixable. But it then affects whichever column it appears in.

3. We may use the same visual language of a table/sections

Also thought about this. Probably more maintainable.

Take it for a test-drive and see how it works for you.

@stokito
Copy link
Contributor

stokito commented Dec 22, 2024

Maybe it would be enough to add a link to a wiki https://openwrt.org/docs/guide-user/base-system/cron.

@Ramon00
Copy link
Contributor

Ramon00 commented Dec 22, 2024

The layout needs some work, especially if the table is empty. See e.g.:
image
Looks like you need to fill in an hour value in the empty box instead of the command. Is it supposed to work already because if I hit save then I just get the rotating circle thing (on 23.05.5).

@systemcrash
Copy link
Contributor Author

The layout needs some work, especially if the table is empty. See e.g.: image Looks like you need to fill in an hour value in the empty box instead of the command. Is it supposed to work already because if I hit save then I just get the rotating circle thing (on 23.05.5).

Yeah, that’s the command value. The minute column could occupy six columns to ensure the command column is placed correctly. But then which column is minute?

It should work. Check the console.

@Ramon00
Copy link
Contributor

Ramon00 commented Dec 22, 2024

image

@hnyman
Copy link
Contributor

hnyman commented Dec 22, 2024

To me the proposed editor looks confusing compared to the direct crontab file editing.

It might be better to just provide more help text about the fields with the old crontab editor.

@systemcrash
Copy link
Contributor Author

Probably. I think this can be provided side-by-side instead of replacing the existing crontab editor.

@Ramon00
Copy link
Contributor

Ramon00 commented Dec 22, 2024

yes side-by-side would be perfect, i.e. show and allow editing of the resulting cron rules. Maybe also good to add a comment field?

@systemcrash
Copy link
Contributor Author

yes side-by-side would be perfect, i.e. show and allow editing of the resulting cron rules. Maybe also good to add a comment field?

Side-by-side it is. A comment field only has a place if the crontab itself also permits it. Otherwise it's a code comment at the end of the command. e.g. command.sh param # comment

@systemcrash
Copy link
Contributor Author

image

Fixed.

Maybe also good to add a comment field?

Added.

Maybe it would be enough to add a link to a wiki https://openwrt.org/docs/guide-user/base-system/cron.

Added.

@Ramon00
Copy link
Contributor

Ramon00 commented Dec 23, 2024

Fixed

I just tested it and indeed now it is functional.

Cosmetic remark, maybe add some pixels space here between the two boxes?
image

One more remark, if I click on the pulldown select "every Xth" it then changes to:
image
If i then hit save, i get a
*/5 undefined undefined undefined undefined
in the file.

@systemcrash
Copy link
Contributor Author

Good find. Fixed now. That'll get converted so that next page load it's no longer an alias but a normal task format.

Anyway, the pretty layout needs some love.

Signed-off-by: Paul Donald <newtwen+github@gmail.com>
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.

4 participants