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.

Sign up for GitHub

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

Jump to bottom

Add a meta to hold current process of plot merge #4038

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

Add a meta to hold current process of plot merge #4038

wants to merge 1 commit into from

Conversation

TheMeinerLP
Copy link
Contributor

Overview

Fixes #3056

Description

Fixes the current duping problem when plots are merged in survival.

Submitter Checklist

  • Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
  • Ensure that the pull request title represents the desired changelog entry.
  • New public fields and methods are annotated with @since TODO.
  • I read and followed the contribution guidelines.

@TheMeinerLP TheMeinerLP requested a review from a team as a code owner May 3, 2023 20:41
@github-actions github-actions bot added the Bugfix This PR fixes a bug label May 3, 2023
PierreSchwang
PierreSchwang reviewed May 3, 2023
View reviewed changes
Core/src/main/java/com/plotsquared/core/plot/Plot.java
*/
public Boolean isMerging() {
Boolean value = (Boolean) this.getMeta("merging");
return value != null && value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be enough to just return this.getMeta("merging") != null I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not have a hasMeta ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that Meta-System could use some refactoring either way. A hasMetadata extension would be great, as well updating the backing Map to an EnumMap and have enum constants for each metadata key. I guess that would benefit for maintainability and documentation purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great. Lets coordinate that together I will help you out.

@TheMeinerLP
Copy link
Contributor Author

TheMeinerLP commented May 3, 2023 •
edited

This PR needs be tested. @OneLiteFeather and me have tested locally and sometimes was working and sometimes not. I am now out for today.
I still have some improvements on my pc, but I am not sure if there are working. The cause why I am not pushing it, is the locally testing are so buggy. Sometimes the breakpoints are not holding in the right method/line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dordsor21 dordsor21 dordsor21 left review comments

@PierreSchwang PierreSchwang PierreSchwang left review comments

Assignees
No one assigned
Labels
Bugfix This PR fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

You can break blocks from the road while merging is in progress
3 participants
@TheMeinerLP @dordsor21 @PierreSchwang