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

Fix: Inconsistent layer name if there's conversion from background to layer #4090

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tetektoza
Copy link

@tetektoza tetektoza commented Oct 6, 2023

This patch fixes #4084.

Currently if we set layer to background and then back to layer, last used name isn't preserved. This patch makes layer save last used layer name before it gets converted to background so it won't get lost.

I agree that my contributions are licensed under the Individual Contributor License Agreement V4.0 ("CLA") as stated in https://github.com/igarastudio/cla/blob/main/cla.md

I have signed the CLA following the steps given in https://github.com/igarastudio/cla#signing

@tetektoza
Copy link
Author

tetektoza commented Oct 6, 2023

This works one way only right now, so if we convert from Layer -> Background, but let me know what you think about this solution, I can implement it in a similar way so it will save layer name for background as well.

//edit - I've added support for saving last background name as well, you can check it out, I can always change it :)

…layer

This patch fixes aseprite#4084.

Currently if we set layer to background and then back to layer, last
used name isn't preserved. This patch makes layer save last used layer name before
it gets converted to background so it won't get lost.
@dacap
Copy link
Member

dacap commented Oct 6, 2023

Thanks @tetektoza, but I think it's an overkill to add two new string fields to all doc::Layer objects to fix this UI problem.

Probably we should fix only the "Layer 0" name problem, and the solution would be to use some code similar to NewLayerCommand::getUniqueLayerName() (merging common code in some util functions).

@dacap dacap self-assigned this Oct 6, 2023
@tetektoza
Copy link
Author

Thanks @tetektoza, but I think it's an overkill to add two new string fields to all doc::Layer objects to fix this UI problem.

Probably we should fix only the "Layer 0" name problem, and the solution would be to use some code similar to NewLayerCommand::getUniqueLayerName() (merging common code in some util functions).

Hmm, and how about if we tried to do it with string_view somehow?

@dacap
Copy link
Member

dacap commented Oct 6, 2023

how about if we tried to do it with string_view somehow?

A string_view still needs a memory buffer to store the string (another issue is that we are not yet using string views, but it's unrelated to this). Not sure how a string_view would solve the issue here.

The idea would be to avoid storing extra strings in the doc library just to solve one problem that is generated in the UI level (app module).

@tetektoza
Copy link
Author

tetektoza commented Oct 9, 2023

@dacap Coming back to this - how do you want it to work? If user changes let's say from Layer 1 to Background and then from Background to Layer, should the number be +1 from the current highest? Like if we have the highest number as Layer 4, the conversion from Background to layer will give it a new number - 5, or should it save last used number and use it? So if we had Layer 1 before conversion to Background, then converting it back to layer will make it Layer 1 once again, even if there's another layer with higher number?

If we decide for the first option, then I guess we can move some code as you said from getUniqueLayerName to .cpp file which is holding some util functions for layers, how about it?

@dacap
Copy link
Member

dacap commented Nov 3, 2023

At the moment I think this bug is quite low priority, if a simple solution (without modifying the doc library) can be found we can think in merging this. Right now, as it is working, it's not quite bad and not a critical bug.

@dacap dacap removed their assignment Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent layer number if we change from background layer to layer
2 participants