Duplicate and copy & paste slices by martincapello · Pull Request #5049 · aseprite/aseprite

martincapello · 2025-03-07T21:06:21Z

Gasparoken · 2025-03-12T23:39:59Z

It works great! Just a couple of subjetive design comments (i.e. optional):

Would be great (IMO) if the position of the new Slices were displaced a few pixel to right and down to give a simple feedback to the artist that shows the duplication was actually done.
Perhaps would be better to add "Copy" as suffix in the name of the new Slices to preserve consistency with the Layer duplication and to express better the copy.

ckaiser

Duplicating with the right click menu works, but trying to use Ctrl+C or the copy menu does not for me, from what I can see, Site::selectedSlices() ends up being 0 when called in get_selected_slices - not sure if it's calling the wrong Site or before it gets populated or what exactly is going on there.

Left some comments over some localization/style stuff as well.

ckaiser · 2025-04-28T16:05:49Z

+  doc->notifyBeforeSlicesDuplication();
+  for (auto* s : selectedSlices) {
+    Slice* slice = new Slice(*s);
+    slice->setName(slice->name() + " Copy");


This should probably be localized with "{} Copy"

ckaiser · 2025-04-28T16:18:07Z

+      editor->clearSlicesSelection();
+      for (auto& s : slices) {
+        Slice* slice = new Slice(s);
+        slice->setName(slice->name() + " Copy");


Localization here as well.

ckaiser · 2025-04-28T16:33:40Z

      if (!m_editor->selectSliceBox(bounds) && (bounds.w > 1 || bounds.h > 1)) {
        Slice* slice = new Slice;
-        slice->setName(getUniqueSliceName());
+        slice->setName(get_unique_slice_name(m_sprite));


In the end now this ends up being only used here, seems like it could still be just a private member instead of a part of the utils, which might make the addition of the extra file and translation unit harder to justify since it could also just be a static function like Slice::getSelectedFromSite but that's just a style thing.

About get_unique_slice_name, I think I used it in other places at some point, but maybe after some changes it ended up being used in just that place...so yeah, I will move that function back to the private scope.
About replacing get_selected_slices with a static Slice::getSelectedFromSite function, sure, I'll do it.

Okay, I've just recalled why I moved the getUniqueSliceName to an utility function. It didn't make sense to me that the ToolLoopImpl class provided that function. So, my reasoning was that moving it to another translation unit would remove some noise from the ToolLoopImpl and reduce its size (speaking of lines of code) a bit.
So now I would prefer to keep the get_unique_slice_name function as an utility function in a separate translation unit. But I'm open to change my mind if you feel strongly about this.

I might consider moving it as another static member of Slice, something like: Slice::makeUniqueName()maybe?

I cannot convert the get_selected_slices function into Slice::getSelectedFromSite because the Site class belongs to the app module (src/app/site.h) and the Slice class belongs to the doc module (src/doc/slice.h), and the doc module doesn't depend on the app module. Hence the Slice class can't see the Site class.

I have also found that the doc module doesn't depend on the fmt lib, which the get_unique_slice_name uses (I know I could easily avoid using the fmt library but I prefer to leave these functions in the app module).

So I think I will leave both functions as is.

martincapello · 2025-04-28T21:16:10Z

trying to use Ctrl+C or the copy menu does not for me ...

I just tried in macOS and it works both ways, will try in Windows to see if there is an issue there. Take into account that when using the Ctrl+C and Ctrl+V or copy menu and then paste menu, the copied slices are pasted in the same position as the original ones, not displaced as it happens when using the Duplicate slice context menu. However, the new slices are selected after pasting them, so you can easily move them around to displace them from the original position.

EDIT: See my comment...I thought it worked on macOS because I didn't realize that my mouse pointer was not over the sprite editor...

martincapello · 2025-04-28T21:18:58Z

Force pushed a rebased version with the latest from beta. So, there are no changes yet.

martincapello · 2025-04-29T13:56:25Z

I can confirm that in Windows Ctrl+C doesn't copy the slices. However, the copy menu and paste menu do work as expected. Also, I've tried to copy the slices with the menu and paste them with Ctrl+V and it worked as well.
So, I'm guessing that there is some collision with the keys combo in Windows. Will investigate.

EDIT: I was wrong. Actually it works the same in macOS and Windows. The thing is that the Ctrl key (Cmd key in macOS) is bound to the Move tool (quick), and the Ctrl+C is bound to the Copy command. Then when the mouse cursor is over the sprite editor and the user presses Ctrl+C (Cmd+C), the Move tool is activated first and thus the C key is ignored/discarded. But, if the mouse pointer is not over the sprite editor, then Ctrl+C works as expected.
Now I'm wondering if actually this is the expected behavior (I believe it is but I would need @dacap confirmation)...because the Ctrl key is bound to a tool that is getting the control when it is activated. Changing the key assigned to the Move tool (quick) "fixes" the issue.

dacap · 2025-05-19T21:53:46Z

I'm going to test this PR. About the i18n of {} Copy text, I don't know if it's possible to create a generic string for that. E.g. Portuguese might need a Cópia da {} or Cópia do {}, or French Copie du {} or Copie de {}.

martincapello · 2025-06-06T19:55:25Z

I'm going to test this PR. About the i18n of {} Copy text, I don't know if it's possible to create a generic string for that. E.g. Portuguese might need a Cópia da {} or Cópia do {}, or French Copie du {} or Copie de {}.

In this particular case I'm using the message for when a slice is copied. And the parameter is the name of the slice. Obviously we are not going to determine the name's gender to show the correct variant. So I guess you mean in the case the {} Copy text is used for some program entity, like the slice, or a frame, then in english we would have Slice Copy or Frame Copy. But in Spanish they could be Copia de la rebanada and Copia del cuadro, and hence we would need two templates, one for the feminine case, and another for the masculine case as you said.
Then if we define templates by gender I see another issue: we don't know which gender will have the word used as the parameter of the template in the current language, then we cannot use always one of the templates neither.

However, if we use Copia de {}in spanish, it would be Copia de rebanada, Copia de cuadro which works well enough IMO, and also works when using names of entities, like Copia de 'Rebanada 1'. Not sure if this applies to all languages though.

Let me know what do you think we should do about this. I have a fix in mind for using the gender based templates, but I think it doesn't worth the additional complexity.

ckaiser · 2025-06-07T11:11:01Z

Let me know what do you think we should do about this. I have a fix in mind for using the gender based templates, but I think it doesn't worth the additional complexity.

Doing this kind of work ourselves is pretty intense, it's why projects like Fluent exist. I don't think we have enough text to justify it, could just be a matter of not using variables for this kind of thing and instead just having different strings for each (Copy of slice vs Copy of {}), only reserving template variables for stuff we know is a proper name or a user input that will usually not need a translation. It makes things more tedious but it ends up being simpler.

dacap · 2025-06-07T12:44:02Z

As @ckaiser, for our case, we can have the string for each case. If in a future we consider that it’s getting out of hand, we could adopt some library like Fluent (didn’t know about that project, I remember some pluralization features from the old gettext library but never used it).

dacap · 2025-06-07T12:57:01Z

In this particular case I'm using the message for when a slice is copied. And the parameter is the name of the slice.

Probably I was confused by the other PR, but as you said, it might work if it is the copy of a "name". Probably I got confused by the default name of things (later, slice, etc). Not sure if it can work in a generic form.

martincapello · 2025-06-09T12:17:08Z

@ckaiser exactly, that is the easiest way when we don't have to parameterize the text. Unfortunately, I have to.

Probably I was confused by the other PR, but as you said, it might work if it is the copy of a "name". Probably I got confused by the default name of things (later, slice, etc). Not sure if it can work in a generic form.

No worries. I'm not sure either, but I think it is the only way to say that we copied something with a name. In the other PR (#5167) I was using it for when the user duplicates a sprite (the parameter is the name of the file) and when the user duplicates a layer (the parameter is the name of the layer).

Is it okay to keep this as is then? Or should I revert the 000f163 change?

dacap · 2025-06-09T14:27:01Z

Is it okay to keep this as is then? Or should I revert the 000f163 change?

We can keep it, but you should amend that commit, as the string is in the [general_text] section which are strings used for the ui::Entry widget. You can move it to [general].

martincapello · 2025-06-09T20:27:41Z

I've just amended the last commit and rebased my branch.

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2025-06-09T20:30:27Z

+
+void DuplicateSliceCommand::onLoadParams(const Params& params)
+{
+  std::string id = params.get("id");


warning: variable 'id' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]

Suggested change

std::string id = params.get("id");

std::string const id = params.get("id");

aseprite-bot · 2025-06-09T20:30:27Z

+      selectedSlices.push_back(reader.sprite()->slices().getById(m_sliceId));
+  }
+
+  ContextWriter writer(context);


warning: variable 'writer' of type 'ContextWriter' can be declared 'const' [misc-const-correctness]

Suggested change

ContextWriter writer(context);

ContextWriter const writer(context);

aseprite-bot · 2025-06-09T20:30:27Z

+    sliceName = selectedSlices[0]->name();
+
+  StatusBar::instance()->invalidate();
+  if (!sliceName.empty()) {


warning: if with identical then and else branches [bugprone-branch-clone]

if (!sliceName.empty()) { ^

Additional context

src/app/commands/cmd_duplicate_slice.cpp:109: else branch starts here

else { ^

aseprite-bot · 2025-06-09T20:30:27Z


+void Doc::notifyBeforeSlicesDuplication()
+{
+  DocEvent ev(this);


warning: variable 'ev' of type 'DocEvent' can be declared 'const' [misc-const-correctness]

Suggested change

DocEvent ev(this);

DocEvent const ev(this);

aseprite-bot · 2025-06-09T20:30:27Z

@@ -510,6 +512,8 @@ bool DocView::onCanCopy(Context* ctx)
    return true;


warning: repeated branch body in conditional chain [bugprone-branch-clone]

return true; ^

Additional context

src/app/ui/doc_view.cpp:511: end of the original

return true; ^

src/app/ui/doc_view.cpp:513: clone 1 starts here

return true; ^

src/app/ui/doc_view.cpp:515: clone 2 starts here

return true; ^

aseprite-bot · 2025-06-09T20:30:28Z

    m_state->onBeforeLayerEditableChange(this, ev.layer(), newState);
 }

+void Editor::onBeforeSlicesDuplication(DocEvent& ev)


warning: parameter 'ev' is unused [misc-unused-parameters]

Suggested change

void Editor::onBeforeSlicesDuplication(DocEvent& ev)

void Editor::onBeforeSlicesDuplication(DocEvent& /*ev*/)

aseprite-bot · 2025-06-09T20:30:28Z

  m_data->picks = picks;
 }

+void Clipboard::copySlices(const std::vector<Slice*> slices)


warning: method 'copySlices' can be made static [readability-convert-member-functions-to-static]

Suggested change

void Clipboard::copySlices(const std::vector<Slice*> slices)

static void Clipboard::copySlices(const std::vector<Slice*> slices)

aseprite-bot · 2025-06-09T20:30:28Z

+      if (slices.empty())
+        return;
+
+      ContextWriter writer(ctx);


warning: variable 'writer' of type 'ContextWriter' can be declared 'const' [misc-const-correctness]

Suggested change

ContextWriter writer(ctx);

ContextWriter const writer(ctx);

aseprite-bot · 2025-06-09T20:30:28Z

                   const doc::Palette* pal,
                   const doc::Tileset* tileset);
  void copyPalette(const doc::Palette* palette, const doc::PalettePicks& picks);
+  void copySlices(const std::vector<doc::Slice*> slices);


warning: parameter 'slices' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

void copySlices(const std::vector<doc::Slice*> slices);

void copySlices(std::vector<doc::Slice*> slices);

martincapello · 2025-08-08T14:56:57Z

@dacap Should I rebase this against main branch now?

dacap · 2025-08-25T18:52:32Z

@dacap Should I rebase this against main branch now?

Please rebase with main 👍

aseprite-bot · 2025-08-27T12:36:59Z

Hi there!

One or more of the commit messages in this PR do not match our code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

martincapello requested a review from dacap as a code owner March 7, 2025 21:06

martincapello assigned Gasparoken Mar 7, 2025

martincapello force-pushed the copy-paste-slices branch 3 times, most recently from 7c4829d to 28a22c0 Compare March 12, 2025 13:50

Gasparoken assigned martincapello and unassigned Gasparoken Mar 12, 2025

martincapello force-pushed the copy-paste-slices branch from 28a22c0 to f787656 Compare March 25, 2025 13:17

martincapello assigned dacap and unassigned martincapello Mar 25, 2025

dacap assigned ckaiser and unassigned dacap Apr 21, 2025

ckaiser suggested changes Apr 28, 2025

View reviewed changes

ckaiser assigned martincapello and unassigned ckaiser Apr 28, 2025

martincapello force-pushed the copy-paste-slices branch from 10b4141 to c4e305d Compare April 28, 2025 21:18

martincapello assigned ckaiser and unassigned martincapello Apr 29, 2025

dacap assigned dacap and unassigned ckaiser Apr 29, 2025

martincapello mentioned this pull request May 14, 2025

Add i18n for some hardcoded strings #5167

Merged

dacap assigned martincapello and unassigned dacap Jun 9, 2025

martincapello force-pushed the copy-paste-slices branch from 000f163 to 4a70697 Compare June 9, 2025 20:26

martincapello assigned dacap and unassigned martincapello Jun 9, 2025

aseprite-bot reviewed Jun 9, 2025

View reviewed changes

dacap assigned martincapello and unassigned dacap Aug 25, 2025

martincapello added 4 commits August 27, 2025 09:25

Add slices copy&paste and duplication (fix aseprite#4466)

50ede4f

Offset duplicated slices to avoid overlapping

e0f33a0

Use "Copy" suffix to name duplicated slices

357c054

Internationalize " Copy" string usage

935283e

martincapello force-pushed the copy-paste-slices branch from 4a70697 to 935283e Compare August 27, 2025 12:36

martincapello changed the base branch from beta to main August 27, 2025 12:37

martincapello assigned dacap and unassigned martincapello Aug 27, 2025

dacap added this to the v1.3.15.1 milestone Aug 27, 2025

dacap merged commit 9c5ca6b into aseprite:main Aug 27, 2025
1 check failed

	std::string id = params.get("id");
	std::string const id = params.get("id");

	ContextWriter writer(context);
	ContextWriter const writer(context);

		@@ -510,6 +512,8 @@ bool DocView::onCanCopy(Context* ctx)
		return true;

	void Editor::onBeforeSlicesDuplication(DocEvent& ev)
	void Editor::onBeforeSlicesDuplication(DocEvent& /ev/)

	void Clipboard::copySlices(const std::vector<Slice*> slices)
	static void Clipboard::copySlices(const std::vector<Slice*> slices)

	void copySlices(const std::vector<doc::Slice*> slices);
	void copySlices(std::vector<doc::Slice*> slices);

Uh oh!

Uh oh!

Conversation

martincapello commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gasparoken commented Mar 12, 2025

Uh oh!

ckaiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martincapello Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martincapello commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martincapello commented Apr 28, 2025

Uh oh!

martincapello commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dacap commented May 19, 2025

Uh oh!

martincapello commented Jun 6, 2025

Uh oh!

ckaiser commented Jun 7, 2025

Uh oh!

dacap commented Jun 7, 2025

Uh oh!

dacap commented Jun 7, 2025

Uh oh!

martincapello commented Jun 9, 2025

Uh oh!

dacap commented Jun 9, 2025

Uh oh!

martincapello commented Jun 9, 2025

Uh oh!

aseprite-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martincapello commented Aug 8, 2025

Uh oh!

dacap commented Aug 25, 2025

Uh oh!

aseprite-bot commented Aug 27, 2025

Uh oh!

Uh oh!

Reviewers

martincapello commented Mar 7, 2025 •

edited

Loading

martincapello Apr 29, 2025 •

edited

Loading

martincapello commented Apr 28, 2025 •

edited

Loading

martincapello commented Apr 29, 2025 •

edited

Loading