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

[Erik Johansson]

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.

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

[trompetin17]

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

[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?

[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):

[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?

[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

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

[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?

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

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

[stilz_]

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

[stilz_]

Someone can review this?

[leper]

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

[Itms]

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! :-)

[stilz_]

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

[stilz_]

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

[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

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

[Itms]

In 15994:

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

[Itms]

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