Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2642 closed enhancement (fixed)

[PATCH] Add Copy and Paste to the Edit menu in Atlas

Reported by: Erik Johansson Owned by: stilz_
Priority: Nice to Have Milestone: Alpha 18
Component: Atlas editor Keywords:
Cc: Patch:

Description

It would be nice to have "Copy (Ctrl + C)", and "Paste (Ctrl + V)" appear in the Edit menu to help people discover that the option exists. And of course with the corresponding functionality.

Attachments (4)

2642.diff (8.3 KB) - added by stilz_ 5 years ago.
2642_2.diff (8.1 KB) - added by stilz_ 5 years ago.
2642_3.diff (9.6 KB) - added by stilz_ 5 years ago.
2642_4.diff (9.6 KB) - added by stilz_ 5 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 5 years ago by historic_bruno

One thing that might be confusing is when and how the copy/paste is activated. Should selecting text in the various input controls also allow copy/pasting, and should it be the same clipboard? (is it possible to have multiple clipboards?)

Also, I noticed that currently you have to select the transform tool to copy/paste entities, so it would be nice if that tool was automatically activated under certain conditions, as it is when placing new objects.

comment:2 Changed 5 years ago by trompetin17

Why if we add two buttons in toolbar "Copy" and "Paste"

comment:3 Changed 5 years ago by Erik Johansson

It could be done of course, but imho the toolbar should have as few buttons as possible. Not sure if we have a rule for these things, but imho the toolbar buttons should really be the ones you can't use the application without. Everything else should be in the tabs/menus/using hotkeys. And you can use Atlas without copying-and-pasting, it's very useful of course, but we can't put everything in the toolbar.

Actually thinking about it a bit more I think it's probably best to try and limit it to only actual tools, i.e. you select one of them and then do something on the map. There's probably going to be some exception eventually, but as a general rule that's probably the best one.

As for when which copy-and-paste should be active, wouldn't the easiest/clearest be if copy-and-paste worked on the map if you have the move/select tool selected, but text otherwise?

comment:4 Changed 5 years ago by historic_bruno

#942 is about the toolbar. As for layout, the only mockup I've seen for Atlas is this old one from Michael (note it has a weird copy/paste button):

http://i.imgur.com/j9CKh.jpg

comment:5 Changed 5 years ago by stilz_

I find current implementation of copy-paste mechanism somewhat tricky to extend. Everything happens in TransformObject? class implementation. Because this class is not accessible from outside, there's no way to dynamic_cast<TransformObject?>(GetCurrentTool?()).OnStartPaste?(). Also, faking wxKeyEvent does not seem to be elegant way. Do you have ideas how to solve this elegantly?

comment:6 Changed 5 years ago by trompetin17

If you look in ScenarioEditor/Sections/Object/Object?.cpp, there is a way to pass arguments to the tool, in this case this pass a wxString

m_ScenarioEditor.GetToolManager().SetCurrentTool(_T("PlaceObject"), &id);
virtual void Init(void* initData, ScenarioEditor* scenarioEditor)
61	    {
62	        StateDrivenTool<PlaceObject>::Init(initData, scenarioEditor);
63	
64	        wxASSERT(initData);
65	        wxString& id = *static_cast<wxString*>(initData);
66	        m_ObjectID = id;
67	        SendObjectMsg(true);
68	    }

you could use the same way to call TransformObject? and pass something like a number or wxString, and from this point you could call "onPasteStart".

let me know if this is useful for you

Changed 5 years ago by stilz_

Attachment: 2642.diff added

comment:7 Changed 5 years ago by stilz_

It was helpful at first look, but then I got stuck on OnDisable? which was clearing selected objects. I decided to go other way around - add new virtual method to ITool - OnCommand?. Please review attached patch and tell me whether it is acceptable. The patch adds copy and paste menu items and some code to control when these items are enabled. Also, I would be thankful if someone could test it on windows host.

comment:8 Changed 5 years ago by trompetin17

hi stilz_, the first thing is remove

using std::vector; 

from your code and use std::vector in

void ScenarioEditor::OnSelectedObjectsChange(vector<ObjectID> const& selectedObjects) 

and could you explain me more about onDisabled, what happen to you?

comment:9 Changed 5 years ago by stilz_

OnDisabled? is called when tool is being changed. SetCurrentTool?() does not pay attention to current tool, so it may change the tool to the same, causing side effects (Shutdown() method). Flow:

  1. You click on copy.
  2. Tool is set to TransformObject?, g_SelectedObject is cleared
  3. TransformObject? is initialized with &id meaning copy, but there's nothing to copy.

comment:10 in reply to:  9 Changed 5 years ago by trompetin17

Could you fix in your patch when you need to compare two wxString use == and not Cmp.

Replying to stilz_:

OnDisabled? is called when tool is being changed. SetCurrentTool?() does not pay attention to current tool, so it may change the tool to the same, causing side effects (Shutdown() method). Flow:

  1. You click on copy.
  2. Tool is set to TransformObject?, g_SelectedObject is cleared
  3. TransformObject? is initialized with &id meaning copy, but there's nothing to copy.

comment:11 Changed 5 years ago by stilz_

Sure, I'll fix this vector issue and start using ==. Any other remarks?

Changed 5 years ago by stilz_

Attachment: 2642_2.diff added

comment:12 Changed 5 years ago by stilz_

Owner: set to stilz_
Status: newassigned

comment:13 Changed 5 years ago by stilz_

Someone can review this?

comment:14 Changed 5 years ago by leper

Keywords: patch review added
Milestone: BacklogAlpha 17

Please follow SubmittingPatches when doing so, else it does not show up in our review query.

Some early returns wouldn't hurt the patch (eg ScenarioEditor::OnMenuOpen(wxMenuEvent& event) and OnCopy()).

comment:15 Changed 5 years ago by stanislas69

Summary: Add Copy and Paste to the Edit menu in Atlas[PATCH] Add Copy and Paste to the Edit menu in Atlas

comment:16 Changed 5 years ago by leper

Milestone: Alpha 17Alpha 18

comment:17 Changed 5 years ago by Itms

Keywords: review removed

Hello stilz, your patch seems to work well. However:

  • The "Paste" entry is always activated, so I can trigger an ugly error message by pasting when the clipboard is empty
  • You have TODOs in your code, that should be fixed
  • The early returns leper mentioned must be fixed too

I hope you're still willing to work on this. If it's not the case, please tell us so someone else can work on the basis of you patch! Sorry for the time took before reviewing, and thanks a lot for your work so far! :-)

comment:18 Changed 5 years ago by stilz_

i'll try to find time to fix it in next few days, for sure

Changed 5 years ago by stilz_

Attachment: 2642_3.diff added

comment:19 Changed 5 years ago by stilz_

Keywords: review added

I uploaded an updated patch. It works well on my machine (Linux).

comment:20 Changed 5 years ago by Itms

Hi stilz_, this works well here too! I have some style remarks to make before we can commit that patch:

  • Remove the brackets for single-line conditionals (cf. ScenarioEditor.cpp lines 644, 646, 652, 654, etc.), you have plenty of these
  • ScenarioEditor.cpp line 904, ScenarioEditor.h line 60, etc., we tend to always write const T& instead of T const&
  • Using early returns in OnMenuOpen would make it prettier (you can also merge all conditionals that disable the Paste menu entry into one)
  • You don't have to write an empty body for pure virtual functions (Tools.h)
  • You should name the void* parameter of your functions (be careful, there is an extra space in Tools.h, l.38), then you can use the WXUNUSED macro to avoid compiler warnings.

Thanks for your work on it! :D

Changed 5 years ago by stilz_

Attachment: 2642_4.diff added

comment:21 Changed 5 years ago by stilz_

Hi, thanks for remarks. I simplified OnMenuOpen function. I don't see any more places for early returns (perhaps I'm too tired ;). Don't hesitate to point it out, though.

Regarding this superfluous body for pure virtual function - somehow it is needed. Otherwise compiler will yell about undefined reference to typeinfo.

comment:22 Changed 5 years ago by Itms

Resolution: fixed
Status: assignedclosed

In 15994:

Add "Copy" and "Paste" entries to the Atlas "Edit" menu. Patch by stilz_, fixes #2642.

comment:23 Changed 5 years ago by Itms

Keywords: review patch removed

Sorry, I nearly forgot this patch (our review queue is way too long!) I fixed some things but I mention them here:

  • don't forget to update the copyright date in the files you modify, if necessary
  • you had typoed "command" in Tools.cpp
  • I removed useless braces in TransformObject?.cpp, line 351

About the pure virtual function, it's me, I lacked context and I thought the body was the body of the function for the abstract class and not its child (which is silly now that I write it...)

Anyway, thanks for the patch! :D

Note: See TracTickets for help on using tickets.