Thursday, 26 November 2015
Sanitizing member variable names in LibreOffice Writer (Comments)
Robinson just branched off libreoffice-5-1 from master in LibreOffice’s core.git repository, so time to talk about what happened behind the scenes in the 5.0 → 5.1 development cycle from my side.
One stylistic detail that annoyed me for a while was the inconsistency around
naming class member variables. In new code it’s common to give them an
(or at least an
m) prefix, but in older code that wasn’t that common, and
various custom hacks were invented to differentiate between pointers which
point to the same memory address, but one being a parameter of a member
function, and the other being a member variable.
Probably the worst scenario is when one was an abbreviation of the other, like pTable and pTbl or pCursor and pCrsr. I took this as an opportunity to play with Clang’s LibTooling, and I wrote two tools back during the Cambridge hackfest to automate the process of finding and fixing missing prefixes.
To scope the renaming, I changed all classes in the
sw module having more
than 20 unprefixed members to follow the above convention, hopefully this
nicely improves code readability, together with the
of pointless abbreviations, also done before the branch-off, so affecting
both libreoffice-5-1 and master. :-)
Both LibreOffice Writer and Impress has the ability to export the document as
PNG, which is one way to create thumbnails for documents — i.e. being able to
preview them before the real loading of the document happens. It turns out
Calc did not have this feature, and given that
ScModelObj also supports the
css::view::XRenderable interface (just like Writer), I hoped that it won’t
be too complex to add one.
DocumentToGraphicRenderer, so that it can handle that Calc does
not implement the
text::XTextViewCursorSupplier interface (Writer uses
this one to expose the cursor is on what page)
png_Portable_Network_Graphic as filter type for Calc
create a new
calc_png_Export filter fragment
register a Calc graphic filters configuration type and filter group in
If you can’t wait till LibreOffice 5.1 is released to try out this new feature, you can get a daily build. :-)
While LibreOffice’s Android port has as good support for ODF as the desktop version, flat ODF was lagging behind. There were two reasons for this:
flat-ODF-related services were simply missing from the .apk, for historical reasons: when the .apk limit was 50MB, it sounded like a good idea to leave these out if it allowed packaging e.g. editing-related services.
flat ODS was blacklisted explicitly, from the times when Calc tiled rendering was missing, and when .ods was included again the document browser, .fods was forgotten.
With these two fixes my favorite test documents (of which the ODF version worked already previously) in flat ODF version also load properly in all Writer, Calc and Impress — as can be seen on the above montage of screenshots.
This use-case came up during this year’s LibreOffice conference. I started the Impress slideshow a few minutes in advance, so the audience could check if they are at the correct room, seeing the title side. Right after starting the slideshow, the presentation timer also started to count. I had to stop and start the presentation, so I had an idea how much time my talk took so far. The side effect of this workaround was that the audience saw an ugly short flash while the presentation was not running at the moment I started talking.
Because hacking the presenter console requires you to have two monitors, it’s not something I looked into during the conference’s hackfest, but now here it is: I added a button to just restart the timer, then it’s not needed to stop/start the presentation manually.
The icon is just a placeholder for now, the real design is in progress.
(via Andras Timar)
In this post I want to go further, and after touching Calc, here is my first story about Base. Of course I lie, it seems that Base forms are kind of read-only Writer documents, at least that’s how I could fix the bug I’m describing here. :-) The topic is still the same, rounding rough edges after the fill attributes rework, this time a stretched background of a Base form. (Which was not stretched at all, it was simply centered both horizontally and vertically on the page.)
I’m not entirely sure how the document from tdf#92586 is created, but the interesting part from styles.xml is:
<style:page-layout-properties ... style:repeat="no-repeat"> <style:background-image ... style:repeat="stretch"/> </style:page-layout-properties>
So the schema allows specifying the way how the bitmap is repeated at two places, and in the past the later attribute won, and with the rework this got lost.
An additional problem that the commit had to solve is that the attribute on the page-layout-properties XML element is mapped to our FillBitmapMode API, while the attribute with the same name on the background-image XML element is mapped to the BackGraphicLocation API, which is specific to Writer. As a result, now there is a compatibility map from the old enumeration to the new one, that’s how the bug document is now rendered the same again, as in the past.
The fix is already backported to the
I think this is my first Calc bugfix. :-) The problem I wanted to fix is that while LibreOffice 4.4 learned advanced fill attributes (gradients, hatches, etc) for page headers / footers in Writer, this broke the saving of simple graphic header backgrounds in Calc. Seeing that no-one stepped up to fix this, I tried to do this myself — and luckily the problem was in the ODF export filter, which is much more familiar to me, compared to Calc core.
Part of that larger feature was changes to the ODF filter, and the bug was exactly about touching shared ODF filter code to please Writer without testing other LibreOffice applications.
The actual problem was overlapping constants: as in multiple constants had the same numeric value. Such issues are sometimes hard to track down, in this case it wasn’t that hard: the context filter that tried to make sure we don’t write duplicated XML attributes removed the background property when it tried to guard header repeat offsets.
Given that this affected the LibreOffice 4.4 and 5.0 series, both branches got a backport of the commit, and so the next release from those lines will have the fix.
If you are into C++ programming, you probably know that smart pointers are not just literally strange things like the above ones. ;-) LibreOffice 5.0 got VclPtr, which is smart pointer specialized to VCL’s needs.
Such refactorings are good things, except that there is no huge rework without regressions. In this case the WMF filter had 3 places where we were leaking VirtualDevices due to a misconversion to the new VclPtr API. The problem was that the document had 135 images, WMF files to be exact. Now given that the leaks were during parsing of WMF records (a WMF file consists of multiple WMF records), at the end we leaked 8884 VirtualDevices. At first the problem was seen as Windows-specific, as at least X on Linux has no problem with creating that many VirtualDevices, but Windows' default resource limits are hit in this case.
A note about testing this bugfix effectively, so it never happens again. The problem was that I wanted to create a minimal reproducer, but I also needed a document with lots of WMF images, each complex enough to trigger the resource limit. At the end I manually created a DOCX file that had the same image copy&pasted multiple times: that way we really imported them multiple times (normally we notice that they’re the same, and only save the image to the file once, and put multiple references to it), and because DOCX is also a ZIP container, the test file can be still only 99KB instead of the original 17MB RTF.
Thanks to the 4.4 → 5.0 Windows bibisect repo, it was immediately obvious that this is a VclPtr problem, and then it was possible to identify the root cause, and finally see that the bug title mentioning RTF was just container of the WMF images in this case, the problem had nothing to with with RTF and the leak wasn’t Windows-specific, either.
If you ever used the mail merge wizard with a Calc data source, then you know how it worked in the past: you’ve got 3 files: the .odt mail template, the .ods data source and a .odb data source definition that defines how to access the .ods.
The target of this LHM-funded project was to get rid of the .odb file and just embed it into the .odt mail template. Why?
Here is the problem description from a user’s point of view: "When a mail merge document is being saved a separate database file is being automatically created. It links the Writer file to the spreadsheet (ODS, XLS, XLSX etc.) as data source. This additional 2kB ODB file confuses users, they might delete it without knowing to break the connection to the data source." An additional problem is that because the non-embedded data source definition is part of the user profile, you can’t just move the three files to an other machine, as .odb registration will be missing there.
If you are interested how this looks like, here is a demo (click on the image to see the video):
That’s it for now — as usual the commits are in master, so you can try this right now with a 5.1 daily build. :-)
What was problematic is that since C++11,
override is a valid keyword
after a member function declaration, and we have our
SAL_OVERRIDE macro that
to be able to use it before all our supported compilers recognize it.
Unsupported parsers include ctags, so if a member function have SAL_OVERRIDE,
ctags only indexed the definition, not the declaration.
The hope is that the later will go away in the long run, so it won’t really be a problem that ctags do not recognize that macro out of the box. :-)