Bug 409867 - KTimeComboBox may not allow valid time entry, depending on format (timeFormatToInputMask() too fragile)
Summary: KTimeComboBox may not allow valid time entry, depending on format (timeFormat...
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kwidgetsaddons
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.60.0
Platform: Other All
: NOR normal
Target Milestone: ---
Assignee: Christoph Feck
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-16 17:09 UTC by Philippe Cloutier
Modified: 2020-07-02 20:46 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Screenshot of KOrganizer dialog with 2 affected fields (92.93 KB, image/png)
2019-07-17 02:46 UTC, Philippe Cloutier
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Cloutier 2019-07-16 17:09:05 UTC
KTimeComboBox extends QLineEdit for time entry, using the inputMask property to set an input mask so users only need to type digits: https://doc.qt.io/qt-5/qlineedit.html#inputMask-prop
Typically, it is used to enter a short time (hours and minutes only). Very often, the format is "23:59". For that, the simple input mask "09:99" works. Very often though, the format is more complicated. For example, "11:59 PM". Unfortunately, as can be seen in the Formats panel of systemsettings (in Regional parameters), there are numerous short time formats possible. As a result, rather than having a mapping of each possible format to each corresponding input mask, KTimeComboBox uses a rather complicated algorithm implemented in KTimeComboBoxPrivate::timeFormatToInputMask(). Unfortunately, that algorithm is fragile (in my book, it's a hack), as the "TODO" at the top acknowledges.

Following the change of the fr_CA sublocale's short time format from the standard "23:59" to the much more particular "23 h 59", the generated input mask is "09 h 99", which is incorrect, since:
1. "h" is a special character for a position with a "Hexadecimal character permitted but not required". This position is therefore initialized to the blank character, which is " ". The field therefore has 7 characters, but on the third, "h" cannot be entered (only 0-9 and A-F).
2. If I understand correctly, the "blank character" being the space, all spaces are removed after input. This means the 3 consecutive spaces in the middle are removed, so if the field contains "12   34", the text value must be "1234", which is absolutely invalid. I am not entirely sure about this though, as the "11:59 PM" should also have a problem with the space, yet it works, and the documentation of QLineEdit's inputMask property is poor (I filed https://bugreports.qt.io/browse/QTBUG-77063 which requests improvement). 

One avenue to fix would be to change QLocale again and switch fr_CA back to "23:59", and this is under discussion: https://unicode-org.atlassian.net/browse/CLDR-8503
But this would only help fr_CA, and timeFormatToInputMask() should not be so sensitive in any case. On the KTimeComboBox side, as a minimum, a broken mask should never be returned.

One way this could probably be achieved is by escaping all characters from the initial mask. For this to work, it would need to be verified that if a backslash precedes a non-special character in QLineEdit's inputMask, the backslash is just ignored. So for example, if we start from "12:34 PM", this would be escaped to "\1\2\:\3\4\ \P\M".

But this would complicate the substitutions, which would need to replace escaped sequences (hours, minutes, seconds and AM/PM) by corresponding special characters. So for example, mask could evolve as follows:
12:34 PM
\1\2\:\3\4\ \P\M
09\:\3\4\ \P\M
09\:99\ \P\M
09\:99\ aa

This may cause issues with non-latin digits, but I must say that if the function is supposed to already support non-latin digits, I am not sure it does well.

I am filing this at normal severity, but I will file a more severe report against incidenceeditor, as the combination of issues creates a severe symptom for KOrganizer.
Comment 1 Philippe Cloutier 2019-07-17 02:46:30 UTC
Created attachment 121570 [details]
Screenshot of KOrganizer dialog with 2 affected fields

In this example, which shows a KOrganizer dialog in French Canadian, I demonstrated the bug by entering invalid times with a digit (above) and a letter (below) in a position supposed to contain a literal ("h").
Comment 2 Philippe Cloutier 2019-07-17 05:14:37 UTC
This exceeds this ticket's scope, but there is another big problem with timeFormatToInputMask(): in 12-hour systems (such as en_US, using format "11:59 PM"), it does not require the AM or PM characters, since the special characters used in the mask are lowercase "a"-s. This allows users of such formats to enter incomplete times such as "10:00", which will be quietly "saved" as midnight.

The special character for a mandatory letter is an *uppercase* "A".
Comment 3 Philippe Cloutier 2019-07-17 19:10:41 UTC
Ah, I forgot one point about the solution. I believe using the space character both as blank and as a literal part of the mask is bad. But except for using a different blank character (perhaps the underscore), I am not sure how that can be fixed with QLineEdit as it currently stands.
Comment 4 Philippe Cloutier 2020-06-30 03:14:56 UTC
Glen Ditchfield has proposed a very good-looking fix: https://invent.kde.org/frameworks/kwidgetsaddons/-/merge_requests/3
Comment 5 Albert Astals Cid 2020-07-02 20:46:48 UTC
Git commit 37c9ebe699e33f84700489982da301bdf8727bd3 by Albert Astals Cid, on behalf of Glen Ditchfield.
Committed on 02/07/2020 at 20:46.
Pushed by aacid into branch 'master'.

Fix KTimeComboBox for locales with unusual characters in formats

KTimeComboBox creates an edit mask by replacing the AM/PM indicator in
a formatted time with 'a' ("optional alphabetic") mask characters.
This does not work for locales that use punctuation in the indicator, such
as the current en_CA format "9:00 A.M.".  This patch uses mask character
'x' ("optional non-blank").

The current fr_CA time format contains an embedded 'h':  "9 h 00".  'h' is
an edit mask character ("optional hexadecimal digit"), so the generated
mask rejects properly-formatted times.  This patch escapes all characters
that are not supposed to be mask characters.

Credit to Filipus Klutiero for a detailed explanation of the errors.
```
Related: bug 405857, bug 406240, bug 410167, bug 411820, bug 412506, bug 420468
```

M  +52   -3    autotests/ktimecomboboxtest.cpp
M  +5    -0    autotests/ktimecomboboxtest.h
M  +26   -23   src/ktimecombobox.cpp

https://invent.kde.org/frameworks/kwidgetsaddons/commit/37c9ebe699e33f84700489982da301bdf8727bd3