Re: Review Request 129709: Fix checking for valid date entered

[David Jarvie]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/
---

(Updated Dec. 31, 2016, 12:13 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and John Layt.


Changes
---

Submitted with commit 1550b2c83f4842eb491b906ff7029fc11fd07d8a by David Jarvie 
to branch master.


Repository: kwidgetsaddons


Description
---

Checks to determine whether an entered date is valid are wrong or missing in 
various places in the code. This is due to:
- not testing if a minimum or maximum date is set before comparing a date to 
the minimum/maximum;
- always rejecting new minimum/maximum dates if the other maximum/minimum date 
is not set;
- not testing dates for validity when set by the date picker or menu.

This results in the following bugs currently:
- When an up/down arrow or page up/down key is pressed to change the date, and 
the minimum and maximum dates are not set, it is always considered invalid and 
the date is not changed.
- When the DateKeywords option is set, and the minimum and maximum dates are 
not set, the only date which is displayed in the menu is "No Date".
- setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
currently set.
- setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
currently set.
- resetDateRange() does nothing.

This patch fixes the above bugs.

Documentation comments in the header file are also improved.


Diffs
-

  autotests/kdatecomboboxtest.cpp c15525a 
  src/kdatecombobox.h d9a20ca 
  src/kdatecombobox.cpp ad1d085 

Diff: https://git.reviewboard.kde.org/r/129709/diff/


Testing
---

Updated autotests pass. GUI changes tested with KAlarm, including setting 
DateKeywords. The bugs described above are now fixed.


Thanks,

David Jarvie

Re: Review Request 129709: Fix checking for valid date entered

[Christoph Feck]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/#review101674
---


Ship it!




Ship It!

- Christoph Feck


On Dec. 30, 2016, 8:10 p.m., David Jarvie wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129709/
> ---
> 
> (Updated Dec. 30, 2016, 8:10 p.m.)
> 
> 
> Review request for KDE Frameworks and John Layt.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Checks to determine whether an entered date is valid are wrong or missing in 
> various places in the code. This is due to:
> - not testing if a minimum or maximum date is set before comparing a date to 
> the minimum/maximum;
> - always rejecting new minimum/maximum dates if the other maximum/minimum 
> date is not set;
> - not testing dates for validity when set by the date picker or menu.
> 
> This results in the following bugs currently:
> - When an up/down arrow or page up/down key is pressed to change the date, 
> and the minimum and maximum dates are not set, it is always considered 
> invalid and the date is not changed.
> - When the DateKeywords option is set, and the minimum and maximum dates are 
> not set, the only date which is displayed in the menu is "No Date".
> - setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
> currently set.
> - setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
> currently set.
> - resetDateRange() does nothing.
> 
> This patch fixes the above bugs.
> 
> Documentation comments in the header file are also improved.
> 
> 
> Diffs
> -
> 
>   autotests/kdatecomboboxtest.cpp c15525a 
>   src/kdatecombobox.h d9a20ca 
>   src/kdatecombobox.cpp ad1d085 
> 
> Diff: https://git.reviewboard.kde.org/r/129709/diff/
> 
> 
> Testing
> ---
> 
> Updated autotests pass. GUI changes tested with KAlarm, including setting 
> DateKeywords. The bugs described above are now fixed.
> 
> 
> Thanks,
> 
> David Jarvie
> 
>

Re: Review Request 129709: Fix checking for valid date entered

[David Jarvie]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/
---

(Updated Dec. 30, 2016, 7:10 p.m.)


Review request for KDE Frameworks and John Layt.


Changes
---

Renamed isValid() to isInDateRange()


Repository: kwidgetsaddons


Description
---

Checks to determine whether an entered date is valid are wrong or missing in 
various places in the code. This is due to:
- not testing if a minimum or maximum date is set before comparing a date to 
the minimum/maximum;
- always rejecting new minimum/maximum dates if the other maximum/minimum date 
is not set;
- not testing dates for validity when set by the date picker or menu.

This results in the following bugs currently:
- When an up/down arrow or page up/down key is pressed to change the date, and 
the minimum and maximum dates are not set, it is always considered invalid and 
the date is not changed.
- When the DateKeywords option is set, and the minimum and maximum dates are 
not set, the only date which is displayed in the menu is "No Date".
- setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
currently set.
- setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
currently set.
- resetDateRange() does nothing.

This patch fixes the above bugs.

Documentation comments in the header file are also improved.


Diffs (updated)
-

  autotests/kdatecomboboxtest.cpp c15525a 
  src/kdatecombobox.h d9a20ca 
  src/kdatecombobox.cpp ad1d085 

Diff: https://git.reviewboard.kde.org/r/129709/diff/


Testing
---

Updated autotests pass. GUI changes tested with KAlarm, including setting 
DateKeywords. The bugs described above are now fixed.


Thanks,

David Jarvie

Re: Review Request 129709: Fix checking for valid date entered

[Christoph Feck]

> On Dec. 30, 2016, 5:59 p.m., Christoph Feck wrote:
> > src/kdatecombobox.cpp, line 58
> > 
> >
> > Could you please add a short comment what this function does? A 
> > condition like
> > 
> > !date.isValid() || isValid(date)
> > 
> > looks confusing without some explanation.

Maybe a different name would help, something mention 'range'. isInDateRange()?


- Christoph


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/#review101672
---


On Dec. 28, 2016, 6:49 p.m., David Jarvie wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129709/
> ---
> 
> (Updated Dec. 28, 2016, 6:49 p.m.)
> 
> 
> Review request for KDE Frameworks and John Layt.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Checks to determine whether an entered date is valid are wrong or missing in 
> various places in the code. This is due to:
> - not testing if a minimum or maximum date is set before comparing a date to 
> the minimum/maximum;
> - always rejecting new minimum/maximum dates if the other maximum/minimum 
> date is not set;
> - not testing dates for validity when set by the date picker or menu.
> 
> This results in the following bugs currently:
> - When an up/down arrow or page up/down key is pressed to change the date, 
> and the minimum and maximum dates are not set, it is always considered 
> invalid and the date is not changed.
> - When the DateKeywords option is set, and the minimum and maximum dates are 
> not set, the only date which is displayed in the menu is "No Date".
> - setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
> currently set.
> - setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
> currently set.
> - resetDateRange() does nothing.
> 
> This patch fixes the above bugs.
> 
> Documentation comments in the header file are also improved.
> 
> 
> Diffs
> -
> 
>   autotests/kdatecomboboxtest.cpp c15525a 
>   src/kdatecombobox.h d9a20ca 
>   src/kdatecombobox.cpp ad1d085 
> 
> Diff: https://git.reviewboard.kde.org/r/129709/diff/
> 
> 
> Testing
> ---
> 
> Updated autotests pass. GUI changes tested with KAlarm, including setting 
> DateKeywords. The bugs described above are now fixed.
> 
> 
> Thanks,
> 
> David Jarvie
> 
>

Re: Review Request 129709: Fix checking for valid date entered

[Christoph Feck]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/#review101672
---




src/kdatecombobox.cpp (line 58)


Could you please add a short comment what this function does? A condition 
like

!date.isValid() || isValid(date)

looks confusing without some explanation.


- Christoph Feck


On Dec. 28, 2016, 6:49 p.m., David Jarvie wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129709/
> ---
> 
> (Updated Dec. 28, 2016, 6:49 p.m.)
> 
> 
> Review request for KDE Frameworks and John Layt.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Checks to determine whether an entered date is valid are wrong or missing in 
> various places in the code. This is due to:
> - not testing if a minimum or maximum date is set before comparing a date to 
> the minimum/maximum;
> - always rejecting new minimum/maximum dates if the other maximum/minimum 
> date is not set;
> - not testing dates for validity when set by the date picker or menu.
> 
> This results in the following bugs currently:
> - When an up/down arrow or page up/down key is pressed to change the date, 
> and the minimum and maximum dates are not set, it is always considered 
> invalid and the date is not changed.
> - When the DateKeywords option is set, and the minimum and maximum dates are 
> not set, the only date which is displayed in the menu is "No Date".
> - setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
> currently set.
> - setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
> currently set.
> - resetDateRange() does nothing.
> 
> This patch fixes the above bugs.
> 
> Documentation comments in the header file are also improved.
> 
> 
> Diffs
> -
> 
>   autotests/kdatecomboboxtest.cpp c15525a 
>   src/kdatecombobox.h d9a20ca 
>   src/kdatecombobox.cpp ad1d085 
> 
> Diff: https://git.reviewboard.kde.org/r/129709/diff/
> 
> 
> Testing
> ---
> 
> Updated autotests pass. GUI changes tested with KAlarm, including setting 
> DateKeywords. The bugs described above are now fixed.
> 
> 
> Thanks,
> 
> David Jarvie
> 
>

Re: Review Request 129709: Fix checking for valid date entered

[David Jarvie]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/
---

(Updated Dec. 28, 2016, 5:49 p.m.)


Review request for KDE Frameworks and John Layt.


Changes
---

Extra checks added to autotests


Repository: kwidgetsaddons


Description (updated)
---

Checks to determine whether an entered date is valid are wrong or missing in 
various places in the code. This is due to:
- not testing if a minimum or maximum date is set before comparing a date to 
the minimum/maximum;
- always rejecting new minimum/maximum dates if the other maximum/minimum date 
is not set;
- not testing dates for validity when set by the date picker or menu.

This results in the following bugs currently:
- When an up/down arrow or page up/down key is pressed to change the date, and 
the minimum and maximum dates are not set, it is always considered invalid and 
the date is not changed.
- When the DateKeywords option is set, and the minimum and maximum dates are 
not set, the only date which is displayed in the menu is "No Date".
- setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
currently set.
- setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
currently set.
- resetDateRange() does nothing.

This patch fixes the above bugs.

Documentation comments in the header file are also improved.


Diffs (updated)
-

  autotests/kdatecomboboxtest.cpp c15525a 
  src/kdatecombobox.h d9a20ca 
  src/kdatecombobox.cpp ad1d085 

Diff: https://git.reviewboard.kde.org/r/129709/diff/


Testing (updated)
---

Updated autotests pass. GUI changes tested with KAlarm, including setting 
DateKeywords. The bugs described above are now fixed.


Thanks,

David Jarvie

Re: Review Request 129709: Fix checking for valid date entered

[Christoph Feck]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/#review101609
---



There is autotests/kdatecomboboxtest.cpp. Does it need adaptions? If not, does 
it still pass?

- Christoph Feck


On Dec. 27, 2016, 6:46 p.m., David Jarvie wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129709/
> ---
> 
> (Updated Dec. 27, 2016, 6:46 p.m.)
> 
> 
> Review request for KDE Frameworks and John Layt.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Checks to determine whether an entered date is valid are wrong or missing in 
> various places in the code. This is due to:
> - not testing if a minimum or maximum date is set before comparing a date to 
> the minimum/maximum;
> - always rejecting new minimum/maximum dates if the other maximum/minimum 
> date is not set;
> - not testing dates for validity when set by the date picker or menu.
> 
> This results in the following bugs currently:
> - When an up/down arrow or page up/down key is pressed to change the date, it 
> is always considered invalid and the date is not changed.
> - When the DateKeywords option is set, the only date which is displayed in 
> the menu is "No Date".
> - setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
> currently set.
> - setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
> currently set.
> - resetDateRange() does nothing.
> 
> This patch fixes the above bugs.
> 
> Documentation comments in the header file are also improved.
> 
> 
> Diffs
> -
> 
>   src/kdatecombobox.h d9a20ca 
>   src/kdatecombobox.cpp ad1d085 
> 
> Diff: https://git.reviewboard.kde.org/r/129709/diff/
> 
> 
> Testing
> ---
> 
> Tested with KAlarm, including setting DateKeywords. The bugs described above 
> are now fixed.
> 
> 
> Thanks,
> 
> David Jarvie
> 
>

Re: Review Request 129709: Fix checking for valid date entered

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/#review101613
---



I'd say this could really do with some autotests

- Albert Astals Cid


On Dec. 27, 2016, 5:46 p.m., David Jarvie wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129709/
> ---
> 
> (Updated Dec. 27, 2016, 5:46 p.m.)
> 
> 
> Review request for KDE Frameworks and John Layt.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Checks to determine whether an entered date is valid are wrong or missing in 
> various places in the code. This is due to:
> - not testing if a minimum or maximum date is set before comparing a date to 
> the minimum/maximum;
> - always rejecting new minimum/maximum dates if the other maximum/minimum 
> date is not set;
> - not testing dates for validity when set by the date picker or menu.
> 
> This results in the following bugs currently:
> - When an up/down arrow or page up/down key is pressed to change the date, it 
> is always considered invalid and the date is not changed.
> - When the DateKeywords option is set, the only date which is displayed in 
> the menu is "No Date".
> - setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
> currently set.
> - setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
> currently set.
> - resetDateRange() does nothing.
> 
> This patch fixes the above bugs.
> 
> Documentation comments in the header file are also improved.
> 
> 
> Diffs
> -
> 
>   src/kdatecombobox.h d9a20ca 
>   src/kdatecombobox.cpp ad1d085 
> 
> Diff: https://git.reviewboard.kde.org/r/129709/diff/
> 
> 
> Testing
> ---
> 
> Tested with KAlarm, including setting DateKeywords. The bugs described above 
> are now fixed.
> 
> 
> Thanks,
> 
> David Jarvie
> 
>

Re: Review Request 129709: Fix checking for valid date entered

[David Jarvie]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/
---

(Updated Dec. 27, 2016, 5:46 p.m.)


Review request for KDE Frameworks and John Layt.


Changes
---

Updated to fix (re)setMinimumDate(), (re)setMaximumDate() and to check dates 
for validity if entered via date picker or menu.


Repository: kwidgetsaddons


Description (updated)
---

Checks to determine whether an entered date is valid are wrong or missing in 
various places in the code. This is due to:
- not testing if a minimum or maximum date is set before comparing a date to 
the minimum/maximum;
- always rejecting new minimum/maximum dates if the other maximum/minimum date 
is not set;
- not testing dates for validity when set by the date picker or menu.

This results in the following bugs currently:
- When an up/down arrow or page up/down key is pressed to change the date, it 
is always considered invalid and the date is not changed.
- When the DateKeywords option is set, the only date which is displayed in the 
menu is "No Date".
- setMinimumDate() and resetMinimumDate() do nothing if no maximum date is 
currently set.
- setMaximumDate() and resetMaximumDate() do nothing if no minimum date is 
currently set.
- resetDateRange() does nothing.

This patch fixes the above bugs.

Documentation comments in the header file are also improved.


Diffs (updated)
-

  src/kdatecombobox.h d9a20ca 
  src/kdatecombobox.cpp ad1d085 

Diff: https://git.reviewboard.kde.org/r/129709/diff/


Testing
---

Tested with KAlarm, including setting DateKeywords. The bugs described above 
are now fixed.


Thanks,

David Jarvie

Review Request 129709: Fix checking for valid date entered

[David Jarvie]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129709/
---

Review request for KDE Frameworks and John Layt.


Repository: kwidgetsaddons


Description
---

In two places, the checks to determine whether an entered date is valid is 
wrong, due to not testing whether a minimum or maximum date is set before 
comparing to the minimum/maximum date. When a minimum and maximum date is not 
set, this results in the following bugs currently:
- When an up/down arrow or page up/down key is pressed to change the date, it 
is always considered invalid and the date is not changed.
- When the DateKeywords option is set, the only date which is displayed for 
selection is "No Date".

This fixes the checks so that if a minimum date is not set, the date is always 
treated as complying with the minimum, and the maximum check is similarly fixed.

Documentation comments in the header file are also improved.


Diffs
-

  src/kdatecombobox.h d9a20ca 
  src/kdatecombobox.cpp ad1d085 

Diff: https://git.reviewboard.kde.org/r/129709/diff/


Testing
---

Tested with KAlarm, including setting DateKeywords. The bugs described above 
are now fixed.


Thanks,

David Jarvie