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

Layers swap place more logically #1147

Merged
merged 9 commits into from Jan 28, 2019

Conversation

Projects
None yet
5 participants
@davidlamhauge
Copy link
Contributor

davidlamhauge commented Dec 26, 2018

This PR concerns issue #1080.
When you rearrange the layers, the layer you drag and the layer you drop it on, has until now swapped place. This has been considered against logical and expected behavior.
(With this change the the layer you drag, will swap place with the layer it is passing, and thus not rearringing the order like before.) Changed: Look below
I know it is a change to the timeline, but I think we should make life easier for our users, while we wait for the new timeline.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Dec 26, 2018

@davidlamhauge Nice. I agree that this is one of those things that even if it's changed later due to refactoring or re-design is such an essential behavior that I don't think people will complain at all to have it early.

I'll test the fix later, let me know which branch I have to download to test it, thanks! 😉

As a side note, In the referenced issue, I laid out the possible characteristics that such an implementation should have mostly along some quality of life improvements for the user experience. So If this implementation works out for most purposes I'll edit the list and check the main functionality as done, leaving the issue itself open for the other related enhancements to be considered and discussed in a future sprint.

Thanks again!

@davidlamhauge

This comment has been minimized.

Copy link
Contributor Author

davidlamhauge commented Dec 27, 2018

@Jose-Moreno The branch is called iss1080_layerdragging. There is a direct link to the branch on the PR page, just above the "All checks have passed" box.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Dec 27, 2018

@davidlamhauge I think I need glasses to read now, didn't see that before. Thanks will test it out tomorrow! 😄

@davidlamhauge

This comment has been minimized.

Copy link
Contributor Author

davidlamhauge commented Dec 27, 2018

Had to change the workings, so the layer swaps after you drop it.
The first version worked fine if you used it normally, but you could get it to crash if you dragged it at lightning speed.

@davidlamhauge davidlamhauge changed the title Layers swap place as they pass each other Layers swap place more logically Dec 27, 2018

CandyFace and others added some commits Jan 3, 2019

Merge pull request #2 from CandyFace/iss1080_layerdragging
Fix layer gutter inaccurate painting
@CandyFace
Copy link
Member

CandyFace left a comment

Minor stuff but I'd like to see them fixed.

I appreciate you having addressed this problem, even if we've talked about not working too much on the timeline.

Show resolved Hide resolved core_lib/src/interface/timelinecells.cpp Outdated
Show resolved Hide resolved core_lib/src/interface/timelinecells.cpp Outdated
Show resolved Hide resolved core_lib/src/interface/timelinecells.h Outdated
@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Jan 4, 2019

I've approved the PR, although the issue mentions that it should be possible to switch mode from dragging in between 2 layers to switching like the old behaviour would with a modifier key or similar.

I'd consider that a feature for the new timeline though, let's just focus on fixing the remaining timeline bugs and leave new stuff to timeline rewrite.

scribblemaniac and others added some commits Jan 17, 2019

Layers move to closest space
Also:
- Refactor moveLayer to swapLayers to better convey its purpose.
- Eliminate the need for a temporary layer
Merge pull request #3 from scribblemaniac/layerdragging
Suggested improvements for layer swapping
@scribblemaniac
Copy link
Member

scribblemaniac left a comment

Looks good @davidlamhauge! Everything you did looked good and was greatly improved over the original behavior. I thought that dropping in the nearest spot would be more natural, hence the suggested enhancements.

@scribblemaniac

This comment has been minimized.

Copy link
Member

scribblemaniac commented Jan 17, 2019

@CandyFace Can you please review the additional changes I've added before we merge this?

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Jan 17, 2019

Yes I'll review and test when I get home from work ;)

@CandyFace
Copy link
Member

CandyFace left a comment

With the newest changes, there's a bug when moving a layer from bottom to top.
The result is:
-- layer1
-- layer2
-- layer3 <- selected

after dragging:
-- layer3
-- layer1 <- selected
-- layer2

However changing Editor::swaplayers
to..

    if (j < i)
    {
        layers()->setCurrentLayer(j + 1);
    }
    else
    {
        layers()->setCurrentLayer(j - 1);
    }

fixes the problem.

When that's been fixed, then it should be 👍

@CandyFace
Copy link
Member

CandyFace left a comment

Code wise it looks good now, I haven't tested it today but assuming I got around all cases yesterday when I reviewed and tested, then I'd say it works as expected.

@chchwy chchwy merged commit 5e009ff into pencil2d:master Jan 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chchwy chchwy added this to the 0.6.3 milestone Jan 28, 2019

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Jan 28, 2019

Tested and looks good to me. Thanks @davidlamhauge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.