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

Multiple instance events anim develop #230

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

ChrisEberleSchool
Copy link
Contributor

@ChrisEberleSchool ChrisEberleSchool commented Feb 4, 2024

Fixed the issue where you were able to open multiple instances of the same event. This change still allows you to open multiple instances of events so long as they are not the same.

BEFORE:
image

Also this same thing was occurring in the event panel in the animation button where multiple same instance animations could be open at once which I fixed!

fixed it so only one instance of the animation panel can be open from a single event instance
when u closed a instance of animation you couldn't reopen it again
- this change ensures you can still open multiple event instances so long as they are different event. If you try opening similiar events they will not open.
@xspanger3770
Copy link
Owner

not sure about this one... array of booleans just feels wrong 💀

@JABirchall
Copy link

JABirchall commented Feb 17, 2024

One bug i can think of with this is, wouldnt the corresponding earthquakes index always be changing as new earthquakes are added and old ones purged?

ArchivedQuakes has a UUID property on it, it would be best to use that to track open windows and animations, the current implimentation is basically a race condition.

xspanger3770 and others added 10 commits February 23, 2024 19:23
Finally update main branch with 0.11.x version features (336 commits) so that my github profile page lights up like a christmas tree and I can start updating the readme files
- I am creating an arrayList of UUID's called openedQuakes. Before a panel can be open its uuid must not be in the array list, checking this with a new  isOpened comparison boolean method.
- this commit handles the animation panel multiple instances.
@ChrisEberleSchool
Copy link
Contributor Author

I fully redid the bugfix in a much simpler way using UUID's as @JABirchall suggested. Should work perfectly now

// loop through openedquakes
for( UUID i: openedQuakes) {
if( i == id) {
return true;

Choose a reason for hiding this comment

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

Would it be possible to use the ArrayList builtin indexOf(id) or contains(id) instead of looping over the array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just replaced my isOpened method with contains! It isn't a huge optimization given how small the openedQuakes ArrayList will be but nonetheless makes the code cleaner so great catch!

openedQuakes.remove(quakeUuid);
}
public void windowClosed(WindowEvent e) {
openedQuakes.remove(quakeUuid);

Choose a reason for hiding this comment

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

Do we need to remove the uuid twice on closing and closed event?
is there a case where 1 of these events doesnt do what it is supposed to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty much for redundancy, I did research and I found it is best to include both! Essentially I could just use windowClosing and it would pretty much cover all cases but I'm pretty sure there are some cases that it wont cover that windowClosed will cover

Choose a reason for hiding this comment

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

All good, just wanted to know the reason around it.

@JABirchall
Copy link

This is ready for @xspanger3770 review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants