[Kst] Concerns with current copy implementation

[George Staikos]

On Tuesday 11 January 2005 18:16, Andrew Walker wrote:
> 1) This isn't implemented yet. Copy and Paste would obviously
> not use the same functions as the menu item Copy.

   Then we have redundant actions, which means this implementation is wrong.

> 2) It depends on _parent as we have to set _parent. Once _parent
> is removed it can be removed here too.

 That statement isn't supported by this code:

+void KstPlotGroup::copyObject() {
+  KstApp::inst()->document()->setModified();
+  if (_parent) {
+    _parent->appendChild(new KstPlotGroup(*this), true);
+  }
+  QTimer::singleShot(0, KstApp::inst(), SLOT(updateDialogs()));
+}

> 3) DnD would be much the same as Copy and Paste.

   And DnD is already implemented and uses different mechanisms - the byte 
stream for remote dragging, and should use the duplicate() method I refer to 
for local dragging.

> 4) The "action" copy was never used, but if its going to be we can
> easily create a new method for the current copyObject()

   By action, I meant the "action of copying", not the KAction "copy".

> 5) Don't think I agree with this.

   What is there to disagree with if you still believe your comments in #2?  
You can't get access to _parent if the action is below _parent...  It is 
mandatory if there is no _parent.  Furthermore, copy is entirely a global 
action.  There is a clipboard and it is global.  Reimplementing copy in each 
object is redundant and unclean.

> 6) This seems to repeat 1)

   No, 6 is saying that simply not duplicating "immediately" is not enough, 
but we actually need an alternative implementation to the binary object too.

> The current implementation does what I set out to do. Make a quick
> copy of a plot which can then be moved to the desired location.
> As ever, once a new feature is in there are numerous proposals to
> how it could be better and/or bigger. My plan (as mentioned in the
> original bug report) was to also add Copy/Paste and DnD. This remains
> to be done and so the bug remains open (96256).

   And this is where the problem lies.  Hacking in a feature wherever it is 
easiest just to get the feature in is wrong.  It creates a maintenance 
problem and destroys the design and architecture that we already spent so 
much time on.  I designed the view objects architecture, I documented the way 
it works, and I even made suggestions before this latest code went in.  This 
code doesn't mesh with that architecture.  If you want or need to hack in 
features in an ad-hoc manner, that is what a branch is for (see: what was 
done for Planck in November).  The fact that you don't agree that the code in 
CVS fixes the bug even supports this.  It doesn't belong in CVS right now.  
If I committed everything I wrote into CVS before it was ready Kst would be 
very unstable.  It's not good behavior.  In fact, it stomps on a feature done 
by others - the design of view objects.  Killing one feature for the sake of 
saving time on another is equally wrong.

   Please, let's take design and collaboration seriously.  Kst has to last and 
has to be maintainable, and this is not the way to do it.

-- 
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/