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

#4961 #5008 #4954 Bug Fixes #5007

Merged
merged 11 commits into from
Nov 12, 2024
Merged

Conversation

Jmr3366
Copy link
Contributor

@Jmr3366 Jmr3366 commented Oct 18, 2024

Fixes:
#4961 NPE on mouse off (needs testing, I can't validate)
#5008 Stat sheet not in front
#4954 User Overlays break with new statsheets

Description of the Change

#4961 adds an empty var if check. Needs testing as I don't have the OS to validate this change on.
#5008 changed variable used from min to max
#4954 forces sorting for all overlays (removed needsSorting bool)

Possible Drawbacks

None


This change is Reviewable

@cwisniew
Copy link
Member

This overlay may be used in other functionality in the future so the name can't be hard coded like this

@Jmr3366
Copy link
Contributor Author

Jmr3366 commented Oct 20, 2024

It needs more work, I'm still missing something

edit
Created a new init in HTMLOverlayPanel using invokeLater being executed in initialize

@Jmr3366 Jmr3366 changed the title #4961 #4954 Bug Fixes #4961 #5008 #4954 Bug Fixes Oct 21, 2024
@kwvanderlinde
Copy link
Collaborator

It needs more work, I'm still missing something

@Jmr3366 At a glance this look good to me. Is this comment still true?

@Jmr3366
Copy link
Contributor Author

Jmr3366 commented Nov 11, 2024

I think it is all good but wanted you or Craig to QC it.

@@ -117,6 +119,20 @@ public ConcurrentSkipListSet<HTMLOverlayManager> getOverlays() {
return overlays.clone();
}

public HTMLOverlayManager init() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this approach to fixing #4954 actually masks the issue rather than fixing it. Did some testing, I believe the only thing that needs to change is in showOverlay() - we need needsSorting to be true even for internal overlays. Otherwise the new overlay ends up in front of the front region.

Also I have a personal motivation to not have overlays automatically open 🙂 - see #5044. Overlays on my system cause a lot of input lag, and this would force me to endure it despite not using the stat sheet feature yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct on both points (masking,sorting) and I looked right past that, thank you sir!

sorting bool served no immediate purpose
@kwvanderlinde kwvanderlinde added this pull request to the merge queue Nov 12, 2024
Merged via the queue into RPTools:develop with commit 1872708 Nov 12, 2024
4 checks passed
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.

3 participants