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
The following commit(s) were added to refs/heads/geoapi-4.0 by this push:
new de47cb2 Simplify FeatureLoader by replacing the `Initial` inner class
by an `initializer` field. Fix a resource leak: Stream was not closed if the
`estimatedSize` was actually exact.
de47cb2 is described below
commit de47cb21834a3c0522e1b899c5eee3a2fdbe124c
Author: Martin Desruisseaux <[email protected]>
AuthorDate: Wed Nov 6 20:03:22 2019 +0100
Simplify FeatureLoader by replacing the `Initial` inner class by an
`initializer` field.
Fix a resource leak: Stream was not closed if the `estimatedSize` was
actually exact.
---
.../org/apache/sis/gui/dataset/FeatureList.java | 99 +++++++++++--
.../org/apache/sis/gui/dataset/FeatureLoader.java | 164 ++++++++++-----------
.../org/apache/sis/gui/dataset/FeatureTable.java | 3 +-
3 files changed, 165 insertions(+), 101 deletions(-)
diff --git
a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
index 2c384f3..9d4144e 100644
---
a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
+++
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
@@ -25,6 +25,7 @@ import javafx.collections.ObservableListBase;
import org.opengis.feature.Feature;
import org.apache.sis.storage.FeatureSet;
import org.apache.sis.internal.gui.BackgroundThreads;
+import org.apache.sis.internal.util.UnmodifiableArrayList;
import org.apache.sis.util.ArraysExt;
@@ -33,11 +34,19 @@ import org.apache.sis.util.ArraysExt;
* When an element is requested, if that element has not yet been read, the
reading is done
* in a background thread.
*
+ * <p>This list is unmodifiable through public API. It can be modified only
through package-private API.
+ * This restriction exists for preventing uncontrolled modifications to
introduce inconsistencies with
+ * the modifications to be applied by the loader running in background
thread.</p>
+ *
* <p>This list does not accept null elements; any attempt to add a null
feature is silently ignored.
* The null value is reserved for meaning that the element is in process of
being loaded.</p>
*
* <p>All methods in this class shall be invoked from JavaFX thread.</p>
*
+ * @todo Current implementation does not release previously loaded features.
+ * We could do that in a previous version if memory usage is a problem,
+ * provided that {@link Spliterator#ORDERED} is set.
+ *
* @author Martin Desruisseaux (Geomatys)
* @version 1.1
* @since 1.1
@@ -45,6 +54,23 @@ import org.apache.sis.util.ArraysExt;
*/
final class FeatureList extends ObservableListBase<Feature> {
/**
+ * Number of empty rows to show in the bottom of the table when we don't
know how many rows still
+ * need to be read. Those rows do not stay empty for long since they will
become valid as soon as
+ * the background loader finished to load a page ({@value
FeatureLoader#PAGE_SIZE} rows).
+ */
+ private static final long NUM_PENDING_ROWS = 10;
+
+ /**
+ * Maximum number of rows that this list will allow.
+ */
+ private static final int MAXIMUM_ROWS = Integer.MAX_VALUE;
+
+ /**
+ * The {@link #elements} value when this list is empty.
+ */
+ private static final Feature[] EMPTY = new Feature[0];
+
+ /**
* The elements in this list, never {@code null}.
*/
private Feature[] elements;
@@ -81,7 +107,28 @@ final class FeatureList extends ObservableListBase<Feature>
{
* Creates a new list of features.
*/
FeatureList() {
- elements = new Feature[0];
+ elements = EMPTY;
+ }
+
+ /**
+ * Returns the currently valid elements.
+ */
+ private List<Feature> validElements() {
+ return UnmodifiableArrayList.wrap(elements, 0, validCount);
+ }
+
+ /**
+ * Clears the content of this list. This method shall be invoked when no
loader is running
+ * in a background thread, or when the loaded decided itself to invoke
this method.
+ */
+ final void clearUnsafe() {
+ final List<Feature> removed = validElements();
+ elements = EMPTY;
+ estimatedSize = 0;
+ validCount = 0;
+ beginChange();
+ nextReplace(0, 0, removed);
+ endChange();
}
/**
@@ -101,10 +148,11 @@ final class FeatureList extends
ObservableListBase<Feature> {
previous.cancel();
}
if (features != null) {
- nextPageLoader = new FeatureLoader.Initial(table, features);
+ nextPageLoader = new FeatureLoader(table, features);
BackgroundThreads.execute(nextPageLoader);
return true;
} else {
+ clearUnsafe();
return false;
}
}
@@ -112,35 +160,43 @@ final class FeatureList extends
ObservableListBase<Feature> {
/**
* Invoked by {@link FeatureLoader} for replacing the current content by a
new list of features.
* The list size after this method invocation will be {@code
expectedSize}, not {@code count}.
- * The missing elements will be implicitly null until {@link
#addFeatures(Feature[], int)} is invoked.
- * If the expected size is unknown (i.e. its value is {@link
Long#MAX_VALUE}),
+ * The missing elements will be implicitly null until {@link
#addFeatures(Feature[], int, boolean)}
+ * is invoked. If the expected size is unknown (i.e. its value is {@link
Long#MAX_VALUE}),
* then an arbitrary size is computed from {@code count}.
*
* @param remainingCount value of {@link Spliterator#estimateSize()}
after partial traversal.
* @param characteristics value of {@link Spliterator#characteristics()}.
* @param features new features. This array is not cloned and may
be modified in-place.
* @param count number of valid elements in the given array.
+ * @param hasMore if the stream may have more features.
*/
@SuppressWarnings("AssignmentToCollectionOrArrayFieldFromParameter")
- final void setFeatures(long remainingCount, int characteristics, final
Feature[] features, final int count) {
+ final void setFeatures(long remainingCount, int characteristics,
+ final Feature[] features, final int count, final
boolean hasMore)
+ {
assert Platform.isFxApplicationThread();
int newValidCount = 0;
for (int i=0; i<count; i++) {
final Feature f = features[i];
if (f != null) features[newValidCount++] = f; // Exclude
null elements.
}
- final List<Feature> removed = Arrays.asList(elements); // Want this
call outside {beginChange … endChange}.
+ final List<Feature> removed = validElements(); // Want this
call outside {beginChange … endChange}.
if (remainingCount == Long.MAX_VALUE) {
- remainingCount = count + 10L; // Arbitrary
additional amount.
+ remainingCount = count + NUM_PENDING_ROWS; // Arbitrary
additional amount.
characteristics = 0;
}
- estimatedSize = (int) Math.min(Integer.MAX_VALUE,
Math.addExact(remainingCount, newValidCount));
+ estimatedSize = (int) Math.min(MAXIMUM_ROWS,
Math.addExact(remainingCount, newValidCount));
isSizeExact = (characteristics & Spliterator.SIZED) != 0;
elements = features;
validCount = newValidCount;
+ if (hasMore && estimatedSize <= newValidCount) {
+ // Estimated size seems incorrect. Add some empty rows for
triggering a new page load.
+ estimatedSize = (int) Math.min(MAXIMUM_ROWS, newValidCount +
NUM_PENDING_ROWS);
+ }
beginChange();
nextReplace(0, estimatedSize, removed);
endChange();
+ checkOverflow();
}
/**
@@ -149,9 +205,10 @@ final class FeatureList extends
ObservableListBase<Feature> {
*
* @param features the features to add. Null elements are ignored.
* @param count number of valid elements in the given array.
+ * @param hasMore if the stream may have more features.
* @throws ArithmeticException if the number of elements exceeds this list
capacity.
*/
- final void addFeatures(final Feature[] features, final int count) {
+ final void addFeatures(final Feature[] features, final int count, final
boolean hasMore) {
assert Platform.isFxApplicationThread();
if (count > 0) {
int newValidCount = Math.addExact(validCount, count);
@@ -170,13 +227,29 @@ final class FeatureList extends
ObservableListBase<Feature> {
*/
final int replaceTo = Math.min(newValidCount, estimatedSize);
final List<Feature> removed = Collections.nCopies(replaceTo -
validCount, null);
- if (newValidCount > estimatedSize) {
+ if (newValidCount >= estimatedSize) {
estimatedSize = newValidCount; // Update before
we send events.
+ if (hasMore) {
+ // Estimated size seems incorrect. Add some empty rows for
triggering a new page load.
+ estimatedSize = (int) Math.min(MAXIMUM_ROWS, newValidCount
+ NUM_PENDING_ROWS);
+ }
}
beginChange();
nextReplace(validCount, replaceTo, removed);
nextAdd(replaceTo, validCount = newValidCount);
endChange();
+ checkOverflow();
+ }
+ }
+
+ /**
+ * If we can not load more features stop the reading process.
+ *
+ * @todo Add some message in the widget for warning the user.
+ */
+ private void checkOverflow() {
+ if (validCount >= MAXIMUM_ROWS) {
+ interrupt();
}
}
@@ -189,6 +262,7 @@ final class FeatureList extends ObservableListBase<Feature>
{
*/
final void setNextPage(final FeatureLoader next) {
assert Platform.isFxApplicationThread();
+ assert nextPageLoader.isDone();
nextPageLoader = next;
if (next == null) {
final int n = estimatedSize - validCount;
@@ -234,8 +308,9 @@ final class FeatureList extends ObservableListBase<Feature>
{
if (isSizeExact && index >= estimatedSize) {
throw new IndexOutOfBoundsException(index);
}
- if (nextPageLoader != null) {
- BackgroundThreads.execute(nextPageLoader);
+ final FeatureLoader loader = nextPageLoader;
+ if (loader != null && loader.getState() == FeatureLoader.State.READY) {
+ BackgroundThreads.execute(loader);
}
return null;
}
diff --git
a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
index ba11213..2e6bc27 100644
---
a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
+++
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
@@ -36,15 +36,12 @@ import org.apache.sis.internal.gui.Resources;
* This task does not load all features; only {@value #PAGE_SIZE} of them are
loaded.
* The boolean value returned by this task tells whether there is more
features to load.
*
- * <p>Loading processes are started by {@link Initial} loader.
- * Only additional pages are loaded by ordinary {@code FeatureLoader}.</p>
- *
* @author Martin Desruisseaux (Geomatys)
* @version 1.1
* @since 1.1
* @module
*/
-class FeatureLoader extends Task<Boolean> implements Consumer<Feature> {
+final class FeatureLoader extends Task<Boolean> implements Consumer<Feature> {
/**
* Maximum number of features to load in a background task.
* If there is more features to load, we will use many tasks.
@@ -60,6 +57,13 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
private final FeatureTable table;
/**
+ * The feature set from which to get the initial configuration.
+ * This is non-null only for the task loading the first {@value
#PAGE_SIZE} instances,
+ * then become null for all subsequent tasks.
+ */
+ private final FeatureSet initializer;
+
+ /**
* The stream to close after we finished to iterate over features.
* This stream should not be used for any other purpose.
*/
@@ -83,23 +87,11 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
private int count;
/**
- * Creates a new loader. Callers shall invoke {@link
#initialize(FeatureSet)}
- * in the worker thread before to let the {@link #call()} method be
executed.
- *
- * @see #initialize(FeatureSet)
+ * Creates a new loader for the given set of features.
*/
- FeatureLoader(final FeatureTable table) {
- this.table = table;
- }
-
- /**
- * Initializes this task for reading features from the specified set.
- * This method is invoked by subclasses after construction but before
- * {@link #call()} execution.
- */
- final void initialize(final FeatureSet features) throws DataStoreException
{
- toClose = features.features(false);
- iterator = toClose.spliterator();
+ FeatureLoader(final FeatureTable table, final FeatureSet features) {
+ this.table = table;
+ initializer = features;
}
/**
@@ -107,17 +99,10 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
* The new task will load the next {@value #PAGE_SIZE} features.
*/
private FeatureLoader(final FeatureLoader previous) {
- table = previous.table;
- toClose = previous.toClose;
- iterator = previous.iterator;
- }
-
- /**
- * Returns the list where to add features.
- * All methods on the returned list shall be invoked from JavaFX thread.
- */
- private FeatureList destination() {
- return (FeatureList) table.getItems();
+ table = previous.table;
+ toClose = previous.toClose;
+ iterator = previous.iterator;
+ initializer = null;
}
/**
@@ -133,23 +118,39 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
* Invoked in a background thread for loading up to {@value #PAGE_SIZE}
features.
* If this method completed successfully but there is still more feature
to read,
* then {@link #iterator} will keep a non-null value and a new {@link
FeatureLoader}
- * should be prepared for reading of another page of features. In other
cases,
- * {@link #iterator} is null and the stream has been closed.
+ * well be prepared by {@link #succeeded()} for reading of another page of
features.
+ * In other cases, {@link #iterator} is null and the stream has been
closed.
*
* @return whether there is more features to load.
*/
@Override
protected Boolean call() throws DataStoreException {
- // Note: iterator.estimateSize() is a count or remaining elements.
- final int stopAt = (int) Math.min(iterator.estimateSize(), PAGE_SIZE);
+ final boolean isTypeKnown;
+ if (initializer != null) {
+ isTypeKnown = setType(initializer.getType());
+ toClose = initializer.features(false);
+ iterator = toClose.spliterator();
+ } else {
+ isTypeKnown = true;
+ }
+ /*
+ * iterator.estimateSize() is a count or remaining elements (not the
total number).
+ * If the number of remaining elements is equal to smaller than the
page size, try
+ * to read one more element in order to check if we really reached the
stream end.
+ * We do that because the estimated count is only approximate.
+ */
+ final long remaining = iterator.estimateSize();
+ final int stopAt = (remaining > PAGE_SIZE) ? PAGE_SIZE : 1 + (int)
remaining;
loaded = new Feature[stopAt];
try {
while (iterator.tryAdvance(this)) {
if (count >= stopAt) {
+ setMissingType(isTypeKnown);
return Boolean.TRUE; // Intentionally skip
the call to close().
}
if (isCancelled()) {
- break;
+ close();
+ return Boolean.FALSE;
}
}
} catch (BackingStoreException e) {
@@ -160,7 +161,8 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
}
throw e.unwrapOrRethrow(DataStoreException.class);
}
- close(); // Loading completed
or has been cancelled.
+ close(); // Loading completed
successfully.
+ setMissingType(isTypeKnown);
return Boolean.FALSE;
}
@@ -213,20 +215,38 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
}
/**
+ * Returns the list where to add features.
+ * All methods on the returned list shall be invoked from JavaFX thread.
+ */
+ private FeatureList destination() {
+ return (FeatureList) table.getItems();
+ }
+
+ /**
* Invoked in JavaFX thread after new feature instances are ready.
* This method adds the new rows in the table and prepares another
* task for loading the next batch of features when needed.
*/
@Override
- protected final void succeeded() {
+ protected void succeeded() {
final FeatureList addTo = destination();
if (addTo.isCurrentLoader(this)) {
- if (this instanceof Initial) {
- addTo.setFeatures(iterator.estimateSize(),
iterator.characteristics(), loaded, count);
+ final boolean hasMore = getValue();
+ if (initializer != null) {
+ final long remainingCount;
+ final int characteristics;
+ if (hasMore) {
+ remainingCount = iterator.estimateSize();
+ characteristics = iterator.characteristics();
+ } else {
+ remainingCount = 0;
+ characteristics = Spliterator.SIZED;
+ }
+ addTo.setFeatures(remainingCount, characteristics, loaded,
count, hasMore);
} else {
- addTo.addFeatures(loaded, count);
+ addTo.addFeatures(loaded, count, hasMore);
}
- addTo.setNextPage(getValue() ? new FeatureLoader(this) : null);
+ addTo.setNextPage(hasMore ? new FeatureLoader(this) : null);
} else try {
close();
} catch (DataStoreException e) {
@@ -242,7 +262,7 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
* @see FeatureTable#interrupt()
*/
@Override
- protected final void cancelled() {
+ protected void cancelled() {
final FeatureList addTo = destination();
final boolean isCurrentLoader = addTo.isCurrentLoader(this);
if (isCurrentLoader) {
@@ -277,55 +297,23 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
* Invoked in JavaFX thread when a loading process failed.
*/
@Override
- protected final void failed() {
+ protected void failed() {
cancelled();
}
/**
- * The task to execute in background thread for initiating the loading
process.
- * This tasks is created only for the first {@value #PAGE_SIZE} features.
- * For all additional features, an ordinary {@link FeatureLoader} will be
used.
- */
- static final class Initial extends FeatureLoader {
- /**
- * The set of features to read.
- */
- private final FeatureSet features;
-
- /**
- * Initializes a new task for loading features from the given set.
- */
- Initial(final FeatureTable table, final FeatureSet features) {
- super(table);
- this.features = features;
- }
-
- /**
- * Gets the feature type, initializes the iterator and gets the first
{@value #PAGE_SIZE} features.
- * The {@link FeatureType} should be given by {@link
FeatureSet#getType()} but this method is robust
- * to incomplete implementations where {@code getType()} returns
{@code null}.
- */
- @Override
- protected Boolean call() throws DataStoreException {
- final boolean isTypeKnown = setType(features.getType());
- initialize(features);
- final Boolean status = super.call();
- if (isTypeKnown) {
- setTypeFromFirst();
- }
- return status;
- }
- }
-
- /**
* Invoked when the feature type may have been found. If the given type is
non-null,
* then this method delegates to {@link
FeatureTable#setFeatureType(FeatureType)} in
* the JavaFX thread. This will erase the previous content and prepare new
columns.
*
+ * <p>This method is invoked, directly or indirectly, only from the {@link
#call()}
+ * method with non-null {@link #initializer}. Consequently the new rows
have not yet
+ * been added at this time.</p>
+ *
* @param type the feature type, or {@code null}.
* @return whether the given type was non-null.
*/
- final boolean setType(final FeatureType type) {
+ private boolean setType(final FeatureType type) {
if (type != null) {
Platform.runLater(() -> table.setFeatureType(type));
return true;
@@ -340,13 +328,15 @@ class FeatureLoader extends Task<Boolean> implements
Consumer<Feature> {
* incomplete implementations exist so we are better to be safe. If we can
not get the type
* from the first feature instances, we will give up.
*/
- final void setTypeFromFirst() throws DataStoreException {
- for (int i=0; i<count; i++) {
- final Feature f = loaded[i];
- if (f != null && setType(f.getType())) {
- return;
+ private void setMissingType(final boolean isTypeKnown) throws
DataStoreException {
+ if (!isTypeKnown) {
+ for (int i=0; i<count; i++) {
+ final Feature f = loaded[i];
+ if (f != null && setType(f.getType())) {
+ return;
+ }
}
+ throw new
DataStoreException(Resources.forLocale(table.textLocale).getString(Resources.Keys.NoFeatureTypeInfo));
}
- throw new
DataStoreException(Resources.forLocale(table.textLocale).getString(Resources.Keys.NoFeatureTypeInfo));
}
}
diff --git
a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
index 909cf88..c26e143 100644
---
a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
+++
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
@@ -99,7 +99,6 @@ public class FeatureTable extends TableView<Feature> {
final FeatureList items = (FeatureList) getItems();
if (!items.setFeatures(this, features)) {
featureType = null;
- items.clear();
getColumns().clear();
}
}
@@ -110,7 +109,7 @@ public class FeatureTable extends TableView<Feature> {
* determined from the given type.
*/
final void setFeatureType(final FeatureType type) {
- getItems().clear();
+ ((FeatureList) getItems()).clearUnsafe();
if (type != null && !type.equals(featureType)) {
final Collection<? extends PropertyType> properties =
type.getProperties(true);
final List<TableColumn<Feature,?>> columns = new
ArrayList<>(properties.size());
