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 of shapes #409

Closed
wants to merge 4 commits into from
Closed

Copy/paste of shapes #409

wants to merge 4 commits into from

Conversation

warmachinesocial
Copy link

Summary / How this PR fixes the problem?

It's still a feature to be implement as of yet

Steps to Test

Just try to use the normal copy/ paste controls for the feature to used

Screenshots

Initially made a rect, then a circle. Selected the rect, then copied it

image

After that, use the paste action,

image

Known Issues / Things To Do

This PR fixes/implements the following bugs/features:

@Alecaddd Alecaddd marked this pull request as draft August 16, 2020 17:57
@Alecaddd
Copy link
Member

Hi, thanks for contributing to Akira.

A couple of general guidelines to follow:

  • If your PR is a still a work in progress, open it as a Draft PR. I updated it already so no problem for now.
  • Don't write that a PR fixes an issue if that issue doesn't have a 1-to-1 relationship with the PR. In this case, you're stating that this PR fixes the Beta Whishlist, but that's incorrect.

Unfortunately, this is not the right approach.

You're assuming that the items to paste are those currently selected. That's not always the case.
You're limiting the copy/paste to 1 item at a time, but that's not scalable and should account for all selected items, therefore, this PR should be postponed after the multiselect is implemented.
With this approach you will not properly inherit all the attributes of the duplicated item (colors, borders, rotation, etc.).
You're currently reaching properties and methods by going through the parent->child hierarchy of all the classes, which is cumbersome and not scalable. You should use events for that.

Thanks again for your contribution, but I recommend to spend a little bit more time in trying to get comfortable with Vala before submitting a PR.
Also, Akira has a fairly complex code base, so you should get yourself acquainted with it before submitting a PR to be sure you're following the same paradigms and architecture.

@Alecaddd
Copy link
Member

Another couple of fundamental things to consider:

  • When copying, the cached elements should belong to the App and not the Window, so users can copy a shape from a window and paste it in another.
  • When pasting, we need to detect what the user has saved in the clipboard and handle those cases (images, svg icons, text, etc.)

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

Successfully merging this pull request may close these issues.

Beta Release Wishlist
2 participants