View Issue Details

IDProjectCategoryView StatusLast Update
0003529ScribusInternalpublic2006-04-20 19:28
Reporterchristoph_s Assigned To 
PriorityhighSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Platformx86OSLinuxOS VersionSuSE 9.3
Product Version1.3.3 
Fixed in Version1.3.4cvs 
Summary0003529: Text frames in scrapbooks lose special characters
DescriptionA text frame stored in a scrapbook loses all special characters like umlauts when dragged to a page.
Steps To ReproduceCreate a text frame that contains special chars, add it to a scrapbook. Now drag it from the scrapbook somewhere on a page. Special characters are lost, instead, red squares are shown.
TagsNo tags attached.
Patch

Relationships

child of 0002215 closedjghali Audit and fix text encoding handling 

Activities

Tsoots

2006-03-29 21:34

reporter   ~0009564

Looks like scrapbook is storing sce files in the system encoding but loads them like they were UTF-8 (converting all the scrapbook sce files to UTF-8 fixes the issue).

christoph_s

2006-03-29 22:25

developer   ~0009565

>>>converting all the scrapbook sce files to UTF-8 fixes the issue

I hope this is meant as a hint for developers, not for users. Tell this an average user, and will turn his back on Scribus immediately ;)

Tsoots

2006-03-29 23:50

reporter   ~0009566

Your average user doesn't even know this place so they are safe from complicated workarounds.

ringerc

2006-04-02 06:32

reporter   ~0009587

[copied from mail so others' can see the explanation too]

I just noticed this fix, and had a look given that I have a bit of
experience bashing my head against encoding issues in Scribus. Having
had a look over the code, I thought I'd offer my comments.

In the following I'm just explaining what's going on. I don't mean to
sledge your work and your fix (hell, you did the work, I didn't).
However, I do think it's important to understand the situation here and
the full effect of what's going on.

Your fix actually relies on another bug elsewhere in Scribus - this is a
case where Scribus corrupts text when it loads it, then does something
else wrong later that balances out.

The crux of the problem in this case is that scrap.cpp uses the broken
loadText() routine from util.cpp . That's there only for backward
compatibility, and really needs to be gotten rid of once all uses of it
have been fixed. What it does is read some input byte-by-byte and shove
each byte into a QString as if it were a unicode code point. I'm sure
you can see how this just won't work for input where the characters
associated with those bytes don't match the unicode spec - which in
practice means anything except US-ASCII. UTF-8 will get badly corrupted,
while I think the latin- codes will usually survive (unless certain
characters with special meanings in UTF-16 are present).

In most places throughout the Scribus codebase, this mangled QString is
demangled by an implicit conversion back to a latin-1 byte string. Those
should NEVER EVER EVER happen - they're absolutely always dangerous, and
often wrong, since in reality the required string is usually either the
output of QString::utf8() or QString::local8Bit() - they're actually a
design flaw in Qt. Unfortunately I can't disable these implicit casts
without breaking a LOT of existing Scribus code that relies on them. All
that code needs to be audited to determine what it actually wants.

Consider that QString::fromUtf8 is defined as:
    
    static QString QString::fromUtf8 ( const char * utf8, int len = -1 )

ie it doesn't take a QString but rather a const char* . That's partly
because converting a QString from utf-8 makes no sense, a QString knows
its own encoding and is a true unicode text string. If you pass it a
QString anyway, the implicit conversion to const char * provided by
QString is enacted:
    
    QString::operator const char * () const

Which says "Returns ascii().". So what we've done is shoved a bunch of
bytes into a QString, then used the implicit conversion to ascii to get
them back out. Because Qt's ascii() actually does the same thing as
latin1() - passing high-bit-set characters through - this generally works.

Just to make things more fun, half the time the saved data is in fact in
a corrupt encoding that is corrupted again on loading in such a way as
to result in the original data. We often do things like encode utf-8
data as if it was latin-1, then decode it as if it was latin-1 later. Or
vice versa. Because of Qt's very permissive text encoding
implmementations, this usually works, but it's ugly, and the way it's
done (using implicit conversions) often breaks down in different
locales.

What we should be doing is just putting the bytes into a QByteArray or
QCstring in the first place, thus correctly representing the fact that
we don't know their encoding, it's "just a string of bytes".
QString::fromUtf8(...) then does the right thing presuming that the
input is in fact utf-8. The first step is to replace the use of:
    
    QString f;
    loadText(f);
with

    QCString f;
    loadTextRaw(f);

... then fix the code that uses 'f' to convert it into a QString with
the appropriate static method:
        QString::fromLocal8Bit(...)
        QString::fromUtf8(...)
        QString::fromLatin1(...)
depending on the encoding of the input text. Sometimes it's necessary to
fix the routines that write the data out too, since they're sometimes
writing corrupt text data to the file due to an invalid encoding
conversion.

The scrapbook routines are still doing very bad things with encodings,
and they may break in different locales. If you like I'm happy to go
through and attack that code. Alternately, if you find my explanation
above to be useful, feel free to hack on it yourself - what with my uni
and work time pressure I sure won't complain.

ringerc

2006-04-02 06:33

reporter   ~0009588

Re-opening as per my explanation below. Please let me know if you want to tackle it or if I should. I've done it before in other parts of the code, eg file load/save and in prefs, so I'm happy to do it. If you want me to do it just assign to me.

Tsoots

2006-04-02 09:01

reporter   ~0009591

We're using QString anyway couldn't we just reimplement the body of the loadText() with the current parameters and convert from QCString to QString safe way. This way we wouldn't need to hack all the places where loadText() is used. fromUTF8() was the evil one in my quick fix. Commenting it out solved the issue.

ringerc

2006-04-02 10:15

reporter   ~0009592

Last edited: 2006-04-02 10:21

The reason I didn't do loadTextRaw that way originally was that it's just not correct to use a QString for data of unknown encoding. In fact, it's not even possible - to produce a valid QString in all situations you *must* know the encoding of the data that you're loading into it. Without knowing the encoding, it's just a string of bytes with no meaning as text.

Now consider a case where the encoding of the input is unknown - perhaps because we need to see a header within the input data in order to determine the encoding. This is the case in many parts of Scribus, including the scrapbook and doc load/save where the name of the root element of the XML document tree (eg <SCRIBUSUTF8NEW>) is examined to determine the document encoding. As it happens that's dodgy - we should be using the <?xml version="1.0" encoding="utf-8"?> directive to set the encoding - but IIRC Qt3 doesn't support that directly, and we need to use the old way for backward compat anyway.

In a situation like one of the above, we must read at least part of the file, examine it to determine its encoding, and can only then convert it to a QString. There are several ways to do this:

- (a) Move the I/O into the routine that needs the information, so it can read part of the file, determine the encoding, and slurp in the rest. Doesn't really gain us much unless the code is able to benefit from the piece-by-piece data in other ways (eg aborting early on unexpected input instead of reading the whole file first).
- (b) Pass an "encoding detector" function to loadText(). Seems like unnecessary complication.
- (c) Have loadText() return a byte string, and let the caller do the encoding detection and conversion.

I took approach (c) and wrote loadRawText to do just that. It'd be reasonable to have the original loadText() take an encoding argument that tells it what encoding to treat the input data as having, but since in many cases (such as the scrapbook) we can't actually know what encoding it's in until after we've read it, I don't think that's useful.

Hope that explanation makes sense.

ringerc

2006-04-02 10:40

reporter   ~0009593

Note that where we _do_ know the encoding, we can usually just use a QTextStream or similar to do the job much more sensibly.

mhanski

2006-04-20 16:48

developer   ~0010187

confirming, since the issue is there, regardless who and how is going to take care of it:)

louisdesjardins

2006-04-20 18:20

developer   ~0010190

This actually affects all accented letters in French (so it must affect a lot of other languages, I presume).

The feature is hardly usable with text at the moment.

Suggestion: raise the priority level from normal to high.

Tsoots

2006-04-20 19:17

reporter   ~0010191

Is this still an issue? I thought I fixed this (at least it's working ok with my finnish chars).

christoph_s

2006-04-20 19:25

developer   ~0010192

Confirmed. Works in latest 1.3.3cvs and in 1.3.4cvs with umlauts and eszett.

Tsoots

2006-04-20 19:28

reporter   ~0010194

This one was for the scrapbook bug which is fixed now (even if it's a dirty hack). Use 0002215 for general encoding issues.

Issue History

Date Modified Username Field Change
2006-03-29 12:10 christoph_s New Issue
2006-03-29 21:34 Tsoots Note Added: 0009564
2006-03-29 22:25 christoph_s Note Added: 0009565
2006-03-29 22:37 cbradney Category - => Internal
2006-03-29 23:49 Tsoots Status new => assigned
2006-03-29 23:49 Tsoots Assigned To => Tsoots
2006-03-29 23:50 Tsoots Status assigned => resolved
2006-03-29 23:50 Tsoots Fixed in Version => 1.3.4cvs
2006-03-29 23:50 Tsoots Resolution open => fixed
2006-03-29 23:50 Tsoots Note Added: 0009566
2006-04-02 05:59 ringerc Relationship added child of 0002215
2006-04-02 06:32 ringerc Note Added: 0009587
2006-04-02 06:33 ringerc Status resolved => feedback
2006-04-02 06:33 ringerc Resolution fixed => reopened
2006-04-02 06:33 ringerc Note Added: 0009588
2006-04-02 09:01 Tsoots Note Added: 0009591
2006-04-02 10:15 ringerc Note Added: 0009592
2006-04-02 10:21 ringerc Note Edited: 0009592
2006-04-02 10:40 ringerc Note Added: 0009593
2006-04-20 16:48 mhanski Note Added: 0010187
2006-04-20 16:48 mhanski Assigned To Tsoots =>
2006-04-20 16:48 mhanski Status feedback => confirmed
2006-04-20 18:20 louisdesjardins Note Added: 0010190
2006-04-20 19:11 mhanski Priority normal => high
2006-04-20 19:11 mhanski Severity minor => major
2006-04-20 19:17 Tsoots Note Added: 0010191
2006-04-20 19:25 christoph_s Note Added: 0010192
2006-04-20 19:28 Tsoots Status confirmed => closed
2006-04-20 19:28 Tsoots Note Added: 0010194
2006-04-20 19:28 Tsoots Resolution reopened => fixed