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.

Sign up for GitHub

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

Jump to bottom

Open up the DebugOverlay to add/remove MetricsModes #2316

Merged
merged 8 commits into from May 26, 2016
Merged

Open up the DebugOverlay to add/remove MetricsModes #2316

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
751b17d
Add ListOrderedSet, Edit DebugOverlay and MetricsMode, register and u…
tdgunes May 11, 2016
f22f5bc
Remove ListOrderedSet, Add DebugRenderingSystem
tdgunes May 14, 2016
0710a66
Rename DebugRenderingSystem to DebugMetricsSystem, add JavaDocs, guar…
tdgunes May 19, 2016
e7c853b
Add CircularToggleSet (a new data structure), DebugMetricsSystemTest,…
tdgunes May 21, 2016
faa2f24
Fix corner case, removing defaultMode, add its test
tdgunes May 21, 2016
92dfda5
Remove collection.CircularToggleSet, add a primitive version to Debug…
tdgunes May 22, 2016
e42d6b9
Edit DebugMetricsSystem, remove CircularToggleSet
tdgunes May 23, 2016
fa3ef13
Refactor wild card import
tdgunes May 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions .../test/java/org/terasology/rendering/nui/layers/ingame/metrics/DebugMetricsSystemTest.java
View file
Open in desktop
Expand Up @@ -132,6 +132,18 @@ public void testRemovingDefaultMetric() {
system.unregister(defaultMode);
}

@Test(expected = NullPointerException.class)
public void testRegisterNull() {
DebugMetricsSystem system = getEmptySystem();
system.register(null);
}

@Test(expected = NullPointerException.class)
public void testUnregisterNull() {
DebugMetricsSystem system = getEmptySystem();
system.unregister(null);
}

@Test
public void testUnregisterWithToggleRemovingNext() {
DebugMetricsSystem system = getEmptySystem();
Expand Down
197 changes: 0 additions & 197 deletions engine-tests/src/test/java/org/terasology/utilities/collection/CircularToggleSetTest.java
View file
Open in desktop

This file was deleted.

43 changes: 39 additions & 4 deletions .../src/main/java/org/terasology/rendering/nui/layers/ingame/metrics/DebugMetricsSystem.java
View file
Open in desktop
Expand Up @@ -16,10 +16,12 @@
package org.terasology.rendering.nui.layers.ingame.metrics;


import com.google.api.client.repackaged.com.google.common.base.Preconditions;
import org.terasology.entitySystem.systems.BaseComponentSystem;
import org.terasology.entitySystem.systems.RegisterSystem;
import org.terasology.registry.Share;
import org.terasology.utilities.collection.CircularToggleSet;

import java.util.ArrayList;


/**
Expand Down Expand Up @@ -52,7 +54,7 @@ public void initialise() {
register(new RunningThreadsMode());
register(new WorldRendererMode());
register(new RenderingExecTimeMeansMode("Rendering - Execution Time: Running Means - Sorted Alphabetically"));
toggle();
currentMode = defaultMode;
}


Expand All @@ -62,6 +64,7 @@ public void initialise() {
* @param mode a MetricsMode instance
*/
public boolean register(MetricsMode mode) {
Preconditions.checkNotNull(mode, "Illegal argument passed, mode is null.");
return modes.add(mode);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the getCurrentMode() javadoc:

Returns current mode, initializes a default {@link NullMetricsMode}, if currentMode is null

The second part of the sentence is no longer true. I'd simply write:

* @return the current MetricsMode instance

nothing else.


Expand Down Expand Up @@ -95,8 +98,9 @@ public MetricsMode toggle() {
* @return True if the MetricsMode instance was in the set. False otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

Above, in the toggle() method, I'd add the following comment line, directed to fellow developers, between the end of the while loop and the return statement:

// This loop will always find at least one available MetricsMode instance, the defaultMode,
// hence never running indefinitely.

It is possible of course to arrive to that conclusion by looking at the rest of the code, but I figure we can make things easier to the techno-archeologist who will dig up this piece of code 1378 yeas from now. =)

*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd write:

/**
Removes from the set the MetricsMode instance provided as input.

If the MetricsMode instance is the mode currently pointed at by the iterator, toggles the iterator forward.

@return True if the MetricsMode instance was in the set. False otherwise.
*/

public boolean unregister(MetricsMode mode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the previous comment regarding this method I suggested to add to the javadoc the exceptions this method throws. There's also this form of method definition that includes the exceptions thrown:

public boolean unregister(MetricsMode mode) throws except1, except2 {

I'm not sure when to do it only in the javadoc, when to do it only in the method definition and when to do it both ways. @msteiger: Maaaaaaaaaartin?!?!?!?

By the way, the same applies to the register method.

Preconditions.checkNotNull(mode, "Illegal argument passed, mode is null.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the javadoc with the exceptions this method might throw.

if (mode == defaultMode) {
throw new IllegalArgumentException("Removing defaultMode is not allowed!");
throw new IllegalArgumentException("Removing defaultMode is not allowed.");
Copy link
Contributor

Choose a reason for hiding this comment

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

@Cervator: this is the exception I was asking you about, but I've come to the conclusion it is the right thing to do, so I won't ask confirmation after all.

}

if (mode == currentMode) {
Expand All @@ -112,7 +116,7 @@ public boolean unregister(MetricsMode mode) {
public void unregisterAll() {
modes.clear();
register(defaultMode);
toggle();
currentMode = defaultMode;
}

/**
Expand All @@ -121,4 +125,35 @@ public void unregisterAll() {
public int getNumberOfModes() {
return modes.size();
}


private class CircularToggleSet<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't know why you didn't simply use an ArrayList straight into DebugMetricsSystem rather than wrap it into a private container class. It's not like the DMS is a particularly complicated piece of code. =) But I think we have argued enough over this PR and I'm ok leaving this as it is just because you like it this way. =)

ArrayList<T> container = new ArrayList<>();
int cursor;

public int size() {
return container.size();
}

public void clear() {
cursor = 0;
container.clear();
}

public boolean remove(T mode) {
return container.remove(mode);
}

public T toggle() {
cursor = (cursor + 1) % size();
return container.get(cursor);
}

public boolean add(T mode) {
if (!container.contains(mode)) {
return container.add(mode);
}
return false;
}
}
}