Fix advanced chunk monitoring system #3499
Conversation
|
Hooray Jenkins reported success with all tests good! |
llvieira
changed the title
|
Hooray Jenkins reported success with all tests good! |
|
Hooray Jenkins reported success with all tests good! |
llvieira
changed the title
|
Hooray Jenkins reported success with all tests good! |
|
|
||
| add(tabs, BorderLayout.CENTER); | ||
| } | ||
|
|
||
| /** | ||
| * This method goes through three steps: |
There was a problem hiding this comment.
Better to start any Javadoc with a brief single sentence describing the overall goal of the method, then optionally dig into the details later. First sentence gets used in the summary box in the Javadoc site, which in this case would thus end up solely containing "This method goes through three steps:" :-)
| private final transient Runnable renderTask; | ||
|
|
||
| /** | ||
| * If true, the active monitoring thread in executor service should stop. |
There was a problem hiding this comment.
You can use the one-line format here since it is a field and tiny
| * If true, the active monitoring thread in executor service should stop. | |
| /** If true, the active monitoring thread in executor service should stop. */ |
(Trying out the new GitHub "Suggestion" feature, but it originates in one line yet my suggestion replaces three lines - hmm)
| @@ -105,13 +104,18 @@ public ChunkMonitorDisplay(int refreshInterval, int chunkSize) { | |||
| addMouseListener(ml); | |||
| addMouseMotionListener(ml); | |||
| addMouseWheelListener(ml); | |||
| this.followPlayer = true; | |||
There was a problem hiding this comment.
With the removal of setFollowPlayer this var itself might be obsolete since there's no way to change it. Probably either keep the setter in expectation that it'll be added as a toggle in the UI sometime or just remove it entirely :-)
There was a problem hiding this comment.
I decided to keep the setter, so we can open an issue to add that new UI functionality.
| public static final Color COLOR_INVALID = Color.red; | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(ChunkMonitorDisplay.class); | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(ChunkMonitorDisplay.class); |
There was a problem hiding this comment.
All caps logger or not is a tricky subject. See https://stackoverflow.com/questions/1417190/should-a-static-final-logger-be-declared-in-upper-case - generally we've kept it lower case so probably best to stay consistent that way, at least for now :-)
There was a problem hiding this comment.
Indeed, according to Java Code Style is should be lower-case. I changed it after CheckStyle scan, however CheckStyle cannot check if this object reference is never followed by a ".".
| @@ -694,12 +631,12 @@ private void renderSelectedChunk(Graphics2D g, int offsetx, int offsety, Vector3 | |||
| } | |||
|
|
|||
| private void renderBox(Graphics2D g, int offsetx, int offsety, Rectangle box) { | |||
| g.setColor(Color.white); | |||
| g.setColor(Color.WHITE); | |||
There was a problem hiding this comment.
This case change on the other hand is definitely good 👍
|
|
||
| /** | ||
| * If true, the active monitoring thread in executor service should stop. |
|
Hooray Jenkins reported success with all tests good! |
5 similar comments
|
Hooray Jenkins reported success with all tests good! |
|
Hooray Jenkins reported success with all tests good! |
|
Hooray Jenkins reported success with all tests good! |
|
Hooray Jenkins reported success with all tests good! |
|
Hooray Jenkins reported success with all tests good! |
llvieira
force-pushed
the
refator-advanced-monitoring
branch
from
9b7393e
to
d1c5439
Compare
|
Hooray Jenkins reported success with all tests good! |
|
All good, thanks! :-) |
Contains
Fixes #3010. The goal here is to fix the problem of preventing the game from close cleanly. After dive into the code I realize that the Thread created by
ExecutorServiceinThreadMonitorPanel,PerformanceMonitorPanelandChunkMonitorDisplaykeeps running after the game closes. Therefore, I changed a little the way thread monitors work. Also the whole monitor code were cleaned up.To fix the problem I created a variable called
stopThreadfor each of the three monitors. IfstopThreadistruethen the monitor thread leaves the loop and the executor service is shutdown, which will allow the game to close gracefully.How to test
"monitoringEnabled": false, totrueinconfig.cfg(run the game once to get one).