Category: Development

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 does a developer fix 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:

1. Understanding the Bugs and QA
2. Developing a Bug Fix
3. Writing the Tests and Finishing the Task

This is the third part.

3. Writing the Tests and Finishing the Task

So, if you have read the two previous parts on “Fixing an Interoperability Bug: Missing Lines in Save and Load” (part 1, part 2), this is the third and last part of these series. In this part, we discuss how to write a test, submit the patch to the Gerrit and try to get it incorporated into the LibreOffice code.

3.1. Writing a Test for the Regression

In order to make sure that this regression will not happen again, we should create test for every bug that is fixed. If we don’t do this, there is a big chance that this bug will occur again and again.

But how to start writing a test? The answer is that there are ~3000 tests available in sw/qa/extras/ folder, and there is a tiny document sw/qa/extras/README that describes them. These tests can be a basis for writing a new test. For example, test for tdf#91687 consist of these lines:

DECLARE_WW8EXPORT_TEST(testTdf91687, "tdf91687.doc")
{ // Exported Watermarks were resized
 uno::Reference xWatermark = getShape(1);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(5172), xWatermark->getSize().Height);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(18105), xWatermark->getSize().Width);
}

It uses a macro DECLARE_WW8EXPORT_TEST too, that checks some properties of the document after loading it, and then again after saving and reloading it. If some property is erroneously changed, the test will fail, and you will get noticed.
As you can see, the name of the test, and the name of the test document in which here is .doc file, come from the bug number.

The test for this regression should check for the position and size of the shapes, so there are similarities between the our desired test and the test above. But how can we find the properties of the elements of the document? The answer is the new UNO object inspection tool for the LibreOffice:

https://blog.documentfoundation.org/blog/2021/03/02/update-on-tender-for-a-built-in-uno-object-inspection-tool-in-libreoffice/

As can be seen in figure 1, the size for Shape7 which is the first vertical line, is visible on the right.

In previous versions, tools called XRAY or MRI were used for object inspection.

Figure 1. UNO object inspection tool for the LibreOffice

The completed test comes below. It takes Shape7, Shape8 and Shape9 which are the three vertical lines, and ensures that they have the same size in the first load, and after save and reload. Without the fix, some of these test fail, which are size checking for the first and last vertical lines. Their size was reduced to ~0, so it is justifiable that previously they were erroneously disappearing after save and reload. The test passes after applying the fix.

DECLARE_WW8EXPORT_TEST(testTdf123321, "shapes-line-ellipse.doc")
{
 // These are the 3 lines in which 1st and 3rd one were
 // disappearing before
 uno::Reference<drawing::XShape> l1 = getShape(7);
 uno::Reference<drawing::XShape> l2 = getShape(8);
 uno::Reference<drawing::XShape> l3 = getShape(9);
 // first line (smallest)
 // Fails without the fix: Expected: 423, Actual: 2
 CPPUNIT_ASSERT_EQUAL(sal_Int32(423), l1->getSize().Height);
 // Fails without the fix: Expected: 0, Actual: 2
 CPPUNIT_ASSERT_EQUAL(sal_Int32(0), l1->getSize().Width);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(7908), l1->getPosition().X);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(37), l1->getPosition().Y);
 // second line (larger)
 CPPUNIT_ASSERT_EQUAL(sal_Int32(2542), l2->getSize().Height);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(2), l2->getSize().Width);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(7916), l2->getPosition().X);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(289), l2->getPosition().Y);
 // third line (largest)
 // Fails without the fix: Expected: 7027, Actual: 2
 CPPUNIT_ASSERT_EQUAL(sal_Int32(7027), l3->getSize().Height);
 // Fails without the fix: Expected: 0, Actual: 2
 CPPUNIT_ASSERT_EQUAL(sal_Int32(0), l3->getSize().Width);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(7911), l3->getPosition().X);
 CPPUNIT_ASSERT_EQUAL(sal_Int32(231), l3->getPosition().Y);
}

As you can see, not all the tests fail without the fix, but they are in place to protect the correct load and save of the shapes in the future.

3.2. Cleaning up and Submitting the Changes

After we finished the fix, we commit the changes, and submit it to the LibreOffice Gerrit.

$ git add include/svx/svdopath.hxx svx/source/svdraw/svdopath.cxx sw/qa/extras/ww8export/data/shapes-line-ellipse.doc sw/qa/extras/ww8export/ww8export2.cxx

$git commit

Let’s see our latest commit:

$ git show --name-only

Author: Hossein <hossein@libreoffice.org>
Date:   Mon Jul 19 13:03:42 2021 +0200

    tdf#123321 Fix DOC/DOCX export bug which made some lines disappear
    
    * Fix the wrong calculation of SdrPathObj (line) extent while exporting
      to .doc and .docx formats. This resolves tdf#123321 which cause some
      lines to disappear after save and reload. This fix does not break
      tdf#91687
    * added tests to make sure this does not happen again, based on the
      .doc file provided in Bugzilla
    * Loading the bad exported file in MS Word is OK, but it does not work
      in LibreOffice, and some of the lines will be hidden. This needs
      additional work.
    
    The regression came from d72e0cadceb0b43928a9b4f18d75c9d5d30afdda
    
    Change-Id: Ic5a7af4d29df741204c50590728542e674ee9f91

include/svx/svdopath.hxx
svx/source/svdraw/svdopath.cxx
sw/qa/extras/ww8export/data/shapes-line-ellipse.doc
sw/qa/extras/ww8export/ww8export2.cxx

The actual commit information is lengthy, but for making it shorter, we only listed the name of the changed or new files. Then, we submit the patch:

$ ./logerrit submit master

This is with the assumption that we have successfully configured Gerrit before. One should set up Gerrit before doing this. A step-by-step manual is presented in The Document Foundation Wiki.

https://wiki.documentfoundation.org/Development/gerrit/setup

3.3 Getting the Submission Merged

Every submission to the Gerrit is reviewed by one or more developers, and is possibly gets merged into to the code after the submitter has done all the requested fixes and improvement. This is done by the decision one of the people with commit access. Commit access is granted to the developers who had a good record of contributions and are trusted to change the code base directly.

So, the code is reviewed in the Gerrit:

https://gerrit.libreoffice.org/c/core/+/119116

Every submitted code is built automatically by Jenkins in a process called CI (continues integration). If the build is successful, you will get a     Verified +1     from Jenkins.

If, for any reason, one of the builds for several platforms that is tested fails, you will get     Verified -1     from Jenkins. Then, you will have to fix the problem and re-submit again. Please note that there are situations that some tests fail because of the Jenkins problem itself. If that is the case, you should ask people in the #libreoffice-dev irc channel to invoke a resume for you, or try rebasing to initiate a rebuild.

The original submission (patch set 1) lacked the tests, so a test was requested. After doing the improvements, including a change in the title and uploading seven patch sets in total, it was given the   Code Review +2   from the reviewer. Here, Miklos Vajna from Collabora has done the reviews and merging.

Then, the proposed changes eventually got merged into the code by the reviewer with appropriate access. After that, everyone can pull the changes from Git:

https://git.libreoffice.org/core/+/bda4d7a7c3fcc259e023f568606be5dcba818db9

3.4 Marking the Bug as Fixed

After the commit is done, the journey ends with marking the bug as fixed. Looking at the other possible registrations of the symptoms at TDF Bugzilla, it turns out that another bug can be considered a duplicate of this one:

Bug 127296 – FILESAVE: DOC Rotated line loses rotation when saved

https://bugs.documentfoundation.org/show_bug.cgi?id=127296

It is reported in 2019-09-02, and it is newer than the one we have fixed. So, we can mark tdf#127296 as a duplicate of this one. After that, we can safely change the status of tdf#123321 to “RESOLVED FIXED”, and we’re done!

3.5 Want to Get Involved?

If you want to get involved in Libreoffice development, you can help in fixing the bugs. Looking at the statistics from TDF’s Bugzilla, we see that there is a total of ~12k open bugs, in which ~1.3k of them are regressions. Among them, around 1.1k are bibisected. They are more suitable for getting into, because the root of the problem is known to some extent.

You can go through the same process that was discussed in the series of articles here to fix the bugs and submit your patch, and if you had any questions, #libreoffice-dev is a good place to ask. Also, it is suggested that you join the LibreOffice development mailing list. See the instructions here.

Hossein Nourikhah is the Developer Community Architect at the The Document Foundation (TDF), the non-profit behind LibreOffice. If you need assistance getting started with LibreOffice development, you can get in touch with him:

E-Mail: hossein@libreoffice.org

IRC: hossein in the IRC channel #libreoffice-dev on the Libera.Chat network ^{connect via webchat}

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:

1. Understanding the Bugs and QA
2. Developing a Bug Fix
3. 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:

• Linux and *BSD systems
• macOS
• on Windows
• for Android

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:

1. filter/source/msfilter/escherex.cxx
2. filter/source/msfilter/msdffimp.cxx
3. include/svx/msdffdef.hxx
4. sw/qa/extras/ww8export/data/tdf91687.doc
5. sw/qa/extras/ww8export/ww8export2.cxx
6. sw/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 ellipeses
• pObj 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;