Fixes to various visualization problems #126
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes to be committed: modified: jung-algorithms/pom.xml modified: jung-algorithms/src/main/java/edu/uci/ics/jung/algorithms/layout/util/VisRunner.java new file: jung-algorithms/src/main/resources/log4j.properties new file: jung-algorithms/src/test/java/edu/uci/ics/jung/algorithms/layout/LayoutTest.java modified: jung-samples/pom.xml modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/AddNodeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/AnimatingAddNodeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/BalloonLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ClusteringDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/EdgeLabelDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/GraphFromGraphMLDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/GraphZoomScrollPaneDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/L2RTreeLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/LensDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/PluggableRendererDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/SatelliteViewDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ShortestPathDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ShowLayouts.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/SubLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/TreeCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/TreeLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemoWithLayouts.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexImageShaperDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexLabelAsShapeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VisualizationImageServerDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/WorldMapGraphDemo.java modified: jung-visualization/pom.xml modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/DefaultVisualizationModel.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/PluggableRenderContext.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/RenderContext.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/VisualizationModel.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/VisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EditingModalGraphMouse.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EditingPopupGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SatelliteVisualizationViewer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SimpleEdgeSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SimpleVertexSupport.java new file: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/AbstractEdgeShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/EdgeShape.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/GradientEdgePaintTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/ParallelEdgeShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/BoundingRectangleCollector.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/BoundingRectanglePaintable.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutChangeListener.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutEvent.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutEventSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutTransition.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/ObservableCachingLayout.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/ClosestShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/LayoutLensShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/ShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/ViewLensShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicEdgeLabelRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicEdgeRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicVertexLabelRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicVertexRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/CachingEdgeRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/CachingRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/CachingVertexRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/GradientVertexRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/Renderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/ReshapingEdgeRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/VertexLabelAsShapeRenderer.java new file: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/Spatial.java new file: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/SpatialGrid.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/subLayout/GraphCollapser.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/transform/shape/MagnifyImageLensSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/transform/shape/ViewLensSupport.java new file: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/util/Context.java new file: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/util/LayoutMediator.java new file: jung-visualization/src/main/resources/log4j.properties modified: pom.xml
|
Can you summarize at a medium-high level what changes are involved here? Also, did you intend to commit the log4j.properties file? |
|
The first thing that pops out to me is that log4j is being used for logging. Do we really need logging for JUNG? If we do really need logging, then I believe slf4j should at least be used as a frontend for log4j, to better allow users to swap out log4j for their preferred logging framework if they desire. |
jrtom
reviewed
Sep 25, 2017
| @@ -10,6 +10,7 @@ | |||
| package edu.uci.ics.jung.algorithms.layout.util; | |||
|
|
|||
| import edu.uci.ics.jung.algorithms.util.IterativeContext; | |||
| import org.apache.log4j.Logger; | |||
There was a problem hiding this comment.
Please remove the logging code (here and elsewhere)--both the Logger stuff and the System.out.println()s.
Changes to be committed: modified: jung-algorithms/pom.xml modified: jung-algorithms/src/main/java/edu/uci/ics/jung/algorithms/layout/util/VisRunner.java deleted: jung-algorithms/src/main/resources/log4j.properties deleted: jung-algorithms/src/test/java/edu/uci/ics/jung/algorithms/layout/LayoutTest.java
|
I removed all changes to jung-algorithms.
…On Mon, Sep 25, 2017 at 11:37 AM, jrtom ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In jung-algorithms/src/main/java/edu/uci/ics/jung/algorithms/
layout/util/VisRunner.java
<#126 (comment)>:
> @@ -10,6 +10,7 @@
package edu.uci.ics.jung.algorithms.layout.util;
import edu.uci.ics.jung.algorithms.util.IterativeContext;
+import org.apache.log4j.Logger;
Please remove the logging code (here and elsewhere)--both the Logger stuff
and the System.out.println()s.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#126 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbB7yAxUfJS0RfScdT7Twj74SpGsXAks5sl8iqgaJpZM4Pi6ec>
.
|
|
- removed all the statefulness (mostly of Network) that broke many demos
- i added a class to connect Network with Layout in visualization so you don't have to do it elsewhere.
- yes, i intended to add logging, but just to visualization and samples
…On Mon, Sep 25, 2017 at 11:35 AM, jrtom ***@***.***> wrote:
Can you summarize at a medium-high level what changes are involved here?
Also, did you intend to commit the log4j.properties file?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#126 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbBxD6eU4ZCH__c5rSDcfr2kRx0Pqxks5sl8hKgaJpZM4Pi6ec>
.
|
|
OK. I will take a look at this as soon as I can, but I want to finish throwing together the design doc I mentioned in #104 first, and then I'd like to see those designs compared in that doc before I make a final call on this. Thanks for putting this together, at any rate; I think that it will really help to be able to see how your ideas are working in action. |
Changes to be committed: modified: jung-visualization/pom.xml modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/DefaultVisualizationModel.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutTransition.java
Changes to be committed: modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/DefaultVisualizationModel.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutTransition.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/SpatialGrid.java
jbduncan
reviewed
Sep 25, 2017
| layout.setInitializer(vv.getGraphLayout()); | ||
| layout.setSize(vv.getSize()); | ||
| System.err.println("using graph " + graph_names[graph_index]); | ||
| System.err.println("using layout: " + layout.getClass()); |
There was a problem hiding this comment.
@tomnelson I'm a bit puzzled by these System.err.println statements. Did you mean to leave them in? If so, could you explain your reasoning to me?
jung-visualization/pom.xml
Outdated
| <dependencies> | ||
| <dependency> | ||
| <groupId>commons-logging</groupId> | ||
| <artifactId>commons-logging</artifactId> | ||
| <version>${version.commons-logging}</version> |
There was a problem hiding this comment.
@tomnelson I'm also a bit puzzled by this Commons Logging import. Could you explain for me why we need it on top of log4j?
jung-visualization/pom.xml
Outdated
| <dependency> | ||
| <groupId>org.slf4j</groupId> | ||
| <artifactId>slf4j-log4j12</artifactId> | ||
| <version>1.7.5</version> |
There was a problem hiding this comment.
I think the slf4j version number should be extracted as a property, just like log4j.
| public void setLayoutMediator(LayoutMediator<V, E> layoutMediator) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("setLayoutMediator to " + layoutMediator); | ||
| } |
There was a problem hiding this comment.
It's not really clear to me that a JUNG user would find this logging useful. After all, the typical developer has access to a debugger for cases like this, and if JUNG ends up used in a command-line application, we would clutter the output log, annoying users (who would experience it firsthand) and developers (who would have to find a way to control or disable it) alike.
| spatialTransform = spatialTransform.createInverse(); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
I wonder if it would be better to let exceptions thrown here to propogate to the caller.
| /** | ||
| * A Multimap of box number to Lists of nodes in that box | ||
| * | ||
| * @return |
There was a problem hiding this comment.
Not clear to me that this is useful. Please remove.
| * | ||
| * @param boxX | ||
| * @param boxY | ||
| * @return |
There was a problem hiding this comment.
Please fill these out or remove them.
| * given a box number, return the x,y coordinates in the grid coordinate system | ||
| * | ||
| * @param boxIndex | ||
| * @return |
There was a problem hiding this comment.
Please fill these out or remove them
There was a problem hiding this comment.
This applies to all methods in this class.
| import java.util.Set; | ||
| import java.util.function.Function; | ||
|
|
||
| /** Created by Tom Nelson on 9/22/17. */ |
There was a problem hiding this comment.
Please remove this comment and all other "Created by..." comments like this.
There was a problem hiding this comment.
Is someone planning to remove all of the author names in comments for the 'legacy' code?
There was a problem hiding this comment.
Good question. :)
Not yet, but I've added it as an item to #101, so it's tracked now.
There was a problem hiding this comment.
Why do you want to remove authors from the comments?
There was a problem hiding this comment.
There's a couple of reasons. Firstly, I wanted the author(s) removed because I automatically associate comments of this style with auto-generated comments from IDEs, which I personally find unhelpful. Secondly, I just simply hadn't realised before writing my initial comment above that comments like this do already exist in the code base.
I personally advocate either removing author comments of this style, or alternatively rejigging them so they have the following format (which I believe is more standard in the Java ecosystem). But if you'd rather keep author comments as they are for consistency, I'd totally understand.
/**
* (Class javadoc goes here.)
*
* @author Tom Nelson
*/
pom.xml
Outdated
| @@ -176,7 +176,7 @@ | |||
| <execution> | |||
| <phase>verify</phase> | |||
| <goals> | |||
| <goal>check</goal> | |||
| <goal>format</goal> | |||
There was a problem hiding this comment.
Please do NOT change this. If you want to format code, you can run mvn fmt:format as described in CONTRIBUTING.md. :)
There was a problem hiding this comment.
FYI, the reason to not change this is because Travis CI depends on the mvn fmt command working this way to check that people have properly formatted their code before check-in.
There was a problem hiding this comment.
If you leave it at check, then the build fails. I hope nobody is checking in code when the build failed. If your goal is to have the code formatted, why not have formatting be a part of every build? What am I missing here?
There was a problem hiding this comment.
In theory, no one should be checking in code when the build failed, because we have the Travis check to forestall that (at least for the common.graph and master branches).
I haven't formed a strong opinion on this issue yet, but I think that the point of leaving it as a check is to not require formatting on every build; I can imagine that there might be circumstances in which you might want to leave the formatting until you were done developing.
But maybe there's something I'm missing; @jbduncan, can you elaborate?
There was a problem hiding this comment.
@jrtom I admit I'm not too clear either on why checking is preferred to formatting at CI-time. I think it's a combination of reasons.
I can think of two reasons ATM. Firstly, it's partially to do with how formatters (google-java-format included) can sometimes contain bugs which can cause code to be formatted weirdly, or to flat-out fail with an error or exception. So, if formatting happened during CI-time, the CI may fail or format things strangely for reasons not to do with the checked-in code, which has the potential to confuse things.
Secondly, formatters are prone to occasionally formatting code incompletely, thus requiring formatters to be run twice or so. Thus, formatting by CI wouldn't be guaranteed to be hermetic, which again is a cause for confusion.
I actually experienced this "problem" (if you'd call it that) with google-java-format when I was working on introducing it to JUNG a good few weeks/months ago; it required me to run mvn fmt:format twice before certain source files were formatted completely, strangely.
I'm hoping @nedtwigg, who I collaborate with on Spotless, can elaborate further on this, because I'm sure an issue was opened in the past on Spotless's issue tracker asking why Spotless's spotlessCheck task (similar to mvn fmt:format for us) is the default over spotlessApply (similar to mvn fmt:check), but I can't find that issue yet.
There was a problem hiding this comment.
Replying to #126 (comment):
@tomnelson That sounds good to me, having a default maven profile for formatting code and a CI-specific one for checking code.
Would you be happy to create a new PR for that?
If not, I may have the time and energy in the foreseeable future to investigate Maven profiles and create them myself.
There was a problem hiding this comment.
We definitely wouldn't want the CI check to be formatting code itself; I don't want an automated CI check to be trying to modify the code (I don't even know what that would do, but I feel no strong need to find out :) ).
So I would say that until we have multiple profiles that we should keep it at "check".
There was a problem hiding this comment.
Replying to comment #126 (comment):
@tomnelson It should be easy to apply the new profile without needing admin permissions. I assume that profiles can be chosen from the command-line? If so, all you'd need to do is change this line in the Travis config to ensure it runs the right profile.
|
Thank you for being thorough with your review of my changes to this
snapshot branch. Did you happen to build and run any of the demos?
(and when you did, did you see any log messages?)
I spent quite a bit of time figuring out how visualization became so
broken. I also enjoyed revisiting the codebase and experimenting with
spatial data structures. There is a lot that can be done in that area.
I agree that a lot of cleanup of this snapshot branch is required before
release. I will hold off until I have some idea where the rest of the
project is going.
Nice to meet you!
…On Mon, Sep 25, 2017 at 1:33 PM, Jonathan Bluett-Duncan < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In jung-samples/src/main/java/edu/uci/ics/jung/samples/ShowLayouts.java
<#126 (comment)>:
> layout.setInitializer(vv.getGraphLayout());
layout.setSize(vv.getSize());
+ System.err.println("using graph " + graph_names[graph_index]);
+ System.err.println("using layout: " + layout.getClass());
@tomnelson <https://github.com/tomnelson> I'm a bit puzzled by these
System.err.println statements. Did you mean to leave them in? If so,
could you explain your reasoning to me?
------------------------------
In jung-visualization/pom.xml
<#126 (comment)>:
> <dependencies>
<dependency>
+ <groupId>commons-logging</groupId>
+ <artifactId>commons-logging</artifactId>
+ <version>${version.commons-logging}</version>
@tomnelson <https://github.com/tomnelson> I'm also a bit puzzled by this
Commons Logging import. Could you explain for me why we need it on top of
log4j?
------------------------------
In jung-visualization/pom.xml
<#126 (comment)>:
> <dependencies>
<dependency>
+ <groupId>commons-logging</groupId>
+ <artifactId>commons-logging</artifactId>
+ <version>${version.commons-logging}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ <version>1.7.5</version>
+ </dependency>
+
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-log4j12</artifactId>
+ <version>1.7.5</version>
I think the slf4j version number should be extracted as a property, just
like log4j.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
BasicVisualizationServer.java
<#126 (comment)>:
> @@ -224,12 +228,19 @@ public void setRenderer(Renderer<V, E> r) {
return renderer;
}
- public void setGraphLayout(Layout<V> layout) {
+ public void setLayoutMediator(LayoutMediator<V, E> layoutMediator, Dimension d) {
+ model.setLayoutMediator(layoutMediator, d);
+ }
+
+ public void setLayoutMediator(LayoutMediator<V, E> layoutMediator) {
+ if (log.isDebugEnabled()) {
+ log.debug("setLayoutMediator to " + layoutMediator);
+ }
It's not really clear to me that a JUNG user would find this logging
useful. After all, the typical developer has access to a debugger for cases
like this, and if JUNG ends up used in a command-line application, we would
clutter the output log, annoying users (who would experience it firsthand)
and developers (who would have to find a way to control or disable it)
alike.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
BasicVisualizationServer.java
<#126 (comment)>:
> @@ -304,7 +315,48 @@ protected void renderGraph(Graphics2D g2d) {
newXform.concatenate(
renderContext.getMultiLayerTransformer().getTransformer(Layer.VIEW).getTransform());
+ AffineTransform spatialTransform = new AffineTransform(oldXform);
+ spatialTransform.concatenate(
+ renderContext.getMultiLayerTransformer().getTransformer(Layer.VIEW).getTransform());
+ spatialTransform.concatenate(
+ renderContext.getMultiLayerTransformer().getTransformer(Layer.LAYOUT).getTransform());
+
+ try {
+ spatialTransform = spatialTransform.createInverse();
+ } catch (Exception e) {
+ e.printStackTrace();
+ }
I wonder if it would be better to let exceptions thrown here to propogate
to the caller.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
BasicVisualizationServer.java
<#126 (comment)>:
> + Dimension layoutSize = model.getLayoutMediator().getLayout().getSize();
+ AffineTransform layoutTransform =
+ renderContext.getMultiLayerTransformer().getTransformer(Layer.LAYOUT).getTransform();
+ Rectangle2D layoutRectangle = new Rectangle2D.Double(0, 0, layoutSize.width, layoutSize.height);
+ Shape bigger = layoutTransform.createTransformedShape(layoutRectangle);
+ if (log.isDebugEnabled()) {
+ log.debug(
+ "layoutXform scale is "
+ + renderContext
+ .getMultiLayerTransformer()
+ .getTransformer(Layer.LAYOUT)
+ .getTransform()
+ .getScaleX());
+ log.debug("newXform scale is " + newXform.getScaleX());
+ log.debug("this thing is " + bigger.getBounds());
+ }
Again, it's not really clear to me why we're logging this debug
information.
------------------------------
In jung-visualization/pom.xml
<#126 (comment)>:
> @@ -9,9 +9,33 @@
<artifactId>jung-visualization</artifactId>
<name>JUNG - Visualization Support</name>
<description>Core visualization support for the JUNG project</description>
-
+ <properties>
+ <version.commons-logging>1.2</version.commons-logging>
+ <version.log4j>1.2.16</version.log4j>
If we really do want to keep log4j (which I admit I'm sceptical about
currently), please consider using log4j 2.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
BasicVisualizationServer.java
<#126 (comment)>:
> @@ -321,8 +373,16 @@ protected void renderGraph(Graphics2D g2d) {
if (layout instanceof Caching) {
((Caching) layout).clear();
}
-
- renderer.render();
+ log.debug(
+ "render nodes from "
+ + model.getLayoutMediator()
+ + " with nodes "
+ + model.getLayoutMediator().getNetwork().nodes());
Ditto.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
DefaultVisualizationModel.java
<#126 (comment)>:
> // if the layout has NOT been initialized yet, initialize its size
// now to the size of the VisualizationViewer window
if (layoutSize == null) {
- layout.setSize(viewSize);
+ newLayout.setSize(viewSize);
+ // newLayout.setSize(viewSize);
I believe this comment has very little use. Would you please consider
removing it.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
PluggableRenderContext.java
<#126 (comment)>:
> @@ -105,11 +104,10 @@
protected GraphicsDecorator graphicsContext;
private EdgeShape<E> edgeShape;
-
+ //Function<Context<Network,E>, Shape>
Ditto.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
PluggableRenderContext.java
<#126 (comment)>:
> PluggableRenderContext(Network<V, E> graph) {
- this.network = graph;
- this.edgeShape = new EdgeShape<E>(graph);
- this.edgeShapeTransformer = edgeShape.new QuadCurve();
+ // this.edgeShape = new EdgeShape<E>(graph);
Ditto.
Please look back over all the other comments you added and consider
whether they're really needed.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/control/EditingModalGraphMouse.java
<#126 (comment)>:
> @@ -64,9 +62,9 @@ public EditingModalGraphMouse(
float in,
float out) {
super(in, out);
- Preconditions.checkArgument(
- rc.getNetwork() instanceof MutableNetwork,
- "Supplied Network instance must be a MutableNetwork");
+ // Preconditions.checkArgument(
+ // layoutMediator.getNetwork() instanceof MutableNetwork,
+ // "Supplied Network instance must be a MutableNetwork");
Please consider uncommenting these lines.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
VisualizationServer.java
<#126 (comment)>:
>
- /** @return the current graph layout. */
- Layout<V> getGraphLayout();
+ // /** @return the current graph layout. */
+ // Layout<V> getGraphLayout();
Please consider uncommenting or removing these lines.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/
VisualizationServer.java
<#126 (comment)>:
> @@ -68,15 +67,15 @@
/** @return the renderer used by this instance. */
Renderer<V, E> getRenderer();
- /**
- * Replaces the current graph layout with ***@***.*** layout}.
- *
- * @param layout the new layout to set
- */
- void setGraphLayout(Layout<V> layout);
+ // /**
+ // * Replaces the current graph layout with ***@***.*** layout}.
+ // *
+ // * @param layout the new layout to set
+ // */
+ // void setGraphLayout(Layout<V> layout);
Please consider uncommenting or removing these lines.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/decorators/AbstractEdgeShapeTransformer.java
<#126 (comment)>:
> +
+import com.google.common.graph.Network;
+import edu.uci.ics.jung.visualization.util.Context;
+import java.awt.Shape;
+import java.util.function.Function;
+
+/**
+ * An interface for decorators that return a <code>Shape</code> for a specified edge.
+ *
+ * @author Tom Nelson
+ */
+public abstract class AbstractEdgeShapeTransformer<V, E>
+ implements Function<Context<Network<V, E>, E>, Shape> {
+
+ /** Specifies how far apart to place the control points for edges being drawn in parallel. */
+ protected float control_offset_increment = 20.f;
Please make this camelCase.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/
LayoutTransition.java
<#126 (comment)>:
> protected boolean done = false;
protected int count = 20;
protected int counter = 0;
protected VisualizationViewer<V, E> vv;
public LayoutTransition(
- VisualizationViewer<V, E> vv, Layout<V> startLayout, Layout<V> endLayout) {
+ VisualizationViewer<V, E> vv,
+ LayoutMediator<V, E> startLayoutMediator,
+ LayoutMediator<V, E> endLayoutMediator) {
+ if (log.isDebugEnabled()) {
+ log.debug("transition from " + startLayoutMediator + " to " + endLayoutMediator);
+ }
Ditto regarding earlier logging comments.
Please consider whether logging statements are needed at all.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/renderers/GradientVertexRenderer.java
<#126 (comment)>:
> @@ -66,17 +62,18 @@ public GradientVertexRenderer(
this.pickedColorTwo = pickedColorTwo;
this.pickedState = vv.getPickedVertexState();
this.cyclic = cyclic;
- this.layout = vv.getGraphLayout();
- this.renderContext = vv.getRenderContext();
+ // this.layout = vv.getGraphLayout();
+ // this.renderContext = vv.getRenderContext();
Ditto regarding earlier comment on commenting-out code.
For all places where you've done this, please consider whether they really
need to be commented out, and if instead they can be uncommented again or
simply removed.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/spatial/Spatial.java
<#126 (comment)>:
> @@ -0,0 +1,32 @@
+package edu.uci.ics.jung.visualization.spatial;
+
+import com.google.common.collect.Multimap;
+import java.awt.*;
+import java.util.Collection;
+
+/** Created by Tom Nelson on 9/22/17. */
Please remove this comment.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/spatial/Spatial.java
<#126 (comment)>:
> @@ -0,0 +1,32 @@
+package edu.uci.ics.jung.visualization.spatial;
+
+import com.google.common.collect.Multimap;
+import java.awt.*;
+import java.util.Collection;
+
+/** Created by Tom Nelson on 9/22/17. */
+public interface Spatial<N> {
+ /**
+ * A Multimap of box number to Lists of nodes in that box
+ *
+ * @return
This empty @return is not needed. Please remove it.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/spatial/SpatialGrid.java
<#126 (comment)>:
> @@ -0,0 +1,253 @@
+package edu.uci.ics.jung.visualization.spatial;
+
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Multimap;
+import com.google.common.collect.Multimaps;
+import com.google.common.collect.Sets;
+import java.awt.*;
+import java.awt.geom.Point2D;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+
+/** Created by Tom Nelson */
Please remove this comment.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/spatial/SpatialGrid.java
<#126 (comment)>:
> + * @param horizontalCount how many tiles in a row
+ * @param verticalCount how many tiles in a column
+ */
+ public SpatialGrid(Rectangle bounds, int horizontalCount, int verticalCount) {
+ this.size = bounds.getSize();
+ this.visibleArea = bounds;
+ this.horizontalCount = horizontalCount;
+ this.verticalCount = verticalCount;
+ this.boxWidth = size.getWidth() / horizontalCount;
+ this.boxHeight = size.getHeight() / verticalCount;
+ }
+
+ /**
+ * A Multimap of box number to Lists of nodes in that box
+ *
+ * @return
Not clear to me that this is useful. Please remove.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/spatial/SpatialGrid.java
<#126 (comment)>:
> + /**
+ * A Multimap of box number to Lists of nodes in that box
+ *
+ * @return
+ */
+ public Multimap<Integer, N> getMap() {
+ return map;
+ }
+
+ /**
+ * given the box x,y coordinates (not the coordinate system) return the box number (0,0) has box 0
+ * (horizontalCount,horizontalCount) has box horizontalCount*verticalCount - 1
+ *
+ * @param boxX
+ * @param boxY
+ * @return
Please fill these out or remove them.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/spatial/SpatialGrid.java
<#126 (comment)>:
> + * @param boxY
+ * @return
+ */
+ public int getBoxNumber(int boxX, int boxY) {
+ return boxY * this.horizontalCount + boxX;
+ }
+
+ public int getBoxNumber(int[] boxXY) {
+ return getBoxNumber(boxXY[0], boxXY[1]);
+ }
+
+ /**
+ * given a box number, return the x,y coordinates in the grid coordinate system
+ *
+ * @param boxIndex
+ * @return
Please fill these out or remove them
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/
visualization/spatial/SpatialGrid.java
<#126 (comment)>:
> + * @param boxY
+ * @return
+ */
+ public int getBoxNumber(int boxX, int boxY) {
+ return boxY * this.horizontalCount + boxX;
+ }
+
+ public int getBoxNumber(int[] boxXY) {
+ return getBoxNumber(boxXY[0], boxXY[1]);
+ }
+
+ /**
+ * given a box number, return the x,y coordinates in the grid coordinate system
+ *
+ * @param boxIndex
+ * @return
This applies to all methods in this class.
------------------------------
In jung-visualization/src/main/java/edu/uci/ics/jung/visualization/util/
LayoutMediator.java
<#126 (comment)>:
> @@ -0,0 +1,89 @@
+package edu.uci.ics.jung.visualization.util;
+
+import com.google.common.graph.Network;
+import edu.uci.ics.jung.algorithms.layout.Layout;
+import edu.uci.ics.jung.algorithms.util.IterativeContext;
+import java.awt.*;
+import java.awt.geom.Point2D;
+import java.util.Set;
+import java.util.function.Function;
+
+/** Created by Tom Nelson on 9/22/17. */
Please remove this comment and all other "Created by..." comments like
this.
------------------------------
In pom.xml <#126 (comment)>:
> @@ -176,7 +176,7 @@
<execution>
<phase>verify</phase>
<goals>
- <goal>check</goal>
+ <goal>format</goal>
Please do NOT change this. If you want to format code, you can run mvn
fmt:format as described in CONTRIBUTING.md. :)
------------------------------
In pom.xml <#126 (comment)>:
> @@ -176,7 +176,7 @@
<execution>
<phase>verify</phase>
<goals>
- <goal>check</goal>
+ <goal>format</goal>
FYI, the reason to not change this is because Travis CI depends on the mvn
fmt command working this way to check that people have properly formatted
their code before check-in.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#126 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbB59_Qio_fwews5PgvkICYq7K8qJ5ks5sl-PbgaJpZM4Pi6ec>
.
|
Hi @tomnelson. I admit that I did not actually run any of the demos. I'll see if I can find and open up my dev machine to run a couple of them, and see if I notice any logging messages that way.
Okey dokey, SGTM! |
|
@tomnelson Nice to meet you, too! :) |
…ations Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ShowLayouts.java modified: jung-visualization/pom.xml modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/DefaultVisualizationModel.java modified: pom.xml
Changes to be committed: modified: jung-visualization/pom.xml modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/SpatialGrid.java
Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemo.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/SpatialGrid.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/subLayout/GraphCollapser.java new file: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/sublayout/GraphCollapserTest.java
Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemoWithLayouts.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EditingModalGraphMouse.java modified: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/sublayout/GraphCollapserTest.java
Changes to be committed: modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutEvent.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/ObservableCachingLayout.java
Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemoWithLayouts.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/subLayout/GraphCollapser.java
Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemoWithLayouts.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/subLayout/GraphCollapser.java modified: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/sublayout/GraphCollapserTest.java
Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/BalloonLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/EdgeLabelDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/L2RTreeLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/MinimumSpanningTreeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/MultiViewDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/PluggableRendererDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/RadialTreeLensDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/TreeCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/TreeLayoutDemo.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/EdgeShape.java
Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/GraphEditorDemo.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EdgeSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EditingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SimpleEdgeSupport.java
…samples. Clean up. Changes to be committed: modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/AddNodeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/AnimatingAddNodeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/AnnotationsDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/BalloonLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ClusteringDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/DemoLensVertexImageShaperDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/DrawnIconVertexDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/EdgeLabelDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/GraphEditorDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/GraphFromGraphMLDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/GraphZoomScrollPaneDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ImageEdgeLabelDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/InternalFrameSatelliteViewDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/L2RTreeLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/LensDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/LensVertexImageShaperDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/MinimumSpanningTreeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/MultiViewDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/PluggableRendererDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/RadialTreeLensDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/SatelliteViewDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ShortestPathDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/ShowLayouts.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/SubLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/TreeCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/TreeLayoutDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/TwoModelDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/UnicodeLabelDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexCollapseDemoWithLayouts.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexImageShaperDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexLabelAsShapeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VertexLabelPositionDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/VisualizationImageServerDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/WorldMapGraphDemo.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/DefaultVisualizationModel.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/GraphZoomScrollPane.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/PluggableRenderContext.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/RenderContext.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/VisualizationImageServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/VisualizationModel.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/VisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/VisualizationViewer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/annotations/AnnotatingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/annotations/AnnotatingModalGraphMouse.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/annotations/AnnotationControls.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/annotations/AnnotationManager.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/annotations/AnnotationPaintable.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/AbsoluteCrossoverScalingControl.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/AnimatedPickingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/CrossoverScalingControl.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/CubicCurveEdgeEffects.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/DefaultModalGraphMouse.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EdgeEffects.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EdgeSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EditingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EditingModalGraphMouse.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/EditingPopupGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/LabelEditingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/LayoutScalingControl.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/LensMagnificationGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/LensTranslatingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/ModalLensGraphMouse.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/MouseListenerTranslator.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/PickingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/RotatingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SatelliteAnimatedPickingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SatelliteRotatingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SatelliteScalingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SatelliteShearingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SatelliteTranslatingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SatelliteVisualizationViewer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/ScalingControl.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/ScalingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/ShearingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SimpleEdgeSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/SimpleVertexSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/TranslatingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/VertexSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/ViewScalingControl.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/control/ViewTranslatingGraphMousePlugin.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/AbstractEdgeShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/AbstractVertexShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/EdgeShape.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/EllipseVertexShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/GradientEdgePaintTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/NumberFormattingTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/ParallelEdgeShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/PickableEdgePaintTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/PickableVertexIconTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/PickableVertexPaintTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/SettableVertexShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/decorators/VertexIconShapeTransformer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/BoundingRectangleCollector.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/BoundingRectanglePaintable.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/LayoutTransition.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/AbstractPickedState.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/ClosestShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/LayoutLensShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/MultiPickedState.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/PickedInfo.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/PickedState.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/ShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/picking/ViewLensShapePickSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicEdgeArrowRenderingSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicEdgeLabelRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicEdgeRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicVertexLabelRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/BasicVertexRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/CachingEdgeRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/CachingRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/CachingVertexRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/CenterEdgeArrowRenderingSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/EdgeArrowRenderingSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/GradientVertexRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/Renderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/ReshapingEdgeRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/renderers/VertexLabelAsShapeRenderer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/Spatial.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/SpatialGrid.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/transform/AbstractLensSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/transform/LayoutLensSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/transform/shape/MagnifyImageLensSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/transform/shape/ViewLensSupport.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/util/LayoutMediator.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/util/VertexShapeFactory.java modified: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/BasicVisualizationServerTest.java new file: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/TestMST.java modified: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/control/TestCrossoverScalingControl.java
|
Sorry I haven't yet taken a look at this set of changes; I'll try to do that this weekend. |
|
Unformatted code will make the build fail if you leave it set to 'check'. I
got so tired of git adding and removing the pom or running a separate mvn
format every time i wanted to build, so I thought I would ask what your
real goal is (hopefully not just making contributors add a google code
format plugin to their IDE)
…On Fri, Sep 29, 2017 at 12:17 PM, jrtom ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pom.xml <#126 (comment)>:
> @@ -176,7 +176,7 @@
<execution>
<phase>verify</phase>
<goals>
- <goal>check</goal>
+ <goal>format</goal>
In theory, no one should be checking in code when the build failed,
because we have the Travis check to forestall that (at least for the
common.graph and master branches).
I haven't formed a strong opinion on this issue yet, but I think that the
point of leaving it as a check is to not require formatting on every build;
I can imagine that there might be circumstances in which you might want to
leave the formatting until you were done developing.
But maybe there's something I'm missing; @jbduncan
<https://github.com/jbduncan>, can you elaborate?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#126 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbB2EXCK6HDWfUweZ35NGKyliXLhYUks5snRgugaJpZM4Pi6ec>
.
|
|
I wondering if a better path might be separate projects for visualization
and (visualization)samples.
…On Fri, Sep 29, 2017 at 12:18 PM, jrtom ***@***.***> wrote:
Sorry I haven't yet taken a look at this set of changes; I'll try to do
that this weekend.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#126 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbB3WACXaHZI7Y5HBhjoGa3mCck1wsks5snRhQgaJpZM4Pi6ec>
.
|
|
I would make a default maven profile (that we use) that formats the code,
and a different profile that CI invokes to just do a check of the formatting
…On Fri, Sep 29, 2017 at 12:37 PM, Jonathan Bluett-Duncan < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pom.xml <#126 (comment)>:
> @@ -176,7 +176,7 @@
<execution>
<phase>verify</phase>
<goals>
- <goal>check</goal>
+ <goal>format</goal>
@jrtom <https://github.com/jrtom> I admit I'm not too clear either on why
checking is preferred to formatting at CI-time. I think it's a combination
of reasons.
I can think of two reasons ATM. Firstly, it's partially to do with how
formatters (google-java-format included) can sometimes contain bugs which
can cause code to be formatted weirdly, or to flat-out fail with an error
or exception. So, if formatting happened during CI-time, the CI may fail or
format things strangely for reasons not to do with the checked-in code,
which has the potential to make things confusing.
Secondly, formatters are prone to occasionally formatting code
incompletely, thus requiring formatters to be run twice or so, so
formatting by CI wouldn't be hermetic, which again is a cause for
confusion. I actually experienced this "problem" (if you'd even call it
that) with google-java-format when I was working on introducing it to JUNG
a good few weeks/months ago; it required me to run mvn fmt:format twice
before certain source files were formatted completely, strangely.
I hope @nedtwigg <https://github.com/nedtwigg>, who I collaborate with on
Spotless <https://github.com/diffplug/spotless>, can elaborate further on
this, because I'm sure an issue was opened in the past on Spotless's issue
tracker asking why Spotless's spotlessCheck task (similar to mvn
fmt:format for us) is the default over spotlessApply (similar to mvn
fmt:check), but I can't find that issue yet.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#126 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbB78FuTdbggv5SrcVe1nWGTFEUPiPks5snRzQgaJpZM4Pi6ec>
.
|
|
I will add the profile, if there is agreement.
Whoever is admin on the CI server would have to add the profile to the
build that runs there.
…On Fri, Sep 29, 2017 at 12:48 PM, Jonathan Bluett-Duncan < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pom.xml <#126 (comment)>:
> @@ -176,7 +176,7 @@
<execution>
<phase>verify</phase>
<goals>
- <goal>check</goal>
+ <goal>format</goal>
Replying to #126 (comment)
<#126 (comment)>:
@tomnelson <https://github.com/tomnelson> That sounds good to me, having
a default maven profile for formatting code and a CI-specific one for
checking code.
Would you be happy to create a new PR for that?
If not, I may have the time and energy in the foreseeable future to
investigate Maven profiles and create them myself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#126 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbBxrdi-Rn5XbJg4y_VpP2sYCp76Llks5snR9hgaJpZM4Pi6ec>
.
|
|
No wonder you guys are so busy! Every time you build, you have to do it
twice! ;)
…On Fri, Sep 29, 2017 at 12:55 PM, jrtom ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pom.xml <#126 (comment)>:
> @@ -176,7 +176,7 @@
<execution>
<phase>verify</phase>
<goals>
- <goal>check</goal>
+ <goal>format</goal>
We definitely wouldn't want the CI check to be formatting code itself; I
don't want an automated CI check to be trying to modify the code (I don't
even know what that would do, but I feel no strong need to find out :) ).
So I would say that until we have multiple profiles that we should keep it
at "check".
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#126 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGbB6TetyhR1CHd_-zlyAKNdByQQwiLks5snSD2gaJpZM4Pi6ec>
.
|
|
I would discourage you from having the formatter fix changes in CI. Here's why: |
…nges in samples. Clean up." This reverts commit 5f351a0.
Changes to be committed: modified: jung-algorithms/src/main/java/edu/uci/ics/jung/algorithms/shortestpath/MinimumSpanningTree.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/MinimumSpanningTreeDemo.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/SimpleGraphDraw.java new file: jung-samples/src/main/java/edu/uci/ics/jung/samples/SimpleGraphSpatialTest.java new file: jung-samples/src/main/java/edu/uci/ics/jung/samples/util/ControlHelpers.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/SpatialGrid.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes to be committed: