Review Request: load and save draw:id and text:id also as xml:id
Boudewijn Rempt
boud at valdyas.org
Sun Feb 26 15:51:22 GMT 2012
> On Feb. 26, 2012, 4:16 a.m., Thorsten Zachmann wrote:
> > libs/flake/KoShape.cpp, lines 1576-1577
> > <http://git.reviewboard.kde.org/r/104074/diff/2/?file=51062#file51062line1576>
> >
> > According to ODF 19.187.2 <draw:glue-point> does not support xml:id. Please revert this change. It does not reference a different object but is a number and therefore it is only draw:id.
> >
> > I just see that it also wrongly writes out an xml:id. Maybe when you are on it can you please remove this.
> >
Well, 19,187 also says:
The draw:id attribute specifies identifiers for draw elements other than <draw:glue-point>.
OpenDocument consumers shall ignore a draw:id attribute if it occurs on a draw element with
an xml:id attribute value.
OpenDocument producers may write draw:id attributes for any draw element in addition to an
xml:id attribute.
The value of a draw:id attribute shall equal the value of an xml:id attribute on the same
element.
The draw:id attribute is deprecated in favor of xml:id. 19.914
So, I'd say that saving xml:id is the right thing to do; for loading, KoElementReference can load draw:id just fine.
> On Feb. 26, 2012, 4:16 a.m., Thorsten Zachmann wrote:
> > libs/flake/KoShapeSavingContext.cpp, lines 152-158
> > <http://git.reviewboard.kde.org/r/104074/diff/2/?file=51066#file51066line152>
> >
> > I think this should be changed to
> >
> > Q_ASSERT(!counter || counter && !prefix.empty());
> > which can be shorten to
> > Q_ASSERT(!counter || !prefix.empty());
> >
> > As then there is no useless if in release mode.
Though it's okay to have a prefix when not using the counter.
> On Feb. 26, 2012, 4:16 a.m., Thorsten Zachmann wrote:
> > libs/flake/KoShapeSavingContext.cpp, lines 207-208
> > <http://git.reviewboard.kde.org/r/104074/diff/2/?file=51066#file51066line207>
> >
> > If you use iterators one lookup into the map can be avoided.
I prefer this way... It's easier to read, and I don't think the performance difference will be noticeable.
> On Feb. 26, 2012, 4:16 a.m., Thorsten Zachmann wrote:
> > stage/part/animations/KPrAnimationBase.cpp, line 93
> > <http://git.reviewboard.kde.org/r/104074/diff/2/?file=51109#file51109line93>
> >
> > Setting the parameter shape is a bit misleading her as it does not read anything that is a shape here.
I addewd a new method existingXmlid to KoShapeSavingContext which makes the meaning here much clearer.
> On Feb. 26, 2012, 4:16 a.m., Thorsten Zachmann wrote:
> > libs/kotext/opendocument/KoTextWriter_p.cpp, lines 711-748
> > <http://git.reviewboard.kde.org/r/104074/diff/2/?file=51090#file51090line711>
> >
> > This looks somewhat suspicious. I don't know the RDF stuff well enough to be sure but should it not check if there is a inlineRdf and add a xmlid in that case or is that done in this code
no, this is fine; if the saveOdf method takes the xml id that was generated and will later do the translation table thingie.
> On Feb. 26, 2012, 4:16 a.m., Thorsten Zachmann wrote:
> > libs/flake/KoShapeSavingContext.cpp, line 166
> > <http://git.reviewboard.kde.org/r/104074/diff/2/?file=51066#file51066line166>
> >
> > It is not needed to check that the referent is not in yet as it is already checked some lines above.
I moved the insert bool away and created a new method for the don't-insert case.
- Boudewijn
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104074/#review10908
-----------------------------------------------------------
On Feb. 25, 2012, 3:12 p.m., Boudewijn Rempt wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104074/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2012, 3:12 p.m.)
>
>
> Review request for Calligra.
>
>
> Description
> -------
>
> draw:id and text:id are deprecated and to be replaced by xml:id. This patch creates a new class KoElementReference that encapsulates loading and saving all three tags, as well as automatically generating a unique id. This is used to make sure that if two items want to save an xml:id on the same element, we only have one, unique tag saved.
>
> I'm not completely happy yet, since parts of KoShapeSavingContext still create numbered, prefixed id's with hidden meanings that are not clearly connect to what the idref actually links to for the master pages. Imo, xml:id should always be guaranteed unique and there should be a clear code-path between the entity that is linked to from an element and the element itself.
>
>
> This addresses bug 288644.
> http://bugs.kde.org/show_bug.cgi?id=288644
>
>
> Diffs
> -----
>
> filters/stage/kpr2odf/Filterkpr2odf.cpp df7079b
> filters/stage/pptx/PptxXmlSlideReader.cpp eaee384
> filters/words/docx/DocxXmlDocumentReader.cpp 6aa31f5
> filters/words/oowriter/ExportFilter.h dea56bd
> filters/words/oowriter/ExportFilter.cc ff44e0c
> karbon/common/KarbonDocument.cpp 6057a18
> krita/ui/flake/kis_shape_layer.cc 8aaca9d
> krita/ui/flake/kis_shape_selection.cpp 95ebeb6
> libs/flake/KoConnectionShape.cpp fda3463
> libs/flake/KoImageCollection.cpp 27b8260
> libs/flake/KoShape.cpp be34b8a
> libs/flake/KoShapeLoadingContext.h 87b0830
> libs/flake/KoShapeLoadingContext.cpp 0573ddf
> libs/flake/KoShapeSavingContext.h c73a680
> libs/flake/KoShapeSavingContext.cpp 46cb477
> libs/kopageapp/KoPADocument.cpp 49ac34a
> libs/kopageapp/KoPAMasterPage.cpp 42e9065
> libs/kopageapp/KoPAPage.cpp fbcf49d
> libs/kopageapp/KoPAPastePage.cpp 9fd795e
> libs/kopageapp/tests/TestPACopyPastePage.h 363f803
> libs/kopageapp/tests/TestPACopyPastePage.cpp 3b023b8
> libs/kotext/KoDocumentRdfBase.h d6f467a
> libs/kotext/KoDocumentRdfBase.cpp 7b6b38c
> libs/kotext/KoInlineNote.h 91823e9
> libs/kotext/KoInlineNote.cpp 897214c
> libs/kotext/KoInlineObject.h 8a1d627
> libs/kotext/KoTextAnchor.cpp 414b347
> libs/kotext/KoTextBlockData.h cc1528b
> libs/kotext/KoTextBlockData.cpp 116b432
> libs/kotext/KoTextDrag.cpp 381f4de
> libs/kotext/KoTextInlineRdf.h fd7bb5e
> libs/kotext/KoTextInlineRdf.cpp f449426
> libs/kotext/changetracker/KoChangeTracker.h 33c2696
> libs/kotext/changetracker/KoChangeTracker.cpp 51b9c5e
> libs/kotext/opendocument/KoTextLoader.cpp 3ea7109
> libs/kotext/opendocument/KoTextWriter.h fd0689c
> libs/kotext/opendocument/KoTextWriter.cpp 56125d3
> libs/kotext/opendocument/KoTextWriter_p.h c89e895
> libs/kotext/opendocument/KoTextWriter_p.cpp 1dbc8bb
> libs/kotext/opendocument/tests/CMakeLists.txt c386cfb
> libs/kotext/opendocument/tests/TestChangeTracking.cpp 665636b
> libs/main/rdf/KoDocumentRdf.h fabc628
> libs/main/rdf/KoDocumentRdf.cpp bfaa9ec
> libs/odf/CMakeLists.txt 838d4f6
> libs/odf/KoElementReference.h PRE-CREATION
> libs/odf/KoElementReference.cpp PRE-CREATION
> libs/odf/KoEmbeddedDocumentSaver.cpp 36506ca
> libs/odf/KoGenChange.cpp 2cd2d97
> libs/odf/KoGenChanges.h d7b51ee
> libs/odf/KoGenChanges.cpp b64d32d
> libs/odf/tests/CMakeLists.txt ee86038
> libs/odf/tests/TestKoElementReference.h PRE-CREATION
> libs/odf/tests/TestKoElementReference.cpp PRE-CREATION
> plan/libs/ui/reports/odt/KoSimpleOdtCheckBox.cpp 79ac92c
> plan/libs/ui/reports/odt/KoSimpleOdtLine.cpp 6cddad2
> plan/libs/ui/reports/odt/KoSimpleOdtPicture.cpp 20aee62
> plan/libs/ui/reports/odt/KoSimpleOdtTextBox.cpp 717b867
> stage/part/animations/KPrAnimationBase.cpp b544485
> tables/part/Doc.cpp 25e3fd3
> words/part/KWOdfLoader.cpp af269ab
> words/part/KWOdfWriter.cpp b998485
>
> Diff: http://git.reviewboard.kde.org/r/104074/diff/
>
>
> Testing
> -------
>
> Manual testing and unittests; on Tuesday I will do a practical test at SKF.
>
>
> Thanks,
>
> Boudewijn Rempt
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20120226/33731b0d/attachment.htm>
More information about the calligra-devel
mailing list