Bug 398405 - Session planner copies RA/DEC to Scheduler without fractional part
Summary: Session planner copies RA/DEC to Scheduler without fractional part
Status: RESOLVED FIXED
Alias: None
Product: kstars
Classification: Applications
Component: general (show other bugs)
Version: git
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: TallFurryMan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-08 21:48 UTC by TallFurryMan
Modified: 2018-10-04 22:37 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description TallFurryMan 2018-09-08 21:48:10 UTC
System is configured in French nimbers, dates and currency amounts. This means the fractional separator is ',' and not '.'.

When using the Session Planner to copy objects to the Scheduler plan, RA/DEC coordinates are copied without the fractional part, and thus are wrong.
Comment 1 Jasem Mutlaq 2018-09-09 11:31:22 UTC
This is what is used:

raBox->setText(object->ra0().toHMSString(false, true));
decBox->setText(object->dec0().toDMSString(false, false, true));

So these function are not producing the correct results? Both above are using high-precision.
Comment 2 TallFurryMan 2018-09-10 05:59:29 UTC
This seems proper indeed. Issue must be elsewhere, let me provide a scenario.
Comment 3 TallFurryMan 2018-09-10 06:03:43 UTC
Issue might be related to QString::toDouble in dms::setFromString. Documentation in Qt11 doesn't mention any locale issue, so perhaps this is a version compatibility problem?
Comment 4 TallFurryMan 2018-09-11 06:41:59 UTC
I reproduced the issue is more serious than I thought. The bug appears (also?) in the .esl load procedure! Could this be a regression introduced by the copy&paste of coordinates? This was working properly during August.
Comment 5 TallFurryMan 2018-09-11 06:46:55 UTC
Coordinates end up "HH MM SS.SS" after conversion instead of "HHh MM' SS,SS"" in the RA/DEC boxes, and the "." character is not recognized properly, leading to offset'd positioning afterwards. With my locale, "." should be "," for the conversion to be successful.

Current code tree thus has loading of scheduler files broken.
Comment 6 Jasem Mutlaq 2018-09-11 21:12:57 UTC
I changed toDMSString in dms.cpp to this:

    QString dummy;
    char pm(' ');
    QChar zero('0');
    int dd = abs(degree());
    int dm = abs(arcmin());
    int ds = abs(arcsec());    

    if (Degrees() < 0.0)
        pm = '-';
    else if (forceSign && Degrees() > 0.0)
        pm = '+';

    if (machineReadable)
        return QString("%1%2:%3:%4").arg(pm)
                .arg(dd, 2, 10, QChar('0'))
                .arg(dm, 2, 10, QChar('0'))
                .arg(ds, 2, 10, QChar('0'));

    if (highPrecision)
    {
        double sec = arcsec() + marcsec() / 1000.;
        return QString("%1%2° %3\' %4\"").arg(pm)
                                         .arg(dd, 2, 10, zero)
                                         .arg(dm, 2, 10, zero)
                                         .arg(sec, 2, 'f', 2, zero);
    }

    return QString("%1%2° %3\' %4\"").arg(pm)
                                     .arg(dd, 2, 10, zero)
                                     .arg(dm, 2, 10, zero)
                                     .arg(ds, 2, 10, zero);


However, I am still getting decimal point even though I changed the language and then also changed the system locale and logged out then in again. I also briefly tried with QLocale().toString with same results. Can you find out what's wrong?
Comment 7 TallFurryMan 2018-09-12 06:29:50 UTC
Read some Qt docs, and found the problem: all of our arg() and number() calls basically ignore locale. To properly support that, we can use "%L1" instead of "%1" for a float number for instance, or use toString(), which uses the current locale but is not very flexible. That also raises the problem of portable XML files, in which we should better choose the right QLocale before reading and writing. Plenty of changes everywhere, apparently.
Comment 8 Jasem Mutlaq 2018-09-12 08:37:52 UTC
I actually tried %L and it had no effect.
Comment 9 TallFurryMan 2018-09-12 10:33:09 UTC
On my system, QString("%L1").arg(float_var) with float_var does produce "1,2", while QString("%1").arg(float_var) does produces "1.2".

I'm checking where all this should be used. I'll try to use the english locale when reading and writing XML files so we keep portability. We'll have interesting issues, as for instance with the float representing exposure duration that is used in capture file names.
Comment 10 TallFurryMan 2018-09-12 12:35:12 UTC
I think the issue has been there for a long time actually. Besides, it might evantually relate to the issue forum user alacant had.

Tested with target "NGC 281", created a scheduler job, saved, loaded XML file, coordinates are wrong and, while easy to overlook, the nature of the problem is relatively obvious. NGC 281 has a fractional part for the seconds value. The resulting coordinates are not too far, but still completely wrong.
Comment 11 TallFurryMan 2018-09-12 12:38:12 UTC
XML files are saved with the system locale, and loaded with no locale. This issue is in Scheduler and Capture modules. Besides, all control texts need rework to take locale into account properly.
Comment 12 TallFurryMan 2018-09-12 12:55:34 UTC
I'm planning to:
- replace atof calls used to read XML numbers, which use the system locale, with QLocale::C string-to-type functions.
- make sure QString calls used to write XML numbers use the QLocale::C serialization.

Side not-so-related question: XML serialization is using the lilXML library, any plan to use QXMLStreamReader and QXMLStreamWriter at some point? This is compatible back to Qt4.3 and could possibly simplify some serialization code. In any case I think saving/loading configurations should be moved out of their related modules first.
Comment 13 Jasem Mutlaq 2018-09-12 15:34:23 UTC
I believe lilXML only supports C locale but I could be wrong. QXMLStreamWriter could be used to replace any use of lilXML in KStars. It was just that lilXML was easier to use since it was in INDI, and I don't believe QXMLStreamWriter was there back in 2003 when this was started.
Comment 14 TallFurryMan 2018-09-13 12:52:36 UTC
Preliminary work in the Capture module at https://github.com/TallFurryMan/kstars, branch improve__support_decimal_locales.
Comment 15 TallFurryMan 2018-09-14 06:18:51 UTC
One first step towards fixing this is in https://phabricator.kde.org/D15492
Comment 16 TallFurryMan 2018-10-04 22:37:51 UTC
Scheduler fixes are in https://phabricator.kde.org/D15865