[pmg-devel] [PATCH pmg-gui v2] StatTimeSelector: don't show invalid month/day combinations

[pmg-devel] [PATCH pmg-gui v2] StatTimeSelector: don't show invalid month/day combinations

[Dominik Csapak]

by limiting the store of the day selector by the selected month

reported by a user in the forum:
https://forum.proxmox.com/threads/wrong-calendar.111631/

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* use js 'Date' to correctly calculate the last day of the month
  including leapyears, etc.

 js/StatTimeSelector.js | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/js/StatTimeSelector.js b/js/StatTimeSelector.js
index f01b058..eae2d91 100644
--- a/js/StatTimeSelector.js
+++ b/js/StatTimeSelector.js
@@ -73,7 +73,25 @@ Ext.define('PMG.StatTimeSelector', {
 	    Ext.GlobalEvents.fireEvent('pmgStatTimeSelectorUpdate', data);
 	},
 
+	updateDays: function() {
+	    let yearsel = this.lookupReference('yearsel');
+	    let monthsel = this.lookupReference('monthsel');
+	    let daysel = this.lookupReference('daysel');
+	    let year = yearsel.getValue();
+	    let month = monthsel.getValue();
+	    // create a date for the next month, but day 0 which wraps to
+	    // the last day of the current month. Our month is already
+	    // 1 greater than what Date expects, so we don't have to add 1
+	    let maxDays = new Date(year, month, 0).getDate();
+	    daysel.getStore().setFilters([{
+		property: 'day',
+		operator: '<=',
+		value: maxDays,
+	    }]);
+	},
+
 	onSelect: function() {
+	    this.updateDays();
 	    this.updateVisibility();
 	},
 
-- 
2.30.2

Re: [pmg-devel] [PATCH pmg-gui v2] StatTimeSelector: don't show invalid month/day combinations

[Thomas Lamprecht]

On 01/07/2022 10:03, Dominik Csapak wrote:
> by limiting the store of the day selector by the selected month
> 
> reported by a user in the forum:
> https://forum.proxmox.com/threads/wrong-calendar.111631/
> 

looks functional, some code nits inline that would make this quite a bit
shorter (and thus easier/quicker to read and grasp), ideally with those
addressed (but also otherwise if you really don't find them sensible):

Reviewed-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * use js 'Date' to correctly calculate the last day of the month
>   including leapyears, etc.
> 
>  js/StatTimeSelector.js | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/js/StatTimeSelector.js b/js/StatTimeSelector.js
> index f01b058..eae2d91 100644
> --- a/js/StatTimeSelector.js
> +++ b/js/StatTimeSelector.js
> @@ -73,7 +73,25 @@ Ext.define('PMG.StatTimeSelector', {
>  	    Ext.GlobalEvents.fireEvent('pmgStatTimeSelectorUpdate', data);
>  	},
>  
> +	updateDays: function() {

really not hard feelings for this one, but would be something like `updateMaxDays`
or `updateDayLimit` slightly more telling?

> +	    let yearsel = this.lookupReference('yearsel');
> +	    let monthsel = this.lookupReference('monthsel');

nit: useless intermediate variables

> +	    let daysel = this.lookupReference('daysel');
> +	    let year = yearsel.getValue();
> +	    let month = monthsel.getValue();

let year = this.lookupReference('yearsel').getValue();
let month = this.lookupReference('monthsel').getValue();

> +	    // create a date for the next month, but day 0 which wraps to
> +	    // the last day of the current month. Our month is already
> +	    // 1 greater than what Date expects, so we don't have to add 1

I'd use 100 cc for comments to reduce vertical space and reword as:

        // get last day of current month through wrapping back day 0 from next (zero indexed) month

a bit terse, but anybody that worked with JS's current date API just a bit would expect
anything sane here...

> +	    let maxDays = new Date(year, month, 0).getDate();
> +	    daysel.getStore().setFilters([{

In the spirit of above I'd drop the single use `daysel` too and instead do:

this.lookupReference('daysel').setFilters([{

> +		property: 'day',
> +		operator: '<=',
> +		value: maxDays,
> +	    }]);
> +	},
> +
>  	onSelect: function() {
> +	    this.updateDays();
>  	    this.updateVisibility();
>  	},
>  

Re: [pmg-devel] [PATCH pmg-gui v2] StatTimeSelector: don't show invalid month/day combinations

[Dominik Csapak]

On 7/1/22 11:32, Thomas Lamprecht wrote:
> On 01/07/2022 10:03, Dominik Csapak wrote:
>> by limiting the store of the day selector by the selected month
>>
>> reported by a user in the forum:
>> https://forum.proxmox.com/threads/wrong-calendar.111631/
>>
> 
> looks functional, some code nits inline that would make this quite a bit
> shorter (and thus easier/quicker to read and grasp), ideally with those
> addressed (but also otherwise if you really don't find them sensible):
> 
> Reviewed-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> 
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes from v1:
>> * use js 'Date' to correctly calculate the last day of the month
>>    including leapyears, etc.
>>
>>   js/StatTimeSelector.js | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/js/StatTimeSelector.js b/js/StatTimeSelector.js
>> index f01b058..eae2d91 100644
>> --- a/js/StatTimeSelector.js
>> +++ b/js/StatTimeSelector.js
>> @@ -73,7 +73,25 @@ Ext.define('PMG.StatTimeSelector', {
>>   	    Ext.GlobalEvents.fireEvent('pmgStatTimeSelectorUpdate', data);
>>   	},
>>   
>> +	updateDays: function() {
> 
> really not hard feelings for this one, but would be something like `updateMaxDays`
> or `updateDayLimit` slightly more telling?
> 
>> +	    let yearsel = this.lookupReference('yearsel');
>> +	    let monthsel = this.lookupReference('monthsel');
> 
> nit: useless intermediate variables
> 
>> +	    let daysel = this.lookupReference('daysel');
>> +	    let year = yearsel.getValue();
>> +	    let month = monthsel.getValue();
> 
> let year = this.lookupReference('yearsel').getValue();
> let month = this.lookupReference('monthsel').getValue();
> 
>> +	    // create a date for the next month, but day 0 which wraps to
>> +	    // the last day of the current month. Our month is already
>> +	    // 1 greater than what Date expects, so we don't have to add 1
> 
> I'd use 100 cc for comments to reduce vertical space and reword as:
> 
>          // get last day of current month through wrapping back day 0 from next (zero indexed) month
> 
> a bit terse, but anybody that worked with JS's current date API just a bit would expect
> anything sane here...
> 
>> +	    let maxDays = new Date(year, month, 0).getDate();
>> +	    daysel.getStore().setFilters([{
> 
> In the spirit of above I'd drop the single use `daysel` too and instead do:
> 
> this.lookupReference('daysel').setFilters([{
> 
>> +		property: 'day',
>> +		operator: '<=',
>> +		value: maxDays,
>> +	    }]);
>> +	},
>> +
>>   	onSelect: function() {
>> +	    this.updateDays();
>>   	    this.updateVisibility();
>>   	},
>>   
> 

sure the changes make sense, sending as v3
although the comment does not fit in 100 columns here (takes up 103)
would that be still ok, or should i try to find an even shorter
sentence?

(also after looking at it again, i'd use 'lookup' instead of
'lookupReference' since that's only an alias and much shorter)

Re: [pmg-devel] [PATCH pmg-gui v2] StatTimeSelector: don't show invalid month/day combinations

[Thomas Lamprecht]

On 01/07/2022 11:57, Dominik Csapak wrote:
> although the comment does not fit in 100 columns here (takes up 103)
> would that be still ok, or should i try to find an even shorter
> sentence?

argh, missed one indentation level (clocked in at 99 here).. fine with either.

> 
> (also after looking at it again, i'd use 'lookup' instead of
> 'lookupReference' since that's only an alias and much shorter)

good call