Add QValidator to KTimeCombobox
Needs RevisionPublic

Authored by bednar on Jul 21 2017, 8:45 PM.

Details

Reviewers
cfeck
Summary

Seeing the "TODO" in KTimeComboBoxPrivate::timeFormatToInputMask, I decided to try and replace the input mask with a QValidator approach (as seen in KDatePicker).
This is a first iteration to see if there is interest and if the approach I took is acceptable.
Thanks for any comments.

Test Plan

As of now, compiled and only ran test suite. 10/10 pass.

Diff Detail

Repository
R236 KWidgetsAddons
Lint
Lint Skipped
Unit
Unit Tests Skipped
bednar created this revision.Jul 21 2017, 8:45 PM
cfeck added a subscriber: cfeck.Jul 22 2017, 7:37 AM

Several code style issues. Please run it through uncrustify-kf5.cfg (see kde-dev-scripts.git)

bednar updated this revision to Diff 17452.Jul 31 2017, 7:06 PM

Ran through uncrustify.
Found a regression in my patch : colon (":") wouldn't be shown anymore, as it is dependent on the input mask. So I left the input mask, and the QValidator works in addition to the input mask.

cfeck added a comment.Aug 3 2017, 9:54 PM

Are you sure the patch is complete? I remember there was code which was supposed to be replaced by the new validator.

src/ktimecombobox.cpp
608

This does not look right.

cfeck requested changes to this revision.Aug 3 2017, 10:06 PM
cfeck added inline comments.
src/ktimecombobox.cpp
33

Please use "const &" for all QTime and QString arguments.

34

Since "min" and "max" might be macros for some compilers, please rename to "minTime" and "maxTime".

37

setMinTime(const QTime &minTime);

It looks more verbose, but also more correct :)

42

Also here, m_minTime and m_maxTime.

76

const QTime time = ...

231

So the KTimeComboBox class already has m_min/maxTime members. Can the code be refactored that they do not need to be stored twice?

This revision now requires changes to proceed.Aug 3 2017, 10:06 PM
kfunk added a subscriber: kfunk.Aug 4 2017, 12:53 PM
kfunk added inline comments.
src/ktimecombobox.cpp
33

Here and below: nullptr can be used directly in KF5