Re: koffice

[Dmitry Kazakov-2]

On Tue, Sep 29, 2009 at 11:51 AM, Thomas Zander <zander@...> wrote:

SVN commit 1029197 by zander:

Inline the KoDocument::undoLastCommand, no need for a virtual method.

 M  +1 -1      krita/image/kis_undo_adapter.cc
 M  +0 -5      libs/main/KoDocument.cpp
 M  +0 -5      libs/main/KoDocument.h


--- trunk/koffice/krita/image/kis_undo_adapter.cc #1029196:1029197
@@ -78,7 +78,7 @@
     * command->undo() and remove the cache without committing it
     * to the undoStack
     */
-    m_doc->undoLastCommand();
+    m_doc->undoStack()->undo();
 }

Argh! =(
You are beaking encapsulation! Isn't this a basic principle of object-oriented programming?
There is an object (doc), and there is it's interface (undoLastCommand). And no calls to private members! (undo stack is private, right?) Why do you use private members?

These calls to privates are curse of our code... They make it simply unmaintainable... Just remember zooming subsys and PixelAspect button case...

I'm insisting on reverting this. I admit that this function may be 'non-virtual', i simply didn't manage to compile this as non-virtual. =(
 

 

 void KisUndoAdapter::setUndo(bool undo)
--- trunk/koffice/libs/main/KoDocument.cpp #1029196:1029197
@@ -2381,11 +2381,6 @@
        d->undoStack->push(command);
 }

-void KoDocument::undoLastCommand()
-{
-    d->undoStack->undo();
-}
-
 void KoDocument::beginMacro(const QString & text)
 {
    d->undoStack->beginMacro(text);
--- trunk/koffice/libs/main/KoDocument.h #1029196:1029197
@@ -790,11 +790,6 @@
    virtual void addCommand(QUndoCommand* command);

    /**
-     * Undo the command that is stored at the top of the stack
-     */
-    virtual void undoLastCommand();
-
-    /**
     * Begins recording of a macro command. At the end endMacro needs to be called.
     * @param text command description
     */

--
Dmitry Kazakov

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Thomas Zander-7]

On Tuesday 29. September 2009 13.15.54 Dmitry Kazakov wrote:
> Argh! =(
> You are beaking encapsulation! Isn't this a basic principle of
> object-oriented programming?
> There is an object (doc), and there is it's interface (undoLastCommand).
> And no calls to private members! (undo stack is private, right?) Why do you
> use private members?

The code is;

m_doc->undoStack()->undo();

undoStack is a public method, not a private member. There is no breaking of
encapsulation in this code.

--
Thomas Zander

Qt Developer Days 2009 | Save your seat - REGISTER NOW!
Munich, Germany: October 12th - 14th LAST CHANCE TO REGISTER!
San Francisco, California: November 2nd - 4th EARLY BIRD PRICING - SAVE $200!
For more info: http://qt.nokia.com/qtdevdays2009

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Dmitry Kazakov-2]

On Tue, Sep 29, 2009 at 2:28 PM, Thomas Zander <zander@...> wrote:

On Tuesday 29. September 2009 13.15.54 Dmitry Kazakov wrote:
> Argh! =(
> You are beaking encapsulation! Isn't this a basic principle of
> object-oriented programming?
> There is an object (doc), and there is it's interface (undoLastCommand).
> And no calls to private members! (undo stack is private, right?) Why do you
> use private members?

The code is;

m_doc->undoStack()->undo();

undoStack is a public method, not a private member. There is no breaking of
encapsulation in this code.

There is. The object, returned by undoStack(), is private. And you grant full access for it to the caller. This means, you grant "full access to a private member". And there is no difference whether it is granted through m_doc->m_undoStack or m_doc->undoStack().

What are the consequences of such an api:
1) Revealing of our internals. What if the implementer of a doc decided not to use QUndoStack for storing undos?

2) [most severe] KoDocument can't do any lock-management! This is a disaster. At the moment we have the same issue with KisImage::projection(). It is used from KisView thread without ANY locking. It causes very much artifacts due to races. But in KisImage's case it's much more difficult to do it another way.

--
Dmitry Kazakov

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Thomas Zander-7]

On Tuesday 29. September 2009 13.56.40 Dmitry Kazakov wrote:

> On Tue, Sep 29, 2009 at 2:28 PM, Thomas Zander <zander@...> wrote:
> > On Tuesday 29. September 2009 13.15.54 Dmitry Kazakov wrote:
> > > Argh! =(
> > > You are beaking encapsulation! Isn't this a basic principle of
> > > object-oriented programming?
> > > There is an object (doc), and there is it's interface
> > > (undoLastCommand). And no calls to private members! (undo stack is
> > > private, right?) Why do
> >
> > you
> >
> > > use private members?
> >
> > The code is;
> >
> > m_doc->undoStack()->undo();
> >
> > undoStack is a public method, not a private member. There is no breaking
> > of encapsulation in this code.
>
> There is. The object, returned by undoStack(), *is* private.

As soon as the public assessor was added, it stopped being private and thus
all results of it becoming public have to be taken. Good and bad.

The good thing is that you can use it and don't have to create new methods,
the bad is that KoDocuments public API states we use the QUndoStack, so we
can't stop using that.

I think its just fine to decide in our API that we use and will continue to use
the QUndoStack (or inherited). So I don't share your concerns and as we are
indeed not breaking encapsulation due to that prior decision, its fine to use
the code snipped pasted above.
--
Thomas Zander

Qt Developer Days 2009 | Save your seat - REGISTER NOW!
Munich, Germany: October 12th - 14th LAST CHANCE TO REGISTER!
San Francisco, California: November 2nd - 4th EARLY BIRD PRICING - SAVE $200!
For more info: http://qt.nokia.com/qtdevdays2009
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Dmitry Kazakov-2]

On Tue, Sep 29, 2009 at 3:09 PM, Thomas Zander <zander@...> wrote:

On Tuesday 29. September 2009 13.56.40 Dmitry Kazakov wrote:
> On Tue, Sep 29, 2009 at 2:28 PM, Thomas Zander <zander@...> wrote:
> > On Tuesday 29. September 2009 13.15.54 Dmitry Kazakov wrote:
> > > Argh! =(
> > > You are beaking encapsulation! Isn't this a basic principle of
> > > object-oriented programming?
> > > There is an object (doc), and there is it's interface
> > > (undoLastCommand). And no calls to private members! (undo stack is
> > > private, right?) Why do
> >
> > you
> >
> > > use private members?
> >
> > The code is;
> >
> > m_doc->undoStack()->undo();
> >
> > undoStack is a public method, not a private member. There is no breaking
> > of encapsulation in this code.
>
> There is. The object, returned by undoStack(), *is* private.

As soon as the public assessor was added, it stopped being private

It's just a workaround that hides broken logic of the object.
 

and thus
all results of it becoming public have to be taken. Good and bad.

The good thing is that you can use it and don't have to create new methods,

Are you that lazy to create a couple of methods? And allow "spaghetti code" instead?
 

the bad is that KoDocuments public API states we use the QUndoStack, so we
can't stop using that.

Why not just derive KoDocument form QUndoStack? Like

class KoDocument : QUndoStack {
...
};

It would make an api clearer and more maintainable. Example, if you decide to change undo() method one day (e.g. add locking), you will just need to overload one virtual method instead of overloading the whole QUndoStack and changing all appropriate constructors in KoDocument.



 

I think its just fine to decide in our API that we use and will continue to use
the QUndoStack (or inherited). So I don't share your concerns and as we are
indeed not breaking encapsulation due to that prior decision,

Please take a look at this:
http://en.wikipedia.org/wiki/Object-oriented_programming#Fundamental_concepts_and_features

"Method - an object's abilities. In language, methods (sometimes referred to as "functions") are verbs. Lassie, being a Dog, has the ability to bark."

In our case, KoDocument do not have any ability "to UndoStack". It is not a verb even.

More than that, usually, return values of the functions are considered as "out-values". But the construction above uses them as an 'in-value'.

 

its fine to use
the code snipped pasted above.

Well, zooming subsystem has already become unusable because of the access to private members (managers have cyclic links to each other). That's why it has hacks now. Do you really want the same fate for KoDocument? "API design matters" [0]


Btw, if you still insist on your method, please answer a question: how are you going to add locking for undo() operations in the future? For example, one day you will need to acquire "BigSomeStuffLock" on every undo. How are you going to do this?


[0] - http://mags.acm.org/communications/200905/?pg=48&pm=2
--
Dmitry Kazakov

_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Tuesday 29 September 2009, Dmitry Kazakov wrote:

> It's just a workaround that hides broken logic of the object.

There are a lot more things broken in koffice's
KoMainWindow/KoView/KoApplication/KoDocument design -- mostly because for ten
years or longer stuff has accreted. That's the way software development in the
real world goes. You design something clean, it grows, accretes rubbish and
then you have to refactor. Nobody's code is safe from that.

Fortunately, the Oslo sprint is coming up, and that's a good moment to check
the warts in komain's api :-)

--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop