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

Update holograms on PlotChangeOwnerEvent in correct way #109

Merged
merged 4 commits into from
Oct 23, 2022
Merged

Update holograms on PlotChangeOwnerEvent in correct way #109

merged 4 commits into from
Oct 23, 2022

Conversation

BlockyTheDev
Copy link
Contributor

@BlockyTheDev BlockyTheDev commented Oct 8, 2022

Overview

Fixes #43

Description

This fixes a bug, where new holograms where created on PlotChangeOwnerEvent, but the old one wasn't removed. Because of that there was a huge stack of armorstands and unreadable text created over the time. With this pull request it is going to be deleted first and then the a new one ist created.

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.

@BlockyTheDev BlockyTheDev requested a review from a team as a code owner October 8, 2022 22:46
@BlockyTheDev BlockyTheDev changed the title Update holograms on PlotChangeOwnerEvent correct. Update holograms on PlotChangeOwnerEvent correct Oct 8, 2022
@BlockyTheDev BlockyTheDev changed the title Update holograms on PlotChangeOwnerEvent correct Update holograms on PlotChangeOwnerEvent in correct way Oct 8, 2022
@SirYwell
Copy link
Member

SirYwell commented Oct 9, 2022

I'd assume this call


to already prevent the issue from happening in the first place, do you know why that isn't the case?

@BlockyTheDev
Copy link
Contributor Author

BlockyTheDev commented Oct 9, 2022

Ohh, I should have looked into the method updatePlayer. As far as I see it should handle the "replacement" of the lines, which is not working right, too.

Sorry for my English. I am a German student.

@BlockyTheDev
Copy link
Contributor Author

BlockyTheDev commented Oct 9, 2022

I found out why the updatePlayer methode isn't working in the way it should. I will investigate that when I have time in the next days.

Edit: It is not working, because in this line the plot has a new owner, so the hash in the HoloPlotID instance is a different one, in comparison to the stored hologram in the HashMap.

HoloPlotID id = new HoloPlotID(plot.getId(), plot.getOwnerAbs());

Because of that it is entering following if statement block:
if (!holograms.containsKey(id)) {

The options I am seeing to fix this, is (1) adding an UUID argument with the old player to the updatePlayer methode, which gets the result of e.getOldPlayer() on the call in onPlotChangeOwner and in the other calls from the other subscribed events null or (2) removing the hologram in onPlotChangeOwner (without missing to remove it from the HashMap) and then calling the updatePlayer methode again.

If it would be my plugin i would solve it in the second way. @SirYwell just write me the way I should use (If there is a better one just write too) then I am going to make a new commit in this pr tomorrow.

@SirYwell
Copy link
Member

Edit: It is not working, because in this line the plot has a new owner, so the hash in the HoloPlotID instance is a different one, in comparison to the stored hologram in the HashMap.

Good catch, that makes sense. I think your second suggestion makes sense, but there should be a comment in the code explaining it briefly.

@BlockyTheDev
Copy link
Contributor Author

Now it is removing the HoloPlotID key completely from the HashMap. I also added an information comment, why this way is selected.

Copy link
Member

@SirYwell SirYwell left a comment

Choose a reason for hiding this comment

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

One minor nitpick, but looks good otherwise.

src/main/java/com/plotsquared/holoplots/PSHoloUtil.java Outdated Show resolved Hide resolved
Co-authored-by: Hannes Greule <SirYwell@users.noreply.github.com>
Copy link
Member

@SirYwell SirYwell left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

Copy link
Member

@PierreSchwang PierreSchwang left a comment

Choose a reason for hiding this comment

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

Looks reasonable

@NotMyFault NotMyFault enabled auto-merge (squash) October 23, 2022 19:23
@NotMyFault NotMyFault merged commit d8ca20d into IntellectualSites:main Oct 23, 2022
@NotMyFault NotMyFault added the bug Something isn't working label Mar 23, 2023
@NotMyFault NotMyFault added Bugfix This PR fixes a bug and removed bug Something isn't working labels Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix This PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't append additional holograms on PlotChangeOwnerEvent
4 participants