Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy Paste Functionality #3603

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

athaarnaqvi
Copy link
Contributor

I have introduced an enhancement to the existing duplicate functionality by implementing new Copy and Paste features. Unlike the existing duplication feature, the new Copy and Paste functionality supports copying connections (Links) as well.
-Added key bindings for Copy and Paste operations in the keyPressEvent function.
-Implemented a new Copy function that enables the Paste function through a boolean variable.
-Introduced a new Paste function that handles the duplication of copied items. This function is triggered by the boolean variable set by the Copy function, ensuring that paste operations are only performed when there is copied content to paste.

This enhancement significantly improves the user experience by allowing not only the duplication of items but also their associated connections, thus preserving the integrity of the duplicated elements.

@athaarnaqvi
Copy link
Contributor Author

@grossmj Will you please review this and guide me for further modifications.

@athaarnaqvi
Copy link
Contributor Author

@grossmj There was a problem with KeyPressEvent Function so i fixed it i hope now it will pass all the test cases

@grossmj grossmj changed the base branch from master to 2.2 July 26, 2024 14:28
@grossmj grossmj changed the base branch from 2.2 to master July 26, 2024 14:29
@ghost
Copy link

ghost commented Jul 27, 2024

I just tested this PR and it doesn't work here.

My environment: Debian Linux 12.6 (Bullseye) with Python 3.11.2 and QT 5.15.8.

I cloned the repository https://github.com/athaarnaqvi/gns3-gui and then did a git checkout copy_paste to have exactly the same software version as the OP.

1. Issue: The function copySelectedItems needs the parameter self

When issuing a ctrl-c I get a traceback:

Traceback (most recent call last):
  File "/home/behlers/GNS3/venv/lib/python3.11/site-packages/gns3/graphics_view.py", line 618, in keyPressEvent
    self.copySelectedItems()
TypeError: GraphicsView.copySelectedItems() takes 0 positional arguments but 1 was given

Adding parameter self fixes that.

2. Issue: The global variable isctrlcpressed can't be set

I could verify, that copySelectedItems is called on ctrl-c and it sets isctrlcpressed to True, but on ctrl-v in function pasteItemsFromClipboard the variable is False. I have no explanation for that. But when I change isctrlcpressed to a class variable it works.

3. Issue: The connections are not copied

I used a rectangle area selection to select R1 and R2 and the link between them.
R1-R2-selected

Then I'm using ctrl-c and ctrl-v to cut and paste them. But only the nodes were copied, not the link.
paste

I added a debug output in pasteItemsFromClipboard to show the items selected. Two nodes and 4 labels were selected, but no link.

2024-07-27 23:12:29 INFO graphics_view.py:634 item type=<class 'gns3.items.label_item.LabelItem'>
2024-07-27 23:12:29 INFO graphics_view.py:634 item type=<class 'gns3.items.node_item.NodeItem'>
2024-07-27 23:12:29 INFO graphics_view.py:634 item type=<class 'gns3.items.label_item.LabelItem'>
2024-07-27 23:12:29 INFO graphics_view.py:634 item type=<class 'gns3.items.node_item.NodeItem'>
2024-07-27 23:12:29 INFO graphics_view.py:634 item type=<class 'gns3.items.label_item.LabelItem'>
2024-07-27 23:12:29 INFO graphics_view.py:634 item type=<class 'gns3.items.label_item.LabelItem'>

Here the diffs of all my changes:

diff --git a/gns3/graphics_view.py b/gns3/graphics_view.py
index d28af25c..c317676b 100644
--- a/gns3/graphics_view.py
+++ b/gns3/graphics_view.py
@@ -65,7 +65,6 @@ from .items.image_item import ImageItem
 from .items.logo_item import  LogoItem
 
 log = logging.getLogger(__name__)
-isctrlcpressed = False
 
 class GraphicsView(QtWidgets.QGraphicsView):
     """
@@ -96,6 +95,7 @@ class GraphicsView(QtWidgets.QGraphicsView):
         self._topology = Topology.instance()
         self._background_warning_msgbox = QtWidgets.QErrorMessage(self)
         self._background_warning_msgbox.setWindowTitle("Layer position")
+        self._isctrlcpressed = False
 
         # set the scene
         scene = QtWidgets.QGraphicsScene(parent=self)
@@ -622,14 +622,16 @@ class GraphicsView(QtWidgets.QGraphicsView):
                 self.pasteItemsFromClipboard()
                 
         super().keyPressEvent(event)
-    def copySelectedItems():
-        isctrlcpressed = True
+
+    def copySelectedItems(self):
+        self._isctrlcpressed = True
 
     def pasteItemsFromClipboard(self):  
-        if isctrlcpressed == False:
+        if self._isctrlcpressed == False:
             return
         else:
             for item in self.scene().selectedItems():
+                log.info(f"item type={item.__class__}")
                 if isinstance(item, DrawingItem):
                     if isinstance(item, EllipseItem):
                         type = "ellipse"

So I'm really puzzled, did I select it wrong? Or does the code not work in my environment?

At least I think it needs some polishing.

Just some additional remarks:

  • The ctrl-c shortcut conflicts with commit 21409a8
  • The copySelectedItems function doesn't really copy the selection, it just sets a flag. So if you change the selection after copying and then paste, you get a copy of the changed selection, not a copy of the original selection.
  • The pasteItemsFromClipboard function is almost identical to the duplicateActionSlot function. It should be possible to eliminate this code repetition.

@athaarnaqvi
Copy link
Contributor Author

  • The ctrl-c shortcut conflicts with commit 21409a8

I used the conventional shortcut key for copy. Changing the convention may not be very handful for the users.

  • The copySelectedItems function doesn't really copy the selection, it just sets a flag. So if you change the selection after copying and then paste, you get a copy of the changed selection, not a copy of the original selection.

I have fixed this in the PR as now the copySelectedItems function will call the pasteItemsFromClipboard function only when the CTRL+V is pressed.

  • The pasteItemsFromClipboard function is almost identical to the duplicateActionSlot function. It should be possible to eliminate this code repetition.

I have directly called the duplicate function in here as for the duplication of links can you identify where in the code the links are being added initially then i will work on it.

@athaarnaqvi
Copy link
Contributor Author

Hi!
So there are two functions in graphics_view.py that seem to be used to create links _userNodeLinking and _addLinkSlot. Now please confirm which one is used to create the links in the beginning.
@grossmj Please review it.

@grossmj
Copy link
Member

grossmj commented Aug 5, 2024

I think the correct way to copy items would be to use the QClipboard class in Qt: https://doc.qt.io/qt-5/qclipboard.html

Something similar to https://stackoverflow.com/questions/57353390/how-to-copy-paste-an-qgraphicsitem-in-pyqt5

When CTRL + V is pressed, you retrieve the items saved in the QClipboard and create new ones like it is done in duplicateActionSlot()

Once this has been correctly implement, you can try to focus on copying the links as well, however the actual duplication is done on the server side. You will have to create another PR there.

@athaarnaqvi
Copy link
Contributor Author

okay I will try to cover it as soon as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants