release plan and request for merge

Elvis Stansvik elvstone at gmail.com
Thu Feb 23 10:57:56 GMT 2012


2012/2/23 C. Boemann <cbo at boemann.dk>:
> Hi all
>
> So the minisprint on undo in Words is over and we had success. I'm requesting
> merge of our branch but more on that below.
>
> Assuming we merge within a day or two, I propose we create a release branch
> one of the next days. And on next friday we tag an RC from that release
> branch. Then 3 weeks and tag the final.
>
> As for the merging of the text-undo-minisprint branch Pierre and I worked
> mostly in tandem so we have had constant peer review. However sitting so close
> we could be the victim of group-think so I would be glad if someone else could
> look through the branch. Creating a review request, can be done but since we
> have like moved everything it doesn't make much sense.

Glad to hear the sprint was a success.

I'm definitely not savvy enough with the code and don't know enough
about the problems you've been solving to do a thorough review, so I
just have a few nitpicks:

 const QUrl KoTextDocument::FrameCharFormatUrl =
QUrl("kotext://frameCharFormat");
+const QUrl KoTextDocument::ShapeControllerURL =
QUrl("kotext://shapeController");

s/URL/Url/

+        IndexGeneratorManager ,
+        FrameCharFormat ,
+        ShapeController

s/ ,/,/

-void KoTextEditor::deleteChar(MoveOperation direction,
KoShapeController *shapeController)
+void KoTextEditor::deleteChar(bool previous, KUndo2Command *parent)

Why change from enum -> bool for the direction here? IMHO enum tells a
richer story when you're looking at a random call deleteChar
somewhere, even though it'll just be two values.

+        d->updateState(KoTextEditor::Private::Custom,
i18nc("(qtundo-format)", "Insert Footnote"));
+    }
+    else {

"else" on same line as }. (Same in a few other places to which it has
probably propagated through cut/paste).

+    int inCustomCommand;

The name sounds like a bool but it's a counter of some sort (a
level?), can it be improved?

+        kDebug(32500) << "commandStack is now: " << commandStack.count();
+    }
+    else if (addNewCommand) {

else's in this construct on same line as {.

+                textData =
qobject_cast<KoTextShapeDataBase*>(oldParent->userData());
+            } else  if (container) {

s/  / /

Great work!
Elvis



More information about the calligra-devel mailing list