-
Notifications
You must be signed in to change notification settings - Fork 53
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
base: develop
Are you sure you want to change the base?
Multiple instance events anim develop #230
Conversation
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.
not sure about this one... array of booleans just feels wrong 💀 |
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. |
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
Update readme branch
reverts to standard
- 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.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This is ready for @xspanger3770 review. |
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:
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!