fix: pasting of external exported valuenodes #2086

rodolforg · 2021-03-30T12:28:23Z

ghost · 2021-03-31T17:39:18Z

Congratulations 🎉. DeepCode analyzed your code in 2.886 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

rodolforg · 2021-04-01T12:26:49Z

It does work fine now.
However, I didn't try to fix the copy'n paste issues for Layer_Duplicate or Layer_Skeleton or Layer_Skeleton_Deform.

rodolforg · 2021-04-01T14:45:04Z

@morevnaproject could you please test it? I hope it works like you described on synfig-docs-dev.

morevnaproject · 2021-05-03T15:33:15Z

Hi! I have finally tested the feature. ^__^

First of all, I would like to say that this feature is an epic achievement in solving this old and annoying problem of copy-pasting. This is so exciting!

Here is a set of files, which I used to test this PR -
2086.zip

It contains 2 files:

export-a.sifz
export-b.sifz

I opened both files and copied "Group 1" layer from export-a.sifz to export-b.sifz.
It displays following dialog for me (as expected):

I have notice following issues:

The leftmost column is too tiny - the heading with "Copy?" title (1) is only partially visible.
If I click Cancel button (2), then the layer is copied anyway. As user I would expect the copy operation to be aborted. ^__^
After playing a bit with the feature, I found that it will be useful to have path in "Source file" column always visible (3). The reference document describes different behaviour, but after using the feature I think we should change that.
When I disabled checkbox in second row (for "t2" value) the "OK" button remains inactive -

Disabling checkbox for "t2" value should tell synfig to not copy exported value. Instead it should be linked from export-a.sifz file, so no conflict take place and OK button should become active.

Thanks again for implementing this! 👏

rodolforg · 2021-05-03T16:32:15Z

The leftmost column is too tiny - the heading with "Copy?" title (1) is only partially visible.

Yes, I'm fighting against Gtk to make it right

If I click Cancel button (2), then the layer is copied anyway. As user I would expect the copy operation to be aborted. ^__^

Fixed

3\. After playing a bit with the feature, I found that it will be useful to have path in "Source file" column always visible (3). The [reference document](https://synfig-docs-dev.readthedocs.io/en/latest/projects/copy-paste-exported-values.html) describes different behaviour, but after using the feature I think we should change that.

~~Ok, I'll do it.~~ Done

4\. Disabling checkbox for "t2" value should tell synfig to **not** copy  exported value. Instead it should be linked from `export-a.sifz` file, so no conflict take place and OK button should become active.

Problem is that they are of a different type: vector x real.

rodolforg · 2021-05-03T16:33:56Z

Any suggestions about the text in the dialog?
And the tooltips I added in last commit? @morevnaproject

morevnaproject · 2021-05-05T14:34:49Z

4. Disabling checkbox for "t2" value should tell synfig to not copy exported value. Instead it should be linked from export-a.sifz file, so no conflict take place and OK button should become active.

Problem is that they are of a different type: vector x real.

Yes, they are different types.
But in this case the layer will reference "t2" parameter from export-a.sifz file.
Here I have recorded a short video to demonstrate how it currently works ^__^ (i have used version of Synfig without changes made in this PR) - https://www.youtube.com/watch?v=Q4uJ0HGasfg

morevnaproject · 2021-05-05T15:10:03Z

Any suggestions about the text in the dialog?

I suggest to use "exported value" instead of "exported valuenode" in the text.
Also, I can suggest just a slight change for "Note" text -

if you choose to keep a valuenode linked to other file, you won't be able to load this one if source file(s) are deleted, moved or renamed.

I have made attempt to remove double "if":

If you choose to keep an exported value linked to other file, then your current file become dependent on source (referenced) file. That means you will not be able to load this file if any source file is deleted, moved or renamed.

I am not an expert in English, though. ^___^"

morevnaproject · 2021-05-05T15:11:18Z

And the tooltips I added in last commit?

Tooltips are great!
There is only one issue - the tooltip for "t3" value is empty in my case ^__^ -

rodolforg · 2021-05-08T04:09:44Z

I am not an expert in English, though. ^___^"

Me neither ;)

There is only one issue - the tooltip for "t3" value is empty in my case ^__^ -

It was intentional, but I'll fix it.

rodolforg · 2021-05-08T04:32:32Z

But in this case the layer will reference "t2" parameter from export-a.sifz file.

Ouch ! lol

I fixed it as well as the text changes you suggested.

morevnaproject · 2021-05-26T04:36:22Z

I have notice following issues:
...

If I click Cancel button (2), then the layer is copied anyway. As user I would expect the copy operation to be aborted. ^__^

Works as expected now. ^__^

After playing a bit with the feature, I found that it will be useful to have path in "Source file" column always visible (3). The reference document describes different behaviour, but after using the feature I think we should change that.

Confirmed as fixed.

When I disabled checkbox in second row (for "t2" value) the "OK" button remains inactive -

Confirmed as fixed.

We have following issue remaining:

The leftmost column is too tiny - the heading with "Copy?" title (1) is only partially visible.

...and here are some more, which I've got after testing latest version of this PR:

After text change the dialog became too wide. I suggest to add line break after "... (referenced) file." words -

I have found no other issues so far. Great work! ^__^

ice0 · 2021-05-27T01:42:17Z

After text change the dialog became too wide. I suggest to add line break after "... (referenced) file." words -

Maybe just allow word wrap in the label?

rodolforg · 2021-05-28T09:55:15Z

Here is how it is now.

rodolforg · 2021-05-28T09:57:49Z

i think the only missing issue are reported here:

File corruption: Copy-paste layers with exported values from one file to another #1393 (comment)
File corruption: Copy-paste layers with exported values from one file to another #1393 (comment)

morevnaproject · 2021-05-28T16:12:48Z

i think the only missing issue are reported here:

* [#1393 (comment)](https://github.com/synfig/synfig/issues/1393#issuecomment-831349492)

* [#1393 (comment)](https://github.com/synfig/synfig/issues/1393#issuecomment-810306466)

For first item I have outlined expected behavior in this comment - #1393 (comment)
I need to check how it works in current implementation...

For second item I have wrote comment right now ^__^ - #1393 (comment)

…layer pasting Also Layer Copy behaves like Duplicate: it ignores selected layers inside groups if the group itself is selected. It avoids duplication of (group, its contents AND the selected contents)

… a conflict: don't let it happen!

https://wiki.synfig.org/Canvas "Canvases can be exported (the name must not contains space or :#@$^&()* characters) , [...]" as well as in synfigapp::Instance::import_external_canvas(Canvas::Handle canvas, std::map<Canvas*, Canvas::Handle> &imported)

And don't let user choose to copy a value node that is a dependency for another valuenode that user chose to be a foreign link

they were renamed in fb75250 and 920d20f

rodolforg · 2023-04-23T12:42:27Z

Rebased again :)
If OK, please squash and merge. And report the pending problem.

ice0 · 2023-04-24T05:25:23Z

Squashed and merged :)

Thank you!

rodolforg force-pushed the fix-1393-copy-paste-dialog branch from 22d71fc to 1e6bf25 Compare March 31, 2021 17:38

rodolforg force-pushed the fix-1393-copy-paste-dialog branch from 1e6bf25 to ec21790 Compare March 31, 2021 17:41

rodolforg added the Backport label Apr 1, 2021

rodolforg force-pushed the fix-1393-copy-paste-dialog branch from ec21790 to aa94ee1 Compare April 1, 2021 12:25

rodolforg changed the title ~~DRAFT - Fix pasting of external exported valuenodes - fix #1393~~ Fix pasting of external exported valuenodes - fix #1393 Apr 1, 2021

ice0 assigned morevnaproject Apr 2, 2021

rodolforg mentioned this pull request Apr 16, 2021

[Release] v1.4.1 #1961

Merged

rodolforg marked this pull request as ready for review April 16, 2021 02:11

rodolforg marked this pull request as draft April 16, 2021 13:55

morevnaproject assigned rodolforg and unassigned morevnaproject May 3, 2021

rodolforg force-pushed the fix-1393-copy-paste-dialog branch from aa94ee1 to a66e03f Compare May 3, 2021 22:40

rodolforg force-pushed the fix-1393-copy-paste-dialog branch from 532dad5 to b782c6f Compare May 28, 2021 06:51

rodolforg force-pushed the fix-1393-copy-paste-dialog branch from f1a0abe to 1c7b496 Compare June 12, 2021 11:18

rodolforg added 23 commits April 23, 2023 09:41

DRAFT - create paste dialog - fix synfig#1393

671e3ff

SQUASH - Query user how to deal with external exported valuenodes on …

1f67823

…layer pasting Also Layer Copy behaves like Duplicate: it ignores selected layers inside groups if the group itself is selected. It avoids duplication of (group, its contents AND the selected contents)

SQUASH: canceling the dialog will prevent any layer paste

dc2bfd4

SQUASH: File path now always visible

9253fb4

SQAUSH add tooltips to describe each valuenode status/action

f117caf

SQUASH: improve writing and add missing tooltip

46ce720

SQUASH: improve code reading for updating ok-button sensitivity

8ce890b

SQUASH: fix not allowing to link to external valuenode

3497ed1

SQUASH: Just in case some developer try to read the user choices with…

9b0baa6

… a conflict: don't let it happen!

SQUASH : rewording and forcing minimum width for last column

3c7daca

SQUASH add dialog title

c412f5d

SQAUSH: avoid infinite loop whil sweeping layers and valuenodes

bc73c56

SQUASH: add new column: value node pointer

928d479

SQUASH: handle two different exported valuenodes with same ID

7160e2e

And don't let user choose to copy a value node that is a dependency for another valuenode that user chose to be a foreign link

SQUASH sort headers

45ab6e6

SQUASH remove useless var

7bf4a1f

SQUASH: Follow Konstantin suggestion: better instruction text

b1fa309

SQUASH: better status icons for external-linking and local-linking

edcb8b5

SQUASH: fix nested exported-values being copied

ee48e80

SQUASH minor cleanup

9749266

SQUASH: update renamed methods etl::strprintf and Node::is_descendant

009c2de

they were renamed in fb75250 and 920d20f

SQUASH avoid string copy, just resize

8f55096

rodolforg force-pushed the fix-1393-copy-paste-dialog branch from e314c10 to 8f55096 Compare April 23, 2023 12:41

ice0 approved these changes Apr 24, 2023

View reviewed changes

ice0 merged commit 571d7ea into synfig:master Apr 24, 2023

rodolforg mentioned this pull request Apr 24, 2023

Paste layer dialog: renaming an external valuenode does not allow to link/copy its child exported valuenodes #3058

Open

rodolforg deleted the fix-1393-copy-paste-dialog branch April 24, 2023 23:04

Uh oh!

fix: pasting of external exported valuenodes #2086

fix: pasting of external exported valuenodes #2086

Uh oh!

Conversation

rodolforg commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 31, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

Uh oh!

rodolforg commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodolforg commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morevnaproject commented May 3, 2021

Uh oh!

rodolforg commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodolforg commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morevnaproject commented May 5, 2021

Uh oh!

morevnaproject commented May 5, 2021

Uh oh!

morevnaproject commented May 5, 2021

Uh oh!

rodolforg commented May 8, 2021

Uh oh!

rodolforg commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morevnaproject commented May 26, 2021

Uh oh!

ice0 commented May 27, 2021

Uh oh!

rodolforg commented May 28, 2021

Uh oh!

rodolforg commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morevnaproject commented May 28, 2021

Uh oh!

rodolforg commented Apr 23, 2023

Uh oh!

ice0 commented Apr 24, 2023

Uh oh!

Uh oh!

rodolforg commented Mar 30, 2021 •

edited

Loading

ghost commented Mar 31, 2021 •

edited by ghost

Loading

rodolforg commented Apr 1, 2021 •

edited

Loading

rodolforg commented Apr 1, 2021 •

edited

Loading

rodolforg commented May 3, 2021 •

edited

Loading

rodolforg commented May 3, 2021 •

edited

Loading

rodolforg commented May 8, 2021 •

edited

Loading

rodolforg commented May 28, 2021 •

edited

Loading