Bug 96256

1) This isn't implemented yet. Copy and Paste would obviously
not use the same functions as the menu item Copy.
2) It depends on _parent as we have to set _parent. Once _parent
is removed it can be removed here too.
3) DnD would be much the same as Copy and Paste.
4) The "action" copy was never used, but if its going to be we can
easily create a new method for the current copyObject()
5) Don't think I agree with this.
6) This seems to repeat 1)

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).

-----Original Message-----
From: George Staikos [mailto:staikos@kde.org]
Sent: Tuesday, January 11, 2005 2:38 PM
To: kst@kde.org
Subject: [Kst] Concerns with current copy implementation



I'm a bit concerned about the design/implementation of the current copy
code.
It's good that it's in layout mode now, but I think the architecture is a
bit
off.  Here's why:

1) Copy&Paste is not usable with the new functions added since they
immediately do the copy and paste.
2) It depends too heavily on _parent, which we're trying to remove.  It can
be
implemented efficiently without it.
3) It would be useful for DnD too if implemented differently (as I
suggested,
something like KstViewObjectPtr duplicate() const;)
4) It mixes the "action" copy with the concept of copying a view object,
which
is abstract and reusable elsewhere.
5) Copy as an action should be toplevel.  There's no need to propagate it
lower.  Only duplication of object properties is object-implementation
specific.  I think Copy is a KstTopLevelView/KstViewWindow/KstApp action.
6) We actually need something more abstract for the concept of "copying"
since
we sometimes don't want to create objects or duplicate until the "paste"
happens.  This is particularly the case for DnD and traditional Copy+Paste.
Also useful for Undo/Redo I think.  This is why KstViewObjects have the
byte-stream encoding mechanism, and I think this is probably the proper way
to implement all of this stuff.

--
George Staikos
KDE Developer				http://www.kde.org/
Staikos Computing Services Inc.		http://www.staikos.net/
_______________________________________________
Kst mailing list
Kst@kde.org
https://mail.kde.org/mailman/listinfo/kst

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.

You obviously feel strongly about this and are more than a
little upset. Feel free to implement it any way you choose.
As long as the current functionality remains I'm not overly
concerned with the implementation details.

Andrew

*** Bug has been marked as fixed ***.

CVS commit by arwalker: 

Re-enable Edit...Copy functionality after changes to plotmimesource.cpp

CCMAIL: 96256@bugs.kde.org


  M +1 -14     kst.cpp   1.292
  M +5 -0      kstviewwidget.cpp   1.72


--- kdeextragear-2/kst/kst/kst.cpp  #1.291:1.292
@@ -1683,18 +1683,5 @@ void KstApp::slotCopy() {
     KstViewWindow *vw = dynamic_cast<KstViewWindow*>(activeWindow());
     if (vw) {
-      KstTopLevelViewPtr tlv = vw->view();
-      if (tlv) {
-        QStringList plotList;
-        
-        for (KstViewObjectList::Iterator i = tlv->selectionList().begin(); 
-                                        i != tlv->selectionList().end(); ++i) {
-          plotList.append((*i)->tagName());
-        }
-      
-        if (plotList.size() > 0) {
-          PlotMimeSource *newplots = new PlotMimeSource(vw->caption(), plotList);
-          QApplication::clipboard()->setData(newplots, QClipboard::Clipboard);
-        }
-      }    
+      QApplication::clipboard()->setData(vw->view()->widget()->dragObject(), QClipboard::Clipboard);
     }
   }

--- kdeextragear-2/kst/kst/kstviewwidget.cpp  #1.71:1.72
@@ -58,4 +58,9 @@ QDragObject *KstViewWidget::dragObject()
       plots.append(_view->pressTarget()->tagName());
       vol.append(_view->pressTarget());
+    } else {
+      for (size_t i=0; i<_view->children().size(); i++) {
+        plots.append(_view->children()[i]->tagName());
+        vol.append(_view->children()[i]);
+      }
     }
   } else {