Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#130: Add default content length to progress bar #286

Closed
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a6799be
Added default content length to progress bar
ndemirca Apr 12, 2024
be68571
added test data
ndemirca Apr 12, 2024
82a521f
Finished bug fix
ndemirca Apr 12, 2024
72b90ef
improved unittests
ndemirca Apr 12, 2024
7c81d4a
removed outdated JUnit dependency
ndemirca Apr 12, 2024
2651706
used wiremock for the unit tests
ndemirca Apr 15, 2024
301b5bb
Implemented change requests
ndemirca Apr 16, 2024
483311d
added improvements
ndemirca Apr 16, 2024
c4e6d0a
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca Apr 16, 2024
a95ac8d
implemented change requests
ndemirca Apr 17, 2024
1729d2e
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca Apr 22, 2024
0ad0b7b
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca Apr 23, 2024
356f7ec
Merge branch 'bug/130-Add-Default-Content-Length-To-ProgressBar' of h…
ndemirca Apr 23, 2024
9886857
line ending changes
ndemirca May 15, 2024
368f3a1
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca May 15, 2024
2526187
fixed compile issue
ndemirca May 15, 2024
d931dff
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
ndemirca May 17, 2024
7ac7fa4
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Jul 8, 2024
f7205aa
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Jul 17, 2024
1ecce2b
#130: cleanup
jan-vcapgemini Jul 17, 2024
fc4b554
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Aug 9, 2024
50d38c4
#130: fixed tests and PluginInstaller
jan-vcapgemini Aug 9, 2024
8dccee1
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Aug 28, 2024
acf8e96
#130: refactored progress bar
jan-vcapgemini Aug 30, 2024
ba27c39
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Aug 30, 2024
f3ccb0a
Update cli/src/main/java/com/devonfw/tools/ide/io/AbstractIdeProgress…
jan-vcapgemini Sep 2, 2024
df01ffe
Update cli/src/main/java/com/devonfw/tools/ide/io/IdeProgressBarConso…
jan-vcapgemini Sep 2, 2024
5a146b1
Merge branch 'main' into bug/130-Add-Default-Content-Length-To-Progre…
jan-vcapgemini Sep 2, 2024
c13439d
#130: implemented requested changes
jan-vcapgemini Sep 2, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
<dependency>
<groupId>me.tongfei</groupId>
<artifactId>progressbar</artifactId>
<version>0.10.0</version>
<version>0.10.1</version>
</dependency>
<dependency>
<groupId>org.jline</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@
import com.devonfw.tools.ide.log.IdeLogLevel;
import com.devonfw.tools.ide.log.IdeSubLoggerOut;

import me.tongfei.progressbar.ProgressBarBuilder;
import me.tongfei.progressbar.ProgressBarStyle;

/**
* Default implementation of {@link IdeContext} using the console.
*/
Expand Down Expand Up @@ -63,24 +60,6 @@ protected String readLine() {

@Override
public IdeProgressBar prepareProgressBar(String taskName, long size) {

ProgressBarBuilder pbb = new ProgressBarBuilder();
// default (COLORFUL_UNICODE_BLOCK)
pbb.setStyle(ProgressBarStyle.builder().refreshPrompt("\r").leftBracket("\u001b[33m│").delimitingSequence("")
.rightBracket("│\u001b[0m").block('█').space(' ').fractionSymbols(" ▏▎▍▌▋▊▉").rightSideFractionSymbol(' ')
.build());
// set different style for Windows systems (ASCII)
if (getSystemInfo().isWindows()) {
pbb.setStyle(ProgressBarStyle.builder().refreshPrompt("\r").leftBracket("[").delimitingSequence("")
.rightBracket("]").block('=').space(' ').fractionSymbols(">").rightSideFractionSymbol(' ').build());
}
pbb.showSpeed();
pbb.setTaskName(taskName);
pbb.setUnit("MiB", 1048576);
pbb.setInitialMax(size);
pbb.continuousUpdate();
pbb.setUpdateIntervalMillis(1);
return new IdeProgressBarConsole(pbb.build());
return new IdeProgressBarConsole(getSystemInfo(), taskName, size);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package com.devonfw.tools.ide.io;

/**
* Abstract implementation of {@link IdeProgressBar}.
*/
public abstract class AbstractIdeProgressBar implements IdeProgressBar {

/** The default value for missing content length */
public static final long DEFAULT_CONTENT_LENGTH = 10000000L;

private long currentProgress;
private final long maxLength;

/**
* @param maxLength the maximum length of the progress bar.
*/
public AbstractIdeProgressBar(long maxLength) {

this.maxLength = maxLength;
}

/**
* Increases the progress bar by given step size.
*
* @param stepSize size to step by.
* @param currentProgress
*/
protected abstract void doStepBy(long stepSize, long currentProgress);

/**
* Increases the progress bar by given step size.
*
* @param stepSize size to step by.
*/
protected abstract void doStepBy(long stepSize);

/**
* Sets the progress bar to given step position.
*
* @param stepPosition position to set to.
*/
protected abstract void doStepTo(long stepPosition);

@Override
public void stepBy(long stepSize) {

this.currentProgress += stepSize;
if (this.maxLength > 0) {
// check if maximum overflow
if (this.currentProgress > this.maxLength) {
this.currentProgress = this.maxLength;
doStepTo(this.maxLength);
return;
}
}

doStepBy(stepSize, this.currentProgress);
}

@Override
public void close() {
if (this.currentProgress < this.maxLength) {
Copy link
Member

Choose a reason for hiding this comment

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

handle maxLength < 0 (-1) case

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added a check for maxLength < 0 which will simply return and not change the progress bar anymore.

// TODO: Check if doStepTo should be used instead.
doStepBy(this.maxLength - this.currentProgress, this.currentProgress);
jan-vcapgemini marked this conversation as resolved.
Show resolved Hide resolved
}
}

}
3 changes: 2 additions & 1 deletion cli/src/main/java/com/devonfw/tools/ide/io/FileAccess.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ public interface FileAccess {
* @param url the location of the binary file to download. May also be a local or remote path to copy from.
* @param targetFile the {@link Path} to the target file to download to. Should not already exists. Missing parent directories will be created
* automatically.
* @param test boolean indicating if this is used in a test or not.
*/
void download(String url, Path targetFile);
void download(String url, Path targetFile, boolean test);
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this parameter? This seems odd to have in a regular API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a left over from the first draft and should be removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed now.


/**
* Creates the entire {@link Path} as directories if not already existing.
Expand Down
41 changes: 31 additions & 10 deletions cli/src/main/java/com/devonfw/tools/ide/io/FileAccessImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ private HttpClient createHttpClient(String url) {
}

@Override
public void download(String url, Path target) {
public void download(String url, Path target, boolean test) {

this.context.info("Trying to download {} from {}", target.getFileName(), url);
mkdirs(target.getParent());
Expand All @@ -109,7 +109,7 @@ public void download(String url, Path target) {
Path source = Path.of(url);
if (isFile(source)) {
// network drive
copyFileWithProgressBar(source, target);
copyFileWithProgressBar(source, target, test);
} else {
throw new IllegalArgumentException("Download path does not point to a downloadable file: " + url);
}
Expand All @@ -126,13 +126,10 @@ public void download(String url, Path target) {
* @param target Path of the target directory.
* @param response the {@link HttpResponse} to use.
*/
private void downloadFileWithProgressBar(String url, Path target, HttpResponse<InputStream> response) throws IOException {
private void downloadFileWithProgressBar(String url, Path target, HttpResponse<InputStream> response) {

long contentLength = response.headers().firstValueAsLong("content-length").orElse(0);
if (contentLength == 0) {
this.context.warning("Content-Length was not provided by download source : {} using fallback for the progress bar which will be inaccurate.", url);
contentLength = 10000000;
}
informAboutSettingDefaultContentLength(contentLength, url, null);

byte[] data = new byte[1024];
boolean fileComplete = false;
Expand All @@ -151,6 +148,7 @@ private void downloadFileWithProgressBar(String url, Path target, HttpResponse<I
pb.stepBy(count);
}
}

} catch (Exception e) {
throw new RuntimeException(e);
}
Expand All @@ -161,26 +159,49 @@ private void downloadFileWithProgressBar(String url, Path target, HttpResponse<I
*
* @param source Path of file to copy
* @param target Path of target directory
* @param test the flag about calling this method from test classes.
*/
private void copyFileWithProgressBar(Path source, Path target) throws IOException {
private void copyFileWithProgressBar(Path source, Path target, boolean test) throws IOException {
ndemirca marked this conversation as resolved.
Show resolved Hide resolved

try (InputStream in = new FileInputStream(source.toFile()); OutputStream out = new FileOutputStream(target.toFile())) {
long size;
if (test) {
size = 0L;
} else {
size = source.toFile().length();
}
Copy link
Member

Choose a reason for hiding this comment

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

if you need to test this somehow, extract to protected method long getFileSize(Path) create a subclass of FileAccessImpl for test that overrides and returns 0 instead. Then in your JUnit replace the real FileAccess with the test instance... Seems a little bit more complicated but adding this test flag to our official API seems odd to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've implemented this as you've suggested, thanks.


long size = source.toFile().length();
informAboutSettingDefaultContentLength(size, null, source);
byte[] buf = new byte[1024];
int readBytes;

try (IdeProgressBar pb = this.context.prepareProgressBar("Copying", size)) {
while ((readBytes = in.read(buf)) > 0) {
out.write(buf, 0, readBytes);
pb.stepByOne();
if (size > 0) {
pb.stepBy(readBytes);
}
}
} catch (Exception e) {
throw new RuntimeException(e);
}
}
}

private void informAboutSettingDefaultContentLength(long contentLength, String url, Path path) {

String source;
if (contentLength == 0) {
Copy link
Member

@hohwille hohwille Aug 30, 2024

Choose a reason for hiding this comment

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

why are you using 0 for undefined? IMHO -1 would make more sense also here...

Files can be 0 bytes large and then the user should not get this warning for every file that was empty but gets copied.
Also if the file size is know but smaller than e.g. 1k we should not give a progress bar.
What is good is that we have a dedicated method copyFileWithProgressBar for this use-case so we do not do this for every file. However, I assume this is used when we get a tool release package from a custom tool repository using a file URL and that can also be a network mount that is slower so already copying 1MB could take some seconds...

Copy link
Contributor

Choose a reason for hiding this comment

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

I've set the default for undefined to -1 now.

if (path != null) {
source = path.toString();
} else {
source = url;
}
this.context.warning("Content-Length was not provided by download/copy source: {}.",
source);
}
}

@Override
public void mkdirs(Path directory) {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,6 @@ public interface IdeProgressBar extends AutoCloseable {
*/
void stepBy(long stepSize);

/**
* Increases the progress bar by one step.
*/
default void stepByOne() {

stepBy(1);
}

@Override
void close();
}
Original file line number Diff line number Diff line change
@@ -1,33 +1,88 @@
package com.devonfw.tools.ide.io;

import com.devonfw.tools.ide.os.SystemInfo;

import me.tongfei.progressbar.ProgressBar;
import me.tongfei.progressbar.ProgressBarBuilder;
import me.tongfei.progressbar.ProgressBarStyle;

/**
* Implementation of {@link IdeProgressBar}.
*/
public class IdeProgressBarConsole implements IdeProgressBar {
public class IdeProgressBarConsole extends AbstractIdeProgressBar {

private final ProgressBar progressBar;
private final SystemInfo systemInfo;

/**
* The constructor.
*
* @param progressBar the {@link ProgressBar} to initialize.
* @param systemInfo the {@link SystemInfo}.
* @param taskName the {@link ProgressBar} to initialize.
* @param maxSize the maximum size of the progress bar.
*/
public IdeProgressBarConsole(SystemInfo systemInfo, String taskName, long maxSize) {

super(maxSize);

this.systemInfo = systemInfo;
this.progressBar = createProgressBar(taskName, maxSize);
}

/**
* Creates the {@link ProgressBar} initializes task name and maximum size as well as the behaviour and style.
*
* @param taskName name of the task.
* @param size of the content.
* @return {@link ProgressBar} to use.
*/
public IdeProgressBarConsole(ProgressBar progressBar) {
protected ProgressBar createProgressBar(String taskName, long size) {

this.progressBar = progressBar;
ProgressBarBuilder pbb = new ProgressBarBuilder();
// default (COLORFUL_UNICODE_BLOCK)
pbb.setStyle(ProgressBarStyle.builder().refreshPrompt("\r").leftBracket("\u001b[33m│").delimitingSequence("")
.rightBracket("│\u001b[0m").block('█').space(' ').fractionSymbols(" ▏▎▍▌▋▊▉").rightSideFractionSymbol(' ')
.build());
// set different style for Windows systems (ASCII)
if (this.systemInfo.isWindows()) {
pbb.setStyle(ProgressBarStyle.builder().refreshPrompt("\r").leftBracket("[").delimitingSequence("")
.rightBracket("]").block('=').space(' ').fractionSymbols(">").rightSideFractionSymbol(' ').build());
}
jan-vcapgemini marked this conversation as resolved.
Show resolved Hide resolved

pbb.setUnit("MiB", 1048576);
if (size == 0) {
pbb.setTaskName(taskName + " (unknown size)");
pbb.setInitialMax(-1);
pbb.hideEta();
} else {
pbb.setTaskName(taskName);
pbb.showSpeed();
pbb.setInitialMax(size);
}
pbb.continuousUpdate();
pbb.setUpdateIntervalMillis(1);

return pbb.build();
}

@Override
public void stepBy(long stepSize) {
protected void doStepBy(long stepSize, long currentProgress) {
doStepBy(stepSize);
}

@Override
protected void doStepBy(long stepSize) {
this.progressBar.stepBy(stepSize);
}

@Override
public void close() {
protected void doStepTo(long stepPosition) {
this.progressBar.stepTo(stepPosition);
}

@Override
public void close() {
super.close();
this.progressBar.close();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ protected Path download(String url, Path target, String downloadFilename, Versio

Path tmpDownloadFile = createTempDownload(downloadFilename);
try {
this.context.getFileAccess().download(url, tmpDownloadFile);
this.context.getFileAccess().download(url, tmpDownloadFile, false);
if (resolvedVersion.toString().equals("latest")) {
// Some software vendors violate best-practices and provide the latest version only under a fixed URL.
// Therefore if a newer version of that file gets released, the same URL suddenly leads to a different
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ private Path downloadPlugin(FileAccess fileAccess, String downloadUrl, Path tmpD
}
String fileName = String.format("%s-plugin-%s%s", this.getName(), pluginId, extension);
Path downloadedFile = tmpDir.resolve(fileName);
fileAccess.download(downloadUrl, downloadedFile);
fileAccess.download(downloadUrl, downloadedFile, false);
return downloadedFile;
}

Expand Down
5 changes: 3 additions & 2 deletions cli/src/main/java/com/devonfw/tools/ide/tool/mvn/Mvn.java
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,9 @@ private Set<String> findVariables(String content) {
@Override
public void installPlugin(PluginDescriptor plugin) {

Path mavenPlugin = this.getToolPath().resolve("lib/ext/" + plugin.getName() + ".jar");
this.context.getFileAccess().download(plugin.getUrl(), mavenPlugin);
Path mavenPlugin = this.context.getSoftwarePath().resolve(this.tool).resolve("lib/ext/" + plugin.getName() + ".jar");
this.context.getFileAccess().download(plugin.getUrl(), mavenPlugin, false);

if (Files.exists(mavenPlugin)) {
this.context.success("Successfully added {} to {}", plugin.getName(), mavenPlugin.toString());
} else {
Expand Down
Loading
Loading