[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: KSpread devs, review?
From:       Tomas Mecir <mecirt () gmail ! com>
Date:       2009-04-17 14:55:12
Message-ID: 492258b10904170755i74d67828j4aba6f5a34d8a60a () mail ! gmail ! com
[Download RAW message or body]

Well, that code isn't mine, but Stefan isn't around, so I'll give it a shot ...

As far as I understand it, the affected class (TablePageManager) is
used to keep track of table shape positions when printing. Your
proposed fix disables addition of new shapes into the manager, which
effectively disables the whole class' functionality (whatever it is,
didn't read the code too closely).

I'm not quite sure, but this almost looks like a manifestation of some
problem in the TextShape ... what happens is this:
- TablePageManager instance is created within a TableShape instance,
with the TableShape as parameter - the parameter is stored as
d->master
- later on (when resizing and such), the TablePageManager (likes 85-98
of kspread/shape/TablePageManager.cpp) takes the TableShape's parent
and tries to locate the TableShape within its layout. This fails (line
96 sets masterIndex to -1), because for whatever reason the shape
isn't stored in the layout's list - I believe that the real bug is
this - the shape isn't added to the parent's layout somewhere during
loading.

The problem only happens for shapes loaded from a file - newly created
TableShapes can be resized correctly.

The following workaround fixes the crash, though not the actual problem:
Index: shape/TablePageManager.cpp
===================================================================
--- shape/TablePageManager.cpp  (revision 953054)
+++ shape/TablePageManager.cpp  (working copy)
@@ -94,6 +94,7 @@
     Q_CHECK_PTR(layout);
     const QList<KoShape*> textShapes = layout->shapes();
     const int masterIndex = textShapes.indexOf(d->master);
+    if (masterIndex < 0) return;  // huh?
     KoShapeContainer* const textShape =
dynamic_cast<KoShapeContainer*>(textShapes.value(masterIndex + page -
1));
     if (textShape) {
         TableShape* const shape = new
TableShape(d->master->columns(), d->master->rows());


/ Tomas

2009/4/17 Thomas Zander <zander@kde.org>:
> Hi,
>
> I got this bugreport; https://bugs.kde.org/show_bug.cgi?id=189620
>
> Mek was helpful to investigate and come up with a patch but didn't feel
> confident about the code, at least it does fix the crash, though ;)
>
> I'm willing to commit this to the 2.0 branch as this fixes a crasher thats
> kind of easy to hit. In fact, we need *some* sort of solution for the 2.0
> release.
> But as we don't know the code too well, I can fully imagine the patch being
> totally wrong and the beginning of the end of times ;)
>
> So, if a kspread dev can please review it and perhaps even come up with a
> better solution, please help us out.
>
>    Fixes: Resizing Table Shape Resulted in Crash
>    BUG:189620
>
> diff --git a/kspread/shape/TablePageManager.cpp
> b/kspread/shape/TablePageManager.cpp
> index 3920be7..3db8d6b 100644
> --- a/kspread/shape/TablePageManager.cpp
> +++ b/kspread/shape/TablePageManager.cpp
> @@ -79,7 +79,7 @@ void TablePageManager::insertPage(int page)
>  void TablePageManager::preparePage(int page)
>  {
>     // The first page is the master page, which always exists.
> -    if (page == 1) {
> +    if (page == 1 || page > d->pages.count()) {
>         return;
>     }
>
> --
> Thomas Zander
>
> _______________________________________________
> koffice-devel mailing list
> koffice-devel@kde.org
> https://mail.kde.org/mailman/listinfo/koffice-devel
>
>
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel

[prev in list] [next in list] [prev in thread] [next in thread]