implemented thumbnail preview for every picture attachment #299

cirquit · 2017-01-25T13:49:45Z

added javascript as well as selenium tests for this functionality
currently hardcoded scaling and saving the thumbs in the attachment directory

Looks like this:

added javascript as well as selenium tests for this functionality currently hardcoded scaling and saving the thumbs in the attachment directory

janschaefer

Look very good! There are only some minor things I have commented.

janschaefer · 2017-01-25T14:05:16Z

jgiven-html-app/src/js/util.js

@@ -225,4 +225,17 @@ export function replaceArguments(string, enumArray, nameArray) {
        }
    }
    return result.join("");
+}
+
+export function insertThumbnailPath(path) {


I would rather call this function getThumbnailPath. Maybe you could also add a short documentation.

janschaefer · 2017-01-25T14:07:27Z

jgiven-html-app/src/test/utilTest.js

+
+      it("works for paths with single number", function() {
+           var path0 = "/simple/data/path0.png";
+           expect(insertThumbnailPath(path0)).toEqual("/simple/data/path-thumb0.png");


Can we have that simpler and just have path0-thumb.png?

this would mean that the getTargetFile method from Html5AttachmentGeneratorwould have to be duplicated for thumbnails, mainly because of the enumeration strategy via multisets

I'm not sure if my solution to this is nice. I tried to minimize duplicate code, but the explicit using filehandles as an argument is maybe not the good practice.

janschaefer · 2017-01-25T14:08:19Z

jgiven-html5-report/src/main/java/com/tngtech/jgiven/report/html5/Html5AttachmentGenerator.java

@@ -105,6 +112,9 @@ private File writeFile( AttachmentModel attachment, MediaType mediaType ) {
        File targetFile = getTargetFile( attachment.getFileName(), extension );
        try {
            if( attachment.isBinary() ) {


I think that is not enough. The attachment could be binary, but no image (e.g. a ZIP file).

janschaefer · 2017-01-25T14:09:29Z

jgiven-html5-report/src/main/java/com/tngtech/jgiven/report/html5/Html5AttachmentGenerator.java

@@ -115,4 +125,41 @@ private File writeFile( AttachmentModel attachment, MediaType mediaType ) {
        return targetFile;
    }

+    private String compressToThumbnail( String base64content, String extension ) {
+        byte[] imageBytes = javax.xml.bind.DatatypeConverter.parseBase64Binary(base64content);


I would extract a method here that takes a BufferedImage and produces one and do the Base64 conversion outside of that.

janschaefer · 2017-01-25T14:13:35Z

jgiven-html5-report/src/main/java/com/tngtech/jgiven/report/html5/Html5AttachmentGenerator.java

@@ -105,6 +112,9 @@ private File writeFile( AttachmentModel attachment, MediaType mediaType ) {
        File targetFile = getTargetFile( attachment.getFileName(), extension );
        try {
            if( attachment.isBinary() ) {
+                File thumbFile = getTargetFile( "attachment-thumb", extension );
+                String thumbnail = compressToThumbnail( attachment.getValue(), extension );
+                Files.write( parseBase64Binary( thumbnail ), thumbFile );


That does not make sense to me, you first encode the thumbnail to Base64, to immediately decode it afterwards again?

change attachments_with_different_media_types to a non image binary because of the implicit conversion to thumbnails

cirquit · 2017-01-25T18:04:27Z

Addressed every issue and modified a test because of the now working automatic conversion to thumbnails of images.

cirquit · 2017-02-01T08:20:56Z

This solves #274

janschaefer · 2017-02-01T08:52:42Z

One last thing: could you add an entry to the changelog, pls?

cirquit · 2017-02-01T18:23:33Z

As discussed offline, this needs to be a configurable option. This will need a little bit of refactoring:

destill the global config for every possible ReportGenerator
create an abstract config parser for cmd arguments and give a specialized one to every report generator so we don't have a --customjs= flag for the AsciiReport
implement a --without-thumbs flag for the html Report
write this information in the meta-data of the html report and update the index.html to show the clip instead of the thumbnail
update gradle plugin
update maven plugin

cirquit · 2017-02-13T17:43:20Z

I updated the interface for the reports, would you mind taking a look at it?

Now the AbstractReport has the base functionality, with a exchangeable cmd-parsing option and is extendable via the 4 abstract methods that are called in the generate() method.

The ReportGenerator does not know anything except the --format= flag, and delegates everything to the implementation of the respective report. It's mainly for command line use, but is also configureable with an internal flagList.

Every other change is basically an adaptation for this interface.

janschaefer

The current implementation makes a high use of string literals. It would be better if you could keep the number of string literals low. At best there should be only one place where each string literal appears. In addition, I think that it would make sense to extract the complete option handling into classes that are separate from the actual report generators.

janschaefer · 2017-02-14T13:06:27Z

jgiven-core/src/main/java/com/tngtech/jgiven/report/AbstractReport.java

+ * The functionality is piped together for an easier and extendable interface to create a custom report.
+ *
+ */
+public abstract class AbstractReport {


Please name it AbstractReportGenerator

janschaefer · 2017-02-14T13:09:52Z

jgiven-core/src/main/java/com/tngtech/jgiven/report/AbstractReport.java

+
+    public void addFlags( String... flags ) {
+        for( String flag : flags ) {
+            this.flags.add( flag );


there should be an addAll Method that already does the loop for you

janschaefer · 2017-02-14T13:12:54Z

jgiven-core/src/main/java/com/tngtech/jgiven/report/AbstractReport.java

+    public void printUsageAndExit() {
+        System.err
+                .println( "Options: \n"
+                        + "  --format=<format>             the format of the report. Either html or text (required) \n"


It could be overkill, but I wonder whether you could not generalize your code further by introducing an CommandLineOption class that contains all information about a command line option, like description, deprecation and so on. Then you code will become more general in the end.

I will follow this train of thought.

janschaefer · 2017-02-14T13:24:39Z

...gradle-plugin/src/main/java/com/tngtech/jgiven/gradle/internal/AbstractJGivenReportImpl.java

-        generator.getConfig().setExcludeEmptyScenarios( isExcludeEmptyScenarios() );
-        if (getTitle() != null) {
-            generator.getConfig().setTitle( getTitle() );
+        generator.addFlag( "--targetDir=" + getDestination() );


I don't really like introducing String constants here. Why not keeping the old methods? Maybe it would be useful to separate the configuration option logic into a class that is separated from the generator?

The decision to use string constants benefits the extensibility of the ReportGenerator. It only knows the format and calls the respective report based on that, but it should not know the implementation details of that report.

The ReportGenerator is mainly a command line interface + the added interface to provide it with flags when instantiated as an object which has a single end point and doesn't have to be reimplemented generate(List<String> args.

The problem lies in the custom flags that every report may have - how does one access these custom configurations from the ReportGenerator that sees only an AbstractReport?

A compromise would be to set all already possibly known flag options like sourceDir/targetDir via a custom function, but every custom flag would still be passed as string, but that would be a weird interface to use.

I thought that the unified access point should be the command line flags, as this is the main usage of the reports.

The problem I see is that you have more or less introduced the command-line frontend into the backend code. This is typically a bad idea. It would be nicer to have clean separation of the frontend (parsing) and the backend. So that the backend does not have to know anything about how command line arguments are parsed/have to look like etc. So my suggestion would be the following:

Introduce a class for a command line option, something like:

class CommandLineOption { String longName; // mandatory, can be used as key later String shortName; String description; boolean hasValue; boolean optional; Object defaultValue; }

Have a generic method in the AbstractReportGenerator to specify additional CommandLineOption

Implement a CommandLineOptionParser that takes a list of CommandLineOption and an array of Strings and returns a map from String to Object, where String is the longName of the argument and Object the parsed value.

Have a generic method (in the command line parser) to get the value for an CommandLineOption like:

public Object getValue( CommandLineOption option) { return parsedOptions.get(option.getLongName()); }

If we are at it already, we could even go a step further and generalize this to completely abstract away where the configuration options come from. Something similar to what the TNG config-builder does: https://github.com/TNG/config-builder (but simpler, not using annotations etc). Because in the end, we also want to provide a way to configure the cssFile, for example, using properties. So we may need a general configuration option class like:

class ConfigurationOption { String longName; String shortName; String propertyString; String envString; boolean optional; boolean hasValue; String defaultValue; ... }

You should also provide a Builder class for that so that it easier to create instances of that class. And instead of only getting these options by using the command line arguments, we would also look for properties and environment variables.

In the end however, there should also be a class like ReportConfig and HtmlReportConfig that only contain fields with the final configuration values:

class ReportConfig { File targetDirectory; ... } class HtmlReportConfig extends ReportConfig { File cssFile; }

This is can then be used by the backend code.

cirquit · 2017-02-17T01:03:17Z

Refactored the Report creation with ConfigOptions. Some things are still kind of not optimal:

I had to let the AbstractConfig in AbstractConfigGenerator be public, because the gradle-plugin has two-step configuration (JGivenReportTask#generate and JGivenReport#createGenerator (previous version was named configure)
The encoding of the convertion function is done via an interface com.tngtech.jgiven.report.config.converter.StringConverter since we are using Java 7. There may be a better option to do this
the tests in com.tngtech.jgiven.report.WhenReportGenerator have to have the three different configurations so the corresponding reports get the correct instantion of the AbstractReportConfig. I don't know if my solution to downcast the AbstractReportConfig is the right way to create an interface for passing the configuration from the plugins or tests
the ConfigOption is prepared for the property / environment parsing as well as the ConfigOptionParser but it's not implemented yet

An introduced change:

the format for the command line use now defaults to html5, because the gradle/maven plugins do so

I just noticed that the help config option is not parsed anywhere but is a shared flag for every report, just like format. The difference to sourceDir is that this terminates the report and should be checked for in the ConfigOptionParser. I'm going to fix this on monday.

moved the logic of the command line flags to the AbstractConfig added suggestions for typos to the ConfigParser based on Levenshtein added docs

cirquit · 2017-03-15T14:40:54Z

Besides adding a stronger coupling for the ConfigOptions in the AbstractConfig, I added also a suggestion algorithm if the flags have typos.

This is done automatically for any future flags as well.

implemented thumbnail preview for every picture attachment

5f3f6aa

added javascript as well as selenium tests for this functionality currently hardcoded scaling and saving the thumbs in the attachment directory

cirquit added enhancement HTML Report labels Jan 25, 2017

janschaefer suggested changes Jan 25, 2017

View reviewed changes

addressed issues

b12e4b1

change attachments_with_different_media_types to a non image binary because of the implicit conversion to thumbnails

removed unnecessary junit runner with dataprovider

6ab5273

added changelog

3133ceb

janschaefer added the under development label Feb 7, 2017

added new interface for report creation, updated dependencies

dfd6dbd

cirquit added the core label Feb 13, 2017

cirquit added 2 commits February 13, 2017 22:36

added configurable option for thumbnails

0e2368d

Merge branch 'master' into issue274-preview-images

f4b41c3

janschaefer suggested changes Feb 14, 2017

View reviewed changes

refactored report generation with extendable ConfigOptions

c1daf50

cirquit added 2 commits February 20, 2017 22:39

refactored the help parsing to ConfigParser

f116263

stronger coupling for AbstractConfig / Report

69eb4ee

moved the logic of the command line flags to the AbstractConfig added suggestions for typos to the ConfigParser based on Levenshtein added docs

Merge branch 'master' into issue274-preview-images

18f4151

janschaefer merged commit 1f8fc7f into TNG:master Mar 20, 2017

cirquit deleted the issue274-preview-images branch March 22, 2017 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented thumbnail preview for every picture attachment #299

implemented thumbnail preview for every picture attachment #299

cirquit commented Jan 25, 2017

janschaefer left a comment

janschaefer Jan 25, 2017

janschaefer Jan 25, 2017

cirquit Jan 25, 2017

cirquit Jan 25, 2017

janschaefer Jan 25, 2017

janschaefer Jan 25, 2017

janschaefer Jan 25, 2017

cirquit commented Jan 25, 2017

cirquit commented Feb 1, 2017 •

edited

Loading

janschaefer commented Feb 1, 2017

cirquit commented Feb 1, 2017 •

edited

Loading

cirquit commented Feb 13, 2017 •

edited

Loading

janschaefer left a comment

janschaefer Feb 14, 2017

janschaefer Feb 14, 2017

janschaefer Feb 14, 2017

cirquit Feb 15, 2017

janschaefer Feb 14, 2017

cirquit Feb 15, 2017

janschaefer Feb 15, 2017

cirquit commented Feb 17, 2017 •

edited

Loading

cirquit commented Mar 15, 2017

implemented thumbnail preview for every picture attachment #299

implemented thumbnail preview for every picture attachment #299

Conversation

cirquit commented Jan 25, 2017

janschaefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirquit commented Jan 25, 2017

cirquit commented Feb 1, 2017 • edited Loading

janschaefer commented Feb 1, 2017

cirquit commented Feb 1, 2017 • edited Loading

cirquit commented Feb 13, 2017 • edited Loading

janschaefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cirquit commented Feb 17, 2017 • edited Loading

cirquit commented Mar 15, 2017

cirquit commented Feb 1, 2017 •

edited

Loading

cirquit commented Feb 1, 2017 •

edited

Loading

cirquit commented Feb 13, 2017 •

edited

Loading

cirquit commented Feb 17, 2017 •

edited

Loading