By Hossein Nourikah, Developer Community Architect at The Document Foundation
In LibreOffice, interoperability is considered a very important aspect of the software. Today LibreOffice can load and save various file formats from many different Office applications from different companies across the world. But, bugs are inevitable parts of every software: There are situations where the application does not behave as it should, and a developer should take action and fix it, so that it will behave as it is expected by the user.
What if you encounter a bug in LibreOffice, and how a developer fixes the problem? In these series of articles, we discuss the steps needed to fix a bug. In the end, we will provide a test and make sure that the same problem does not happen in the future, again.
The article is presented in three parts:
- Understanding the Bugs and QA
- Developing a Bug Fix
- Writing the Tests and Finishing the Task
This is the second part.
2. Developing a Bug Fix
So, if you have read the previous part of “Fixing an Interoperability Bug: Missing Lines in Save and Load” (part 1), this is the sequel. Here we try to develop a solution for the problem with our knowledge of C++.
The first thing we need is to build the LibreOffice core. Depending on the platform, you can find the instructions in the TDF wiki.
See the instructions for building LibreOffice on:
2.1. Finding the Responsible Change
So, now we know that through this commit, these files were changed. One (or possibly all) of the changes is responsible for the regression. So we try to find a minimize set of changes that lead to the problem. We can use --name-only option from git:
$ git show d72e0cadceb0b43928a9b4f18d75c9d5d30afdda --name-only
So, the changed files are:
filter/source/msfilter/escherex.cxxfilter/source/msfilter/msdffimp.cxxinclude/svx/msdffdef.hxxsw/qa/extras/ww8export/data/tdf91687.docsw/qa/extras/ww8export/ww8export2.cxxsw/source/filter/ww8/wrtw8esh.cxx
Sometimes we have to go back to this specific commit using git checkout command to be able to work on the exact commit before the one that created the bug. But, most of the time, reverting the specific commit is easier, and the build will be much faster. Also, building older versions of LibeOffice is tricky. We can instead invoke this command:
$ git revert d72e0cadceb0b43928a9b4f18d75c9d5d30afdda
Now we should try to resolve the possible conflicts, then build the code and make sure the problem has gone away! Then we can try to re-introduce one or more changes to achieve a minimal set of changes that reproduces the problem.
We can start at the file level to find the relevant files to work on. Files 4 and 5 are related to the tests, so they should have no impact. File 1 and 2 seem to be related, as both are from filter/source/msfilter folder, file 3 is also related to these two files. After leaving out all the files, we see that changes from 6 (sw/source/filter/ww8/wrtw8esh.cxx) is enough to reproduce the problem. So, we have few lines of code changes in front of us:
@@ -756,7 +756,12 @@ void PlcDrawObj::WritePlc( WW8Export& rWrt ) const
OSL_ENSURE(pObj, "Where is the SDR-Object?");
if (pObj)
{
- aRect = pObj->GetSnapRect();
+ aRect = pObj->GetLogicRect();
+
+ // We have to export original size with padding
+ const SfxItemSet& rSet = pObj->GetMergedItemSet();
+ const SdrMetricItem* pItem = static_cast(rSet.GetItem(SDRATTR_TEXT_UPPERDIST));
+ aRect.SetSize(Size(aRect.GetWidth(), aRect.GetHeight() + pItem->GetValue()));
}
}
If we work further on this piece of code, we will find that the change from pObj->GetSnapRect() to pObj->GetLogicRect() is the source of regression: Good catch!
2.2. Creating a Fix
As described in the previous section, going back to pObj->GetSnapRect() fixes the problem, but wait! Isn’t that change supposed to fix a problem?
(GOOD)
(BAD)
Figure 1: Wrong increase of watermark size
after save and reload
If we go back to git master using:
git reset --hard HEAD^1
and then only change pObj->GetSnapRect() to pObj->GetSnapRect(), this test fails with the change. Try:
$ make CPPUNIT_TEST_NAME="testTdf91687" -sr CppunitTest_sw_ww8export2
If we take a look at the tdf#91687 in the Bugzilla, we see the example sw/qa/extras/ww8export/data/tdf91687.doc that contains a watermark (Figure 1) that gets bigger by save and reload! Looking more carefully to the changes, it becomes clear that rotation is important here. If we change the watermark example in order to set rotation to zero, nothing happens.
This is not limited to watermarks: Saving and re-loading any custom shape leads to such a wrong increase in size. On the other hand, this does not have any effect on the lines, rotated or not.
2.3. Fixing the Side Effects
In order to fix the undesired side effects, we should make a difference between the lines and the complex shapes. So, what is the difference? If we look closely to the code, we find out that pObj object is from SdrObj class, which is abstract DrawObject. The documentation for this class can be found here:
https://docs.libreoffice.org/svx/html/classSdrObject.html
Figure 2. Class hierarchy for SdrObject
So, objects from different SdrObject subclasses are sent here, and appropriate GetSnapRect() or GetLogicRect() is called. But which method? According to the runtime polymorphism, this is determined at runtime. So without debugging, nothing becomes clear.
By setting a break-point at this modified line and stepping into the GetLogicRect() function, it becomes clear that two types of object are created for the shapes:
pObj is SdrObjCustomShape (x4): representing 4 ellipesespObj is SdrPathObj (x5): representing 5 lines
So, the trick would be creating a GetLogicRect() method for SdrPathObj and make it invoke GetSnapRect(). In this way, lines and custom shapes are treated differently.
We declare GetLogicRect() in include/svx/svdopath.hxx:
virtual const tools::Rectangle& GetLogicRect() const override;
In the third part, we are going to talk about the steps needed to get our changes merged into the LibreOffice Code. If you want to get involved in LibreOffice development, it is suggested that you join the LibreOffice development mailing list. See the instructions here.
