Fixing display of user defined variables of type date (and time) , need help

[Sebastian Sauer]

On 10/16/2012 03:24 AM, Friedrich W. H. Kossebau wrote:
> Am Freitag, 28. September 2012, 14:21:33 schrieb Sebastian Sauer:
>> Aloha Friedrich and * :)
>>
>> On 09/27/2012 11:49 PM, Friedrich W. H. Kossebau wrote:
>>> Hi,
>>>
>>> I looked into why date variables are always only displayed with the
>>> default
>>> format and found that the error lies in that
>>> KoOdfNumberStyles::format(...)
>>> expects the value for date (and seems also time) to be a number in string
>>> form:
>>> --- 8< ---
>>> QString format(...)
>>> {
>>> [...]
>>>
>>>            case Date: {
>>>            
>>>               bool ok;
>>>               int v = value.toInt(&ok);
>>>               return ok ? formatDate(v, format.formatStr) : value;
>>>
>>> [...]
>>> }
>>> --- 8< ---
>>>
>>> Problem now is that DateVariable keeps the value as date string around and
>>> not in that single number string version (from what I understood, no yet
>>> the complete picture).
>> Indeed and it makes sense cause the variableManager()->value() is used
>> direct for display
> Hm, is it really used directly for display?

I was referring here to Kovariable/KoVariableManager like used e.g. in 
calligra/plugins/variables/PageVariable.cpp

     if (property == KoInlineObject::PageCount) {
         KoOdfNumberDefinition defaultDefinition; // FIXME Should fetch 
from pagestyle
         QString newValue = value.toInt() >= 0 ? 
m_numberFormat.formattedNumber(value.toInt(), &defaultDefinition) : 
QString();
         setValue(newValue);
     }

See also calligra/libs/kotext/KoVariableManager.h

     /**
      * Set or create a variable to the new value.
      * @param name the name of the variable.
      * @param value the new value.
      * @param type The type of the value. This is only set for 
user-defined variables.
      * For non user defined variables the value is empty. If set then 
it should be one
      * of the following values;
      * \li string
      * \li boolean
      * \li currency
      * \li date
      * \li float
      * \li percentage
      * \li time
      * \li void
      * \li formula
      */
     void setValue(const QString &name, const QString &value, const 
QString &type = QString());

That means for all non UserVariables (like date/time/etc) setValue is 
called with the final string to display. The KoVariable itself has no 
information how that strting got actually produced (formatings, 
datastyles, etc).

That means anyone who tries to use KovariableManager->value("VarName") 
gets a string and that's it. The original information (like format, etc) 
is more or less lost. In rea;ity its not since the variables themself 
remember those original things, save it back, allow editing e.g. the format.

The problem raises if, like in the case of a UserVariable, something 
else then the variable plugin itself tries to do something more with the 
variable's value then only displaying it. In the case of the 
user-variable it allows to reference another variable (like a date/time 
variable), take the value there and reformat it with another format for 
display. Kovariable/KoVariableManager does not allow that.

>  From how I understood meanwhile
> the existing code the idea of that UserVariable is that is takes the original
> data value from the variable manager (which keeps the data as found in the
> ODF document with <text:user-field-decl>)

calligra/plugins/variables/DateVariable.cpp

void DateVariable::update()
{
     ...
     switch (m_displayType) {
     case Time:
         if (m_definition.isEmpty()) {
             setValue(target.time().toString(Qt::LocalDate));
         }
         else {
             setValue(target.time().toString(m_definition));
         }
         break;
     case Date:
         if (m_definition.isEmpty()) {
             setValue(target.date().toString(Qt::LocalDate));
         }
         else {
             setValue(target.toString(m_definition));
         }
         break;
     }
}

setValue() is called with the final product (means the string to 
display) after applying formatings and stuff. The whole original 
information like m_definition and the target QDateTime is never passed 
out of the DateVariable and hence noone except the DateVariable can use 
the original information.

> and creates the actual text string
> to display by applying the format set for the UserVariable (as found in the
> data-style referred to by the "style:data-style-name" attribute <text:user-
> field-get>/<text:user-field-input>).

Its actually a multilevel step.

In the DatePlugin we do:
1) DateVariable+DateFormat => DateString

In the UserVariable we do:
2) DateString+OtherDateFormat=>NewDateString

Note that in 1) we only pass the resulting DateString to the 
KoVariable::setValue and KovariableManager. In 2) the UserVariable 
fetches that DateString and tries to make it into NewDateString what 
fails cause what the UserVariable would need to do is:
3) DateVariable+OtherDateFormat=>NewDateString
where DateVariable is the input we get at 1) but that is including the 
DateFormat never ever made available to the outside world like for the 
UserVariable.

A quick a dirty way would be to just
QDateTime dt = parse(DateString);
QString newDateString = format(dt, otherDateFormat);
but that is error-prune and will be in some cases not possible. Better 
would be to just make it possible that the UserVariable can access the 
original DateVariable QDateTime from the DatePlugin and use it's own 
OtherDateFormat to produce the proper NewDateString.

The fundamental problem I see here is that KoVariable/KovariableManager 
was never designed to pass more in/on/getOut then a QString which 
represents the final string for display for the variable(s). In the case 
of UserVariables (and I am sure there are lot more cases) that is just 
not enough. What would be needed is the original date, the unformatted 
raw-value and/or in the case of the DatePlugin the QDateTime.

> And the current code also seems to not completely support the 1.2 version of
> the spec, where e.g. time and date are not stored as a float-value, but
> instead in the dateTime/date format as specified for xmlschema (the one
> similar to ISO 8601).

The code? You mean those;

DateVariable::loadOdf
...
// hopefully this simple detection works in all cases
const bool isDateTime = (value.indexOf(QLatin1Char('T')) != -1);


?

Or the UserVariable itself? The UserVariable itself should in the ideal 
case not need to deal with read/write formats since the DatePlugin does 
that already and also produces the proper QDateTime. But then there is 
certainly the use-case that a UserVariable may need to read/write a ODF 
cached value.....

> So I am currently creating a test document which both has variables declared
> following the 1.2 spec (from what I understand) as well as variables following
> that old OOo(?) way, to test also backward compatibility.
> Hopefully I can turn it into some automatic test in the end, for now manual
> testing is at least a start, to assist in fixing/completing the code.

Makes lot of sense. Especially extending the unittests. iirc the 
OdfNumberFormat parsing had quit already quit a few unittests but seems 
none of them tests exactly that problem. Well, iirc cause the problem is 
more down/up the stack at the place where we push/pull variables in...

>> but that opens the problem that it does not contain
>> the information what format was used to produce the date/time what makes
>> it rather hard to extract the original value (means translate it back)
>> to e.g. apply another format...
>>
>> I think for the load/save roundtrip this is not a problem cause we just
>> load, display and save it like it was but as soon as someone likes e.g.
>> during editing change the display format those formatted string value
>> needs to be translated to its QTime/QDate/QDateTime representation to
>> proper apply the format.
>>
>>> There is also this TODO (from you, Sebastian):
>>> --- 8< ---
>>> void UserVariable::valueChanged()
>>> {
>>>
>>>       //TODO apply following also to plugins/variables/DateVariable.cpp:96
>>>       //TODO handle formula
>>>       QString value = variableManager()->value(m_name);
>>>       value = KoOdfNumberStyles::format(value, m_numberstyle);
>>>
>>> kDebug() << m_name<<variableManager()->value(m_name) <<
>>> m_numberstyle.formatStr << value;
>>>
>>>       setValue(value);
>>>
>>> }
>> iirc that's what the TODO was referring to. In the DateVariable we
>> already proper parse the formatted value string to earn a
>> QTime/QDate./QDateTime representation that can then be used future.
>>
>> It's a special case which would needs to be handled in the UserVariable
>> where the original formatted string is reused with a new format *AND*
>> for editing where a user likes to change the format. Both is handled in
>> the DateVariable but not in the UserVariable.
>>
>>> --- 8< ---
>>>
>>> Are you working on this any time soon, Sebastian? Or could you point me
>>> into the direction how this TODO is planned to be solved, so I can give
>>> it a try?
>> There seems to be no way currently to make local members a
>> variable-instance has accessible to other variables. The KoProperties
>> are shared among all instances of a variable (iirc, or not?) and so the
>> easy way to just extend the DateVariable to proper write the in loadOdf
>> read m_definition into the KoProperties like
>> props->setProperty("definition", m_definition); is not possible.
>>
>> I think what would be needed is a generic "variableInstanceProperty" (or
>> since Kovariable inherits from QObject somebody could just use
>> QObject::setProperty) and then in loadOdf and on changing the
>> m_definition of a DateVariable someone could
>> QObject::setProperty("dateDefinition", m_definition) and in the
>> UserVariable extract that info using
>> QObject::property("dateDefinition").toString() and then something like
>> QDate date = parseDate(variableManager()->value(m_name),
>> variableManager()->variable(m_name)->property("dateDefinition")); or so...
>>
>> Maybe it (the parsing) could even be done a level up in the KoOdfNum*
>> itself? iirc there is some nice parsing code in
>> calligra/filters/sheets/xlsx/NumberFormatParser.cpp and there is also
>> quit some logic in the DateVariable which looks like something that may
>> of more common use (means e.g. also for other variables).
>>
>> All that untested and without looking at the code for a long while and
>> just now had my first looks after finally figuring out the git checkout
>> errors I had for a longer time got solved :) Hope it gives some ideas
>> and hope it helps to address that problem. Lot of thanks for looking
>> into it! :)
> Cheers
> Friedrich
>