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

Added next run and last run time to timers display. #4503

Conversation

Projects
None yet
5 participants
@harishanand95
Copy link
Contributor

commented May 30, 2016

@dperpeet @petervo I need moment.js in this. i did like this selinux/moment to get moment as of now. Can u tell me how to add moment right way?
@andreasn can u look into designs here?
screenshot from 2016-05-30 16-41-55

also what should I do when it shows last run 46 years ago i.e 1970 ?

@petervo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2016

You should rebase this on the latest master.

To use moment you should create a link in your directory to the moment.js file in bower_components and then change the makefile to include it in the bundle for this package. You can see an example of this in either the selinux, kubernetes or ostree package.

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch from a12908c to fa4eec5 May 31, 2016

@harishanand95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

@andreasn why does the disabled section has a different style than enabled and static ?

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

What do you mean more specifically?
That they don't have the "Previous Run" and "Next Run"? or that they are kind of centered in the middle of the table?

@harishanand95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

@andreasn Previous run and next run is what I added extra. Is it okay? Is the repetition of the words next run and last run for each of entry good?
and yes I was actually talking about the id's being centered in the middle for disable? Why is that? I felt it would have been nice to have it like enabled and static.

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch 3 times, most recently from 295b6b9 to f5f9156 Jun 2, 2016

@harishanand95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2016

screenshot from 2016-06-06 15-01-31
screenshot from 2016-06-06 15-12-32

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch 3 times, most recently from 79791ab to c5059eb Jun 9, 2016

@@ -127,4 +131,4 @@ update-lib:: update-bower
$(JSMODULE) -m -d jquery -o $(srcdir)/pkg/systemd/bootstrap-datepicker.js \
$(BOWER)/bootstrap-datepicker/dist/js/bootstrap-datepicker.js
$(JSMODULE) -m -d jquery -o $(srcdir)/pkg/systemd/bootstrap-combobox.js \
$(BOWER)/bootstrap-combobox/js/bootstrap-combobox.js
$(BOWER)/bootstrap-combobox/js/bootstrap-combobox.js

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

missing newline

unit.is_timer = true;
if ( unit.ActiveState == "active" ) {
var timer_unit = systemd_client.proxy('org.freedesktop.systemd1.Timer', unit.path);
wait_valid(timer_unit, add_timer_properties, unit);

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

wait_valid is only used here - I think it'd be clearer to have the code inline instead of a function.

@@ -218,6 +227,31 @@ define([
});
}

function wait_valid(proxy, callback, args) {
proxy.wait(function() {
if (proxy.valid)

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

indentation

}

function add_timer_properties(timer_unit,unit) {
unit.LastTriggerTime = moment(timer_unit.LastTriggerUSec/1000).fromNow();

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

The problem with this is that it doesn't update. Using moment.js is very nice, but it does bring in the whole refresh question: If you use timestamps / specific times, you can just render them and that's it. If you compare to "now", then that needs to update or become confusing after a while.
Related to this: I would feel better if the times gathered from the backend were actual timestamps. Then you could run the moment.js conversion when displaying, in this case: https://github.com/cockpit-project/cockpit/pull/4503/files#diff-276d25fb4053326350efbc4f097771c2R291

if (timer_unit.LastTriggerUSec === 0)
unit.LastTriggerTime = _("not known");
var next_run_time = 0;
if ( timer_unit.NextElapseUSecRealtime === 0)

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

whitespace

unit.LastTriggerTime = moment(timer_unit.LastTriggerUSec/1000).fromNow();
if (timer_unit.LastTriggerUSec === 0)
unit.LastTriggerTime = _("not known");
var next_run_time = 0;

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

you're mixing different capitalization styles (LastTriggerTime, next_run_time)
You can either stick to the old one (next_run_time), since this is an edit of an existing file, or write the new code with camelCase.

// A template, create a fake unit for it

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

extra whitespace (not just an empty line)

</body>
</html>
</html>

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

missing newline

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

looks good, but there is still some work to be done

if (name.indexOf("@") != -1) {

if ( name.slice(-5) == "timer")

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

whitespace, better: if (name

if (name.indexOf("@") != -1) {

if ( name.slice(-5) == "timer")
is_timer = true;

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 16, 2016

Member

indentation: 4 spaces

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch from c5059eb to a879454 Jun 16, 2016

@harishanand95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2016

screenshot from 2016-06-16 23-10-17

@dperpeet I have changed next run time and last trigger time from relative time to calendar time-stamp.
Sorry abt the indentation and spaces

next_run_time = timer_unit.NextElapseUSecRealtime;
}
unit.NextRunTime = moment(next_run_time/1000).calendar();
}

This comment has been minimized.

Copy link
@harishanand95

harishanand95 Jun 16, 2016

Author Contributor

I found in init.js that variables are written like next_run_time, active_state, systemd_manager etc while properties of unit are like unit.LoadState, unit.UnitFileState, unit.CombinedState et cetera.
so I think having properties as unit.NextRunTime, unit.LastTriggerTime and variables as next_run_time is better than changing?

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 20, 2016

Member

I found in init.js that variables are written like next_run_time, active_state, systemd_manager etc while properties of unit are like unit.LoadState, unit.UnitFileState, unit.CombinedState et cetera.
so I think having properties as unit.NextRunTime, unit.LastTriggerTime and variables as next_run_time is better than changing?

Old cockpit code uses the underscore notation. In newer code, we've switched to the way most of the other js code seems to be written: camelCase.

The exposed DBUS properties (e.g. unit.NextRunTime) are actually a special case, since that is a DBUS convention. Cockpit API will automatically expose the properties if they begin with a capital letter.

Looking at your code again, I believe you did this right - you can remain consistent with the current code.

// A template, create a fake unit for it
units_by_path[name] = {
Id: name,
Description: cockpit.format(_("$0 Template"), name),
UnitFileState: state[1]
UnitFileState: state[1],
is_timer: is_timer

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 20, 2016

Member

nitpick: is_timer seems to be only used once
Could you just write is_timer: (name.slice(-5) == "timer") here?

@dperpeet dperpeet added question and removed needswork labels Jun 20, 2016

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch 2 times, most recently from 378cf80 to 4ba4abd Jun 22, 2016

b.wait_visible('#services')
m.execute("systemctl start test.timer")
b.wait_in_text(svc_sel('test.timer'), "Today")
b.wait_in_text(svc_sel('test.timer'), "unknown")

This comment has been minimized.

Copy link
@harishanand95

harishanand95 Jun 22, 2016

Author Contributor

testservices-testbasic-timer

@dperpeet The test checks if test.timer shows "Today" as part of next run and "unknown" in last trigger time.

@andreasn andreasn self-assigned this Jun 22, 2016

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2016

the labels "Next Run Time" and "Last Trigger Time" sounds a bit off to me.
Could it be just "Next Run" and "Last Trigger" instead?
That it's times you'll be able to see from the fact that the labels are times and dates.

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch 2 times, most recently from 0447b9a to 5b3bd23 Jun 23, 2016

@harishanand95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2016

@andreasn removed "Time" from Next Run Time and Last Trigger Time and also added few deleted new lines that was already present in the code.
screenshot from 2016-06-23 17-43-11

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2016

Can current state be just "State"?

@harishanand95

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2016

@andreasn Corrected
screenshot from 2016-06-23 18-19-49

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2016

Looks good to me.
Thanks for fixing this!

@dperpeet dperpeet removed the question label Jun 24, 2016

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch from de38899 to b4dedaf Jun 27, 2016

b.click('#services-filter :nth-child(4)')
b.wait_visible('#services')
m.execute("systemctl start test.timer")
b.wait_in_text(svc_sel('test.timer'), "Today")

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 27, 2016

Member

I think this is our most common source for race conditions in our tests: before you can wait for the text to be there, you should first wait for the element o be present.
In this case, add a line before: b.wait_present(svc_sel('test.timer'))

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 27, 2016

Member

also, make sure that svc_sel('test.timer') is exactly the selector you want to use here

@dperpeet dperpeet added the needswork label Jun 27, 2016

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch from b4dedaf to 17316ee Jun 27, 2016

@harishanand95 harishanand95 referenced this pull request Jun 28, 2016

Closed

systemd create timer option #4645

6 of 6 tasks complete

@dperpeet dperpeet removed the needswork label Jun 28, 2016

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch 2 times, most recently from 24deff7 to 9575f8d Jun 28, 2016

@dperpeet dperpeet assigned dperpeet and unassigned andreasn Jun 29, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

There's a merge conflict - can you please rebase on master and squash the commits into one? Thanks!

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@andreasn Right now this code doesn't update the display, it only shows the time differences (e.g. "in 5 seconds") on load. I think this is ok for now. Do you agree?

@dperpeet dperpeet added the needswork label Jun 30, 2016

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

@dperpeet yeah, I think it's all right for now, but I would like it to be fixed as a followup.

@harishanand95 harishanand95 force-pushed the harishanand95:systemd-nextrun-lastrun branch from 9575f8d to bc4632d Jun 30, 2016

@dperpeet dperpeet removed the needswork label Jun 30, 2016

@dperpeet dperpeet closed this in bd9e8ae Jul 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.