This is an automated email from the ASF dual-hosted git repository. desruisseaux pushed a commit to branch geoapi-4.0 in repository https://gitbox.apache.org/repos/asf/sis.git
commit bac4f078546d4fa22f250ff4a199738fbb0ef3e6 Author: Martin Desruisseaux <[email protected]> AuthorDate: Mon Jan 27 16:47:56 2020 +0100 In case of failure to fetch tiles, show error controls for every failed tiles instead than only the "widest" one. Actually it makes code simpler. --- .../org/apache/sis/gui/coverage/GridError.java | 102 ++++++------- .../java/org/apache/sis/gui/coverage/GridTile.java | 163 +++++++-------------- .../coverage/GridTileCache.java} | 29 ++-- .../java/org/apache/sis/gui/coverage/GridView.java | 5 +- .../org/apache/sis/gui/coverage/GridViewSkin.java | 95 ++++++------ 5 files changed, 160 insertions(+), 234 deletions(-) diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridError.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridError.java index 335c2ce..294cd20 100644 --- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridError.java +++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridError.java @@ -18,6 +18,7 @@ package org.apache.sis.gui.coverage; import java.awt.Rectangle; import javafx.geometry.Pos; +import javafx.geometry.Insets; import javafx.event.ActionEvent; import javafx.scene.control.Button; import javafx.scene.control.Label; @@ -31,6 +32,9 @@ import org.apache.sis.util.resources.Vocabulary; /** * Controls to put in the middle of a tile if an error occurred while loading that tile. + * This class contains the reason why a tile request failed, together with some information + * that depends on the viewing context. In particular {@link #getVisibleRegion(Rectangle)} + * needs to be recomputed every time that the visible area in the {@link GridView} changed. * * @author Martin Desruisseaux (Geomatys) * @version 1.1 @@ -39,103 +43,87 @@ import org.apache.sis.util.resources.Vocabulary; */ final class GridError extends VBox { /** - * The area where to write the error message. + * The tile in error. */ - private final Label message; + private final GridTile tile; /** - * If we failed to fetch a tile, the tile in error. If more than one tile has an error, - * then the tile having the largest intersection with the view area. This visible error - * may change during scrolling. + * The reason for the failure to load a tile. */ - private GridTile.Error visibleError; + private final Throwable exception; /** - * The last error reported. This is usually equal to {@link #visibleError}, except that - * the visible error may be {@code null} while {@code lastError} is never reset to null. + * The first line of {@link #message}, also used as header text in the "details" dialog box. */ - private GridTile.Error lastError; + private final String header; /** - * The zero-based row and columns indices of the area currently shown in {@link GridView}. - * This is updated by {@link GridViewSkin#layoutChildren(double, double, double, double)}. + * The area where to write the error message. The text said "can not fetch tile (x, y)" + * with tile indices, followed by the exception message (if any) on next line. */ - private Rectangle viewArea; + private final Label message; /** - * Incremented every time that a new layout is performed. This is used for detecting if a - * {@link GridTile.Error} instance should recompute its visible area. It is not a problem - * if this value overflows; we just check if values differ, not which one is greater. - * - * @see GridTile.Error#updateCount + * The zero-based row and column indices of the tile. + * This is computed by {@link GridView#getTileBounds(int, int)} and should be constant. */ - private int updateCount; + private final Rectangle region; /** - * Creates a new error control. + * Creates a new error control for the specified exception. */ - GridError() { + GridError(final GridView view, final GridTile tile, final Throwable exception) { super(9); - message = new Label(); - message.setTextFill(Color.RED); + this.tile = tile; + this.exception = exception; + this.region = view.getTileBounds(tile.tileX, tile.tileY); + this.header = Resources.format(Resources.Keys.CanNotFetchTile_2, tile.tileX, tile.tileY); + final Button details = new Button(Vocabulary.format(Vocabulary.Keys.Details)); final Button retry = new Button(Vocabulary.format(Vocabulary.Keys.Retry)); final TilePane buttons = new TilePane(12, 0, details, retry); - message.setLabelFor(buttons); buttons.setPrefRows(1); buttons.setPrefColumns(2); buttons.setAlignment(Pos.CENTER); details.setMaxWidth(100); // Arbitrary limit, width enough for allowing TilePane to resize. retry .setMaxWidth(100); + + final String t = exception.getLocalizedMessage(); + message = new Label((t == null) ? header : header + System.lineSeparator() + t); + message.setTextFill(Color.RED); + message.setLabelFor(buttons); + getChildren().addAll(message, buttons); setAlignment(Pos.CENTER); + setPadding(new Insets(12, 18, 24, 18)); details.setOnAction(this::showDetails); + retry .setOnAction(this::retry); } /** - * Invoked by {@link GridViewSkin#layoutChildren(double, double, double, double)} when a new layout is beginning. + * Returns the bounds of the error controls which is currently visible in the {@link GridView}. + * This is the intersection of {@link #region} with the area currently shown in the grid view. + * May vary during scrolling and is empty if the tile in error is outside the visible area. * - * @param area zero-based row and columns indices of the area currently shown in {@link GridView}. + * @param viewArea zero-based row and columns indices of the area currently shown in {@link GridView}. */ - final void initialize(final Rectangle area) { - viewArea = area; - visibleError = null; - if (++updateCount == 0) { - updateCount = 1; // Paranoiac safety in case we did a cycle over all integer values. - } + final Rectangle getVisibleRegion(final Rectangle viewArea) { + return viewArea.intersection(region); } /** - * Updates this error control with the given status. If this control is already showing an error message, - * it will be updated only if the given status cover a larger view area. If this control has been updated, - * then this method returns the zero-based row and column indices of the region in error in the view area. - * Otherwise (if this method did nothing), this method returns {@code null}. - * - * @param status the candidate error status. - * @return new indices of visible area, or {@code null} if no change. - * This is a direct reference to internal field; do not modify. + * Invoked when the user click on the "details" button. */ - final Rectangle update(final GridTile.Error status) { - if (status != visibleError && status.updateAndCompare(updateCount, viewArea, visibleError)) { - visibleError = lastError = status; - String text = status.message; - final Throwable exception = status.exception; - String more = exception.getLocalizedMessage(); - if (more != null) { - text = text + System.lineSeparator() + more; - } - message.setText(text); - return status.visibleArea; - } - return null; + private void showDetails(final ActionEvent event) { + ExceptionReporter.show(Resources.format(Resources.Keys.ErrorDataAccess), header, exception); } /** - * Invoked when the user click on the "details" button. + * Invoked when the user asked to retry a tile computation. */ - private void showDetails(final ActionEvent event) { - if (lastError != null) { - ExceptionReporter.show(Resources.format(Resources.Keys.ErrorDataAccess), lastError.message, lastError.exception); - } + private void retry(final ActionEvent event) { + final GridView view = (GridView) getParent(); + ((GridViewSkin) view.getSkin()).removeError(this); + tile.clear(); } } diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTile.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTile.java index a1fa654..2780296 100644 --- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTile.java +++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTile.java @@ -16,16 +16,15 @@ */ package org.apache.sis.gui.coverage; -import java.awt.Rectangle; import java.awt.image.Raster; import java.awt.image.RenderedImage; import javafx.concurrent.Task; import org.apache.sis.internal.gui.BackgroundThreads; -import org.apache.sis.internal.gui.Resources; /** - * A {@link Raster} for a {@link RenderedImage} file, potentially to be loaded in a background thread. + * A {@link Raster} for a {@link RenderedImage} tile, + * potentially fetched (loaded or computed) in a background thread. * * @author Martin Desruisseaux (Geomatys) * @version 1.1 @@ -46,18 +45,22 @@ final class GridTile { private final int hashCode; /** - * The tile, or {@code null} if not yet loaded. + * The tile, or {@code null} if not yet fetched. * * @see #tile() */ private Raster tile; /** - * Non-null if a loading is in progress or if an error occurred while fetching the tile. - * The loading status is used for avoiding to create many threads requesting the same tile. - * If loading is in progress, requests for that tile will return {@code null} immediately. + * Non-null if an error occurred while fetching the tile. */ - private Error status; + private GridError error; + + /** + * Whether a fetching is under progress. Used for avoiding to create many threads requesting the same + * tile in same time: if {@code true} then {@link #load(GridView)} returns {@code null} immediately. + */ + private boolean loading; /** * Creates a new tile for the given tile coordinates. @@ -70,7 +73,7 @@ final class GridTile { /** * Returns a hash code value for this tile. This hash code value must be based only on tile indices; - * the {@link #tile} and the {@link #status} must be ignored, because we will use {@link GridTile} + * the {@link #tile} and the {@link #error} must be ignored, because we will use {@link GridTile} * instances also as keys for locating tiles in a hash map. */ @Override @@ -109,15 +112,42 @@ final class GridTile { } /** - * Loads tile from the given image in a background thread and informs the specified view - * when the tile become available. If we already failed to load that tile in a previous - * attempt, then this method may set the {@link GridViewSkin#error} field. + * Clears the cached {@link #tile}. That tile will be recomputed again if needed. + * Note that in many cases the tile will not be really recomputed since a second, + * more sophisticated caching mechanism may be done (at least in Apache SIS case) + * by {@link RenderedImage#getTile(int, int)} implementation. + * + * @return {@code true} if there is no error, in which case the whole {@link GridTile} can be discarded. + */ + final boolean clearTile() { + if (loading) { + return false; + } + tile = null; + return error == null; + } + + /** + * Clears the tile, error and loading flags. This is invoked after a the background thread fetching a + * tile completed, either successfully or with a failure, before to set the new values. This is also + * invoked if we want to retry loading a tile after a failure. + */ + final void clear() { + tile = null; + error = null; + loading = false; + } + + /** + * Fetches (load or compute) tile from the image in a background thread and informs the specified view + * when the tile become available. If we already failed to fetch that tile in a previous attempt, then + * this method returns {@code null}. * - * @param view the view for which to load a tile. + * @param view the view for which to fetch a tile. */ final void load(final GridView view) { - if (status == null) { - status = Error.LOADING; // Pseudo-error. + if (!loading && error == null) { + loading = true; final RenderedImage image = view.getImage(); BackgroundThreads.execute(new Task<Raster>() { /** Invoked in background thread for fetching the tile. */ @@ -132,8 +162,7 @@ final class GridTile { */ @Override protected void succeeded() { super.succeeded(); - tile = null; - status = null; + clear(); if (view.getImage() == image) { tile = getValue(); view.contentChanged(false); @@ -146,12 +175,10 @@ final class GridTile { */ @Override protected void failed() { super.failed(); - tile = null; - status = null; + clear(); if (view.getImage() == image) { - status = new Error(Resources.format(Resources.Keys.CanNotFetchTile_2, tileX, tileY), - view.getTileBounds(tileX, tileY), getException()); - view.contentChanged(false); // For rendering the error message. + error = new GridError(view, GridTile.this, getException()); + ((GridViewSkin) view.getSkin()).errorOccurred(error); } } @@ -162,99 +189,9 @@ final class GridTile { */ @Override protected void cancelled() { super.cancelled(); - tile = null; - status = null; + clear(); } }); - } else if (status != Error.LOADING) { - /* - * A previous attempt failed to load that tile. We may have an error message to report. - * If more than one tile failed, take the one with largest visible area. - */ - ((GridViewSkin) view.getSkin()).errorOccurred(status); - } - } - - /** - * The status of a tile request, either {@link #LOADING} or any other instance in case of error. - * If not {@link #LOADING}, this class contains the reason why a tile request failed, together - * with some information that depends on the viewing context. In particular {@link #visibleArea} - * needs to be recomputed every time the viewed area in the {@link GridView} changed. - */ - static final class Error { - /** - * A pseudo-error for saying that the tile is being fetched. Its effect is similar to an error - * in the sense that {@link #load(GridView)} returns {@code null} immediately, except that no - * error message is recorded. - */ - private static final Error LOADING = new Error(null, null, null); - - /** - * The error message saying "can not fetch tile (x, y)", with tile indices. - */ - final String message; - - /** - * If we failed to load the tile, the reason for the failure. - */ - final Throwable exception; - - /** - * If we failed to load the tile, the zero-based row and column indices of the tile. - * This is computed by {@link GridView#getTileBounds(int, int)} and should be constant. - */ - private final Rectangle region; - - /** - * Intersection of {@link #region} with the area currently shown in the view. - * May vary with scrolling and is empty if the tile in error is outside visible area. - */ - Rectangle visibleArea; - - /** - * The {@link GridError#updateCount} value last time that {@link #visibleArea} was computed. - * Used for detecting if we should recompute the visible area. - */ - private int updateCount; - - /** - * Creates an error status with the given cause. - */ - private Error(final String message, final Rectangle region, final Throwable exception) { - this.message = message; - this.region = region; - this.exception = exception; - } - - /** - * Recomputes the {@link #visibleArea} value if needed, then returns {@code true} - * if the visible area in this {@code Error} if wider than the one in {@code other}. - * - * @param stamp value of {@link GridError#updateCount}. - * @param viewArea value of {@link GridError#viewArea}. - * @param other the previous error, or {@code null}. - * @return whether this status should replace {@code other}. - */ - final boolean updateAndCompare(final int stamp, final Rectangle viewArea, final Error other) { - if (updateCount != stamp) { - visibleArea = viewArea.intersection(region); - updateCount = stamp; - } - if (other == null || other.visibleArea.isEmpty()) { - return true; - } - if (visibleArea.isEmpty()) { - return false; - } - /* - * Gives precedence to width instead than computing the area because the error - * messsage will be written horizontally, so we want more space for writing it. - */ - int c = Integer.compare(visibleArea.width, other.visibleArea.width); - if (c == 0) { - c = Integer.compare(visibleArea.height, other.visibleArea.height); - } - return c > 0; } } } diff --git a/application/sis-javafx/src/main/java/org/apache/sis/internal/gui/BoundedHashMap.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTileCache.java similarity index 63% rename from application/sis-javafx/src/main/java/org/apache/sis/internal/gui/BoundedHashMap.java rename to application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTileCache.java index e41aae6..1afccc4 100644 --- a/application/sis-javafx/src/main/java/org/apache/sis/internal/gui/BoundedHashMap.java +++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTileCache.java @@ -14,14 +14,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.sis.internal.gui; +package org.apache.sis.gui.coverage; import java.util.Map; import java.util.LinkedHashMap; +import org.apache.sis.internal.coverage.j2d.ImageUtilities; /** - * A map with a fixed capacity. When the maximal capacity is exceeded, eldest entries are removed. + * A map of tiles with a fixed capacity. When the maximal capacity is exceeded, eldest entries are cleaned. * This is a trivial implementation on top of {@link LinkedHashMap} used only for very simple caching. * * @author Martin Desruisseaux (Geomatys) @@ -30,29 +31,25 @@ import java.util.LinkedHashMap; * @module */ @SuppressWarnings("serial") -public final class BoundedHashMap<K,V> extends LinkedHashMap<K,V> { +final class GridTileCache extends LinkedHashMap<GridTile,GridTile> { /** - * The maximal capacity. + * Creates a new cache of tiles. */ - private final int capacity; - - /** - * Creates a new map with the given maximal capacity. - * - * @param capacity the maximal capacity. - */ - public BoundedHashMap(final int capacity) { - this.capacity = capacity; + GridTileCache() { } /** - * Removes the given entry if this map has reached its maximal capacity. + * Clears the latest tile if this map has reached its maximal capacity. + * In the common case where there is no error, the whole entry will be discarded. * * @param entry the eldest entry. * @return whether to remove the entry. */ @Override - protected boolean removeEldestEntry(final Map.Entry<K,V> entry) { - return size() > capacity; + protected boolean removeEldestEntry(final Map.Entry<GridTile,GridTile> entry) { + if (size() > ImageUtilities.SUGGESTED_TILE_CACHE_SIZE) { + return entry.getValue().clearTile(); + } + return false; } } diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridView.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridView.java index 550bb17..1a44ed4 100644 --- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridView.java +++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridView.java @@ -16,7 +16,6 @@ */ package org.apache.sis.gui.coverage; -import java.util.Map; import java.text.NumberFormat; import java.text.FieldPosition; import java.awt.Rectangle; @@ -38,8 +37,6 @@ import javafx.scene.paint.Color; import javafx.scene.paint.Paint; import org.apache.sis.util.ArgumentChecks; import org.apache.sis.coverage.grid.GridCoverage; -import org.apache.sis.internal.coverage.j2d.ImageUtilities; -import org.apache.sis.internal.gui.BoundedHashMap; /** @@ -104,7 +101,7 @@ public class GridView extends Control { * since "real" caching is done by {@link org.apache.sis.image.ComputedImage}. The purpose of this cache is * to remember that a tile is immediately available and that we do not need to start a background thread. */ - private final Map<GridTile,GridTile> tiles = new BoundedHashMap<>(ImageUtilities.SUGGESTED_TILE_CACHE_SIZE); + private final GridTileCache tiles = new GridTileCache(); /** * The most recently used tile. Cached separately because it will be the desired tile in the vast majority diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridViewSkin.java b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridViewSkin.java index 53d65bb..9addfee 100644 --- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridViewSkin.java +++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridViewSkin.java @@ -110,10 +110,9 @@ final class GridViewSkin extends VirtualContainerBase<GridView, GridRow> { double cellInnerWidth; /** - * The controls to show in case of error, or {@code null} if none. This is created and - * added to the children list the first time that an error occurs while reading a tile. + * Whether the grid view contains at least one tile that we failed to fetch. */ - private GridError error; + private boolean hasErrors; /** * Creates a new skin for the specified view. @@ -171,39 +170,24 @@ final class GridViewSkin extends VirtualContainerBase<GridView, GridRow> { } /** - * Invoked when an error occurred when fetching a tile. If this is the first time that an error happens - * for this image, then a {@link GridError} node will be added as the last child (i.e. will be drawn on - * top of everything else). That child will be removed if a new image is set. + * Invoked when an error occurred while fetching a tile. The given {@link GridError} node is added as last + * child (i.e. will be drawn on top of everything else). That child will be removed if a new image is set. */ - final void errorOccurred(final GridTile.Error status) { - if (error == null) { - error = new GridError(); - getChildren().add(error); - computeErrorBounds((Flow) getVirtualFlow()); - } - final java.awt.Rectangle area = error.update(status); - if (area != null) { - final Flow flow = (Flow) getVirtualFlow(); - final GridRow firstVisibleRow = area.isEmpty() ? null : flow.getFirstVisibleCell(); - error.setVisible(firstVisibleRow != null); - if (firstVisibleRow != null) { - final double cellHeight = flow.getFixedCellSize(); - final double width = area.width * cellWidth; - final double height = area.height * cellHeight; - layoutInArea(error, - cellWidth * (area.x - firstVisibleColumn) + leftBackground.getWidth(), - cellHeight * (area.y - firstVisibleRow.getIndex()) + topBackground.getHeight(), - width, height, Node.BASELINE_OFFSET_SAME_AS_HEIGHT, HPos.CENTER, VPos.CENTER); - /* - * If after layout the error message size appears too large for the remaining space, hide it. - * The intent is to avoid having the message and buttons on top of numbers in valid tiles. - * It does not seem to work fully however (some overlaps can still happen). - */ - if (error.getHeight() > height || error.getWidth() > width) { - error.setVisible(false); - } - } - } + final void errorOccurred(final GridError error) { + hasErrors = true; + getChildren().add(error); + } + + /** + * Removes the given error. This method is invoked when the user wants to try again to fetch a tile. + * Callers is responsible for invoking {@link GridTile#clear()}. + */ + final void removeError(final GridError error) { + final ObservableList<Node> children = getChildren(); + children.remove(error); + // The list should never be empty, so IndexOutOfBoundsException here would be a bug. + hasErrors = children.get(children.size() - 1) instanceof GridError; + contentChanged(false); } /** @@ -219,10 +203,8 @@ final class GridViewSkin extends VirtualContainerBase<GridView, GridRow> { final void contentChanged(final boolean all) { if (all) { updateItemCount(); - if (error != null) { - error = null; - getChildren().removeIf((node) -> (node instanceof GridError)); - } + getChildren().removeIf((node) -> (node instanceof GridError)); + hasErrors = false; } /* * Following call may be redundant with `updateItemCount()` except if the number of @@ -413,7 +395,7 @@ final class GridViewSkin extends VirtualContainerBase<GridView, GridRow> { pos += cellWidth; } } - if (error != null) { + if (hasErrors) { computeErrorBounds(flow); } } @@ -425,13 +407,38 @@ final class GridViewSkin extends VirtualContainerBase<GridView, GridRow> { */ private void computeErrorBounds(final Flow flow) { final java.awt.Rectangle viewArea = new java.awt.Rectangle(); - final GridRow firstVisibleRow = flow.getFirstVisibleCell(); - if (firstVisibleRow != null) { + final GridRow first = flow.getFirstVisibleCell(); + int firstVisibleRow = 0; + if (first != null) { viewArea.x = firstVisibleColumn; - viewArea.y = firstVisibleRow.getIndex(); + viewArea.y = firstVisibleRow = first.getIndex(); viewArea.width = (int) ((flow.getWidth() - leftBackground.getWidth()) / cellWidth); viewArea.height = (int) (flow.getVisibleHeight() / flow.getFixedCellSize()); } - error.initialize(viewArea); + final ObservableList<Node> children = getChildren(); + for (int i=children.size(); --i >= 0;) { + final Node node = children.get(i); + if (!(node instanceof GridError)) break; + final GridError error = (GridError) node; + boolean visible = false; + final java.awt.Rectangle area = error.getVisibleRegion(viewArea); + if (!area.isEmpty()) { + final double cellHeight = flow.getFixedCellSize(); + final double width = area.width * cellWidth; + final double height = area.height * cellHeight; + layoutInArea(error, + cellWidth * (area.x - firstVisibleColumn) + leftBackground.getWidth(), + cellHeight * (area.y - firstVisibleRow) + topBackground.getHeight(), + width, height, Node.BASELINE_OFFSET_SAME_AS_HEIGHT, HPos.CENTER, VPos.CENTER); + /* + * If after layout the error message size appears too large for the remaining space, hide it. + * The intent is to avoid having the message and buttons on top of cell values in valid tiles. + * It does not seem to work fully however (some overlaps can still happen), so we added some + * inset to mitigate the problem and also for more aerated presentation. + */ + visible = error.getHeight() <= height && error.getWidth() <= width; + } + error.setVisible(visible); + } } }
