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

[O2B-643] Run duration accept format HH:MM:SS #1464

Draft
wants to merge 56 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 55 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
355ddac
works
xsalonx Mar 14, 2024
9702196
works
xsalonx Mar 14, 2024
4b87ce8
refactor
xsalonx Mar 14, 2024
d436308
add model
xsalonx Mar 14, 2024
eda937b
nEPS filter works
xsalonx Mar 15, 2024
8c18794
filters work
xsalonx Mar 15, 2024
15b5dac
filter works
xsalonx Mar 15, 2024
c3b2f86
pol
xsalonx Mar 15, 2024
e9e7420
pol
xsalonx Mar 15, 2024
5e98f04
linter
xsalonx Mar 15, 2024
3cbb87f
add default
xsalonx Mar 15, 2024
1a61b69
docs
xsalonx Mar 15, 2024
2d1731a
conf min max
xsalonx Mar 15, 2024
2475d19
docs
xsalonx Mar 15, 2024
b37573a
value
xsalonx Mar 15, 2024
78950b5
change default
xsalonx Mar 15, 2024
7f03dff
test WIP
xsalonx Mar 15, 2024
58d31e5
Test
xsalonx Mar 15, 2024
191d434
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Mar 18, 2024
fbdefc0
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Mar 25, 2024
3661588
remove override
xsalonx Mar 27, 2024
a06640d
refactor
xsalonx Mar 27, 2024
21c1cf4
use SelectionMode
xsalonx Mar 27, 2024
0ab6860
rename
xsalonx Mar 27, 2024
144317c
rename
xsalonx Mar 27, 2024
40b70e2
bug
xsalonx Mar 27, 2024
68a01d7
simp
xsalonx Mar 27, 2024
ff0da59
cherry-picked
xsalonx Mar 27, 2024
7d57685
cherry -pick
xsalonx Mar 27, 2024
71cb30d
rm
xsalonx Mar 27, 2024
14fc5a8
cleanup
xsalonx Mar 27, 2024
6dda2bc
cl
xsalonx Mar 27, 2024
e639db8
revoke
xsalonx Mar 27, 2024
1d5e0c7
remove
xsalonx Mar 27, 2024
cd9d679
ref
xsalonx May 30, 2024
c4c9fc7
ch
xsalonx May 30, 2024
b59e0d2
add model
xsalonx May 30, 2024
a4933f2
ref WIP
xsalonx May 30, 2024
5d6c941
ref
xsalonx May 30, 2024
fe67005
fix
xsalonx May 30, 2024
f165e23
fix
xsalonx May 30, 2024
52e7bf6
merge main
xsalonx May 30, 2024
318d0a5
cleanup
xsalonx May 30, 2024
7d05396
test WIP
xsalonx May 30, 2024
a4843f3
merge main
xsalonx Jun 3, 2024
6b013dd
fix
xsalonx Jun 3, 2024
c353f54
fix
xsalonx Jun 3, 2024
73425be
fix
xsalonx Jun 4, 2024
c4c70dd
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Jun 4, 2024
2507a4a
simp
xsalonx Jun 4, 2024
e6763c6
merge main
xsalonx Jun 4, 2024
a908084
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Jun 4, 2024
bf412c4
add method
xsalonx Jun 4, 2024
2ea7b4a
merge main
xsalonx Jun 10, 2024
76905be
Merge branch 'main' into xsalonx/misc/O2B-643/run-duration-should-acc…
xsalonx Jun 11, 2024
eb7ae9f
merg main
xsalonx Oct 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 69 additions & 19 deletions lib/public/components/Filters/RunsFilter/durationFilter.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,76 @@
* or submit itself to any jurisdiction.
*/

import { amountFilter } from '../common/filters/amountFilter.js';
import { h } from '/js/src/index.js';
import { comparisonOperatorFilter } from '../common/filters/comparisonOperatorFilter.js';

/**
* Returns the run duration filter component
* Returns the duration filter component
*
* @param {RunsOverviewModel} runModel the runs model object
*
* @return {vnode} the duration filter
* @param {DurationFilterModel} durationFilterModel the duration filter model
* @return {Component} the duration filter
*/
export const durationFilter = (runModel) => amountFilter(
runModel.runDurationFilter,
(filter) => {
runModel.runDurationFilter = filter;
},
{
operatorAttributes: {
id: 'duration-operator',
},
limitAttributes: {
id: 'duration-limit',
},
},
);
export const durationFilter = (durationFilterModel) => {
const { durationInputModel, operatorSelectionModel } = durationFilterModel;

const { hours, minutes, seconds } = durationInputModel.raw;

/**
* Return oninput handler for given time unit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mention what the function should be used for, only explain what it does.

*
* @param {'hours'|'minutes'|'seconds'} unitName time unit
* @return {function(InputEvent, void)} oninput handler for input for given time unit
*/
const updateInputModelHandlerByTimeUnit = (unitName) => (e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems to add more complexity than it brings value I think

const { value } = e.target;
const parsedValue = Number(value);
if (value.length > 0 || 0 <= parsedValue && (unitName === 'hours' || parsedValue < 60)) {
durationInputModel.update({ [unitName]: value.length > 0 ? parsedValue : null });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so, if you delete a value, update will not be triggered, so it will be put back to previous value.
Plus, this kind of processing should be in the model.
What you do in oninput is simply call durationFilterModel update, pass it RAW value (i.e. the string input), you store this raw value and try to extract the actual value.
If user put 80 minutes you still need to keep this as raw value but mark the input as invalid, and not update the actual value.
Please have a look at other inputs, such as DateTimeInputComponent

} else {
durationFilterModel.visualChange$.notify();
}
};

const hoursInput = h('input.flex-grow', {
id: 'hours-input',
type: 'number',
min: 0,
value: hours,
oninput: updateInputModelHandlerByTimeUnit('hours'),
});
const minutesInput = h('input.flex-grow', {
id: 'minutes-input',
type: 'number',
min: 0,
max: 59,
value: minutes,
oninput: updateInputModelHandlerByTimeUnit('minutes'),
}, 'm');
const secondsInput = h('input.flex-grow', {
id: 'seconds-input',
type: 'number',
min: 0,
max: 59,
value: seconds,
oninput: updateInputModelHandlerByTimeUnit('seconds'),
}, 's');

const inputs = h('.flex-row.w-100', [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is w-100 usefull? Can't we use flex-grow or equivalent instead?

hoursInput,
minutesInput,
secondsInput,
h('.flex-row.items-center.p2', [
h('label', { for: 'hours-input' }, 'h'),
':',
h('label', { for: 'minutes-input' }, 'm'),
':',
h('label', { for: 'seconds-input' }, 's'),
]),
]);

return comparisonOperatorFilter(
inputs,
operatorSelectionModel.selected[0],
(operator) => operatorSelectionModel.select(operator),
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* @license
* Copyright CERN and copyright holders of ALICE O2. This software is
* distributed under the terms of the GNU General Public License v3 (GPL
* Version 3), copied verbatim in the file "COPYING".
*
* See http://alice-o2.web.cern.ch/license for full licensing information.
*
* In applying this license CERN does not waive the privileges and immunities
* granted to it by virtue of its status as an Intergovernmental Organization
* or submit itself to any jurisdiction.
*/

import { SelectionModel } from '../../../common/selection/SelectionModel.js';

/**
* Model storing state of a selection of predefined comparison operators
*/
export class ComparisonOperatorSelectionModel extends SelectionModel {
/**
* Constructor
* @param {string} [defaultOperator = '='] one of ['<', '<=', '=', '>=', '>'] operators
*/
constructor(defaultOperator = '=') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't override this parameter anywhere, simply don't add it

super({
availableOptions: ['<', '<=', '=', '>=', '>'].map((operator) => ({ value: operator })),
multiple: false,
allowEmpty: false,
defaultSelection: [{ value: defaultOperator }],
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* @license
* Copyright CERN and copyright holders of ALICE O2. This software is
* distributed under the terms of the GNU General Public License v3 (GPL
* Version 3), copied verbatim in the file "COPYING".
*
* See http://alice-o2.web.cern.ch/license for full licensing information.
*
* In applying this license CERN does not waive the privileges and immunities
* granted to it by virtue of its status as an Intergovernmental Organization
* or submit itself to any jurisdiction.
*/

import { Observable } from '/js/src/index.js';

import { DurationInputModel } from '../../../common/form/inputs/DurationInputModel.js';
import { ComparisonOperatorSelectionModel } from './ComparisonOperatorSelectionModel.js';

/**
* Duration filter model which stores time value and selected operator
*/
export class DurationFilterModel extends Observable {
/**
* Constructor
*/
constructor() {
super();
this._visualChange$ = new Observable();

this._durationInputModel = new DurationInputModel();
this._durationInputModel.bubbleTo(this);
this._operatorSelectionModel = new ComparisonOperatorSelectionModel();
this._operatorSelectionModel.observe(() => {
if (this._durationInputModel.value === null) {
this._visualChange$.notify();
} else {
this.notify();
}
});
Comment on lines +33 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not how it should work I suppose. If operatorSelectionModel has visualChange you need to bubleUp this to the current visualChange, but not mix the global notify and the visual change

}

/**
* Returns the observable notified any time there is a visual change which has no impact on the actual filter value
*
* @return {Observable} the observable
*/
get visualChange$() {
return this._visualChange$;
}

/**
* Retrun duration input model
*
* @return {DurationInputModel} duration input model
*/
get durationInputModel() {
return this._durationInputModel;
}

/**
* Return operator selection model
*
* @return {ComparisonOperatorSelectionModel} operator selection model
*/
get operatorSelectionModel() {
return this._operatorSelectionModel;
}

/**
* States if the filter has been filled
*
* @return {boolean} true if the filter has been filled
*/
isEmpty() {
return this._durationInputModel.value === null;
}

/**
* Reset the filter to its default value
*
* @return {void}
*/
reset() {
this._durationInputModel.reset();
this._operatorSelectionModel.reset();
}
}
98 changes: 98 additions & 0 deletions lib/public/components/common/form/inputs/DurationInputModel.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/**
* @license
* Copyright CERN and copyright holders of ALICE O2. This software is
* distributed under the terms of the GNU General Public License v3 (GPL
* Version 3), copied verbatim in the file "COPYING".
*
* See http://alice-o2.web.cern.ch/license for full licensing information.
*
* In applying this license CERN does not waive the privileges and immunities
* granted to it by virtue of its status as an Intergovernmental Organization
* or submit itself to any jurisdiction.
*/

import { Observable } from '/js/src/index.js';

/**
* @typedef DurationInputRawData
* @property {number} hours the number of hours
* @property {number} minutes the number of minutes, from range [0, 59]
* @property {number} seconds the number of seconds, from range [0, 59]
*/

/**
* Store the duration input
*/
export class DurationInputModel extends Observable {
/**
* Constructor
*/
constructor() {
super();

this._raw = {
hours: null,
minutes: null,
seconds: null,
};

/**
* Timestamp (ms)
* @type {number|null}
* @private
*/
this._value = null;
}

/**
* Update the inputs raw values
* @param {DurationInputRawData} raw the input raw values
* @return {void}
*/
update(raw) {
try {
this._raw = { ...this._raw, ...raw };
const { hours, minutes, seconds } = this._raw;
if ((hours ?? minutes ?? seconds ?? null) === null) {
this._value = null;
} else {
this._value = (hours || 0) * 60 * 60 * 1000
+ (minutes || 0) * 60 * 1000
+ (seconds || 0) * 1000;
}
} catch {
this._value = null;
}

this.notify();
}

/**
* Reset the inputs to its initial state
* @return {void}
*/
reset() {
this._raw = {
hours: null,
minutes: null,
seconds: null,
};
this._value = null;
}

/**
* Returns the raw input values
* @return {DurationInputRawData} the raw values
*/
get raw() {
return this._raw;
}

/**
* Returns the current date represented by the inputs (null if no valid value is represented)
* @return {number|null} the current value
*/
get value() {
return this._value;
}
}
3 changes: 1 addition & 2 deletions lib/public/views/Runs/ActiveColumns/runsActiveColumns.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,7 @@ export const runsActiveColumns = {
noEllipsis: true,
format: (_duration, run) => displayRunDuration(run),
exportFormat: (_duration, run) => formatRunDuration(run),
filter: durationFilter,
filterTooltip: 'Run duration (in minutes)',
filter: ({ runDurationFilterModel }) => durationFilter(runDurationFilterModel),
},
environmentId: {
name: 'Environment ID',
Expand Down
Loading
Loading